Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unneeded ConnectionInterface methods getState(), getOptions(), setOptions() and getServerOptions() #68

Merged
merged 3 commits into from
Jul 6, 2018

Conversation

clue
Copy link
Contributor

@clue clue commented Jul 6, 2018

This simple PR removes the (mostly under-documented) getState(), getOptions(), setOptions() and getConnectionOptions() methods from the ConnectionInterface. As of #64 and #65 the connection state is handled internally and is nothing consumers should have to worry about. Similarly, this now uses a URI for connecting instead of connection options.

This is a BC break obviously, but empirical evidence including our examples suggests this should not affect most consumers.

Builds on top of #64 and #65

});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The creation of the mock should be done outside the callback to ensure it is tracked by PHPUnit.

$quitMock = $this->expectCallableOnce();

$conn->ping()->then(function () use ($conn, $quitMock) {
    $conn->quit()->then($quitMock);
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, updated 👍

@WyriHaximus WyriHaximus merged commit 6aa72dc into friends-of-reactphp:master Jul 6, 2018
@clue clue deleted the api-cleanup branch July 6, 2018 12:44
@iNewLegend
Copy link

Why getState() was deleted?, i found it useful - there is no way to access it. or i missing something?

@clue
Copy link
Contributor Author

clue commented Jul 7, 2018

@clue As of #64 and #65 the connection state is handled internally and is nothing consumers should have to worry about.

@iNewLegend Why getState() was deleted?, i found it useful - there is no way to access it. or i missing something?

Perhaps you can elaborate on your use case and what you need this for? It's my understanding this should be obsolete for the new interfaces. Also, removing this allows us to build lazy connections and connection pools (#27) in the future (pseudo-connections that are in an "idle" state and automatically connect on demand) 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants