Skip to content

Possibility to set persistent connection flag when creating socket - #57

Merged
jmglsn merged 2 commits into
stomp-php:masterfrom
kubasimon:feature/persistent-connection
Jul 29, 2016
Merged

Possibility to set persistent connection flag when creating socket#57
jmglsn merged 2 commits into
stomp-php:masterfrom
kubasimon:feature/persistent-connection

Conversation

@kubasimon

Copy link
Copy Markdown
Contributor

Added posibility to create socket with STREAM_CLIENT_PERSISTENT.

Can you point me what (unit) tests to create?

Comment thread src/Stomp/Network/Connection.php Outdated
* @throws ConnectionException
*/
public function __construct($brokerUri, $connectionTimeout = 1, array $context = [])
public function __construct($brokerUri, $connectionTimeout = 1, array $context = [], $persistentConnection = false)

@staabm staabm Jul 28, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

instead of adding even more __construct() args I would prefer a dedicated setter.

makes client code more readable.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe we should add a setter for stream_socket_client - flags?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see no more flags which would be usefull.
the benefit of having dedicated setters is, that the Connection class api hides the underlying implementation details.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, constructor param replaced with setter. Does it look like you thought?:)

@staabm

staabm commented Jul 28, 2016

Copy link
Copy Markdown
Member

LGTM thx 👍

@jmglsn jmglsn added this to the Release 4.1 milestone Jul 29, 2016
@jmglsn
jmglsn merged commit 31aa588 into stomp-php:master Jul 29, 2016
@kubasimon

Copy link
Copy Markdown
Contributor Author

I wanted to squash commits to one, but nevermind:)

@jmglsn

jmglsn commented Jul 29, 2016

Copy link
Copy Markdown
Member

Sorry too fast and maybe a little bit too early this day for me ;)

@jmglsn

jmglsn commented Jul 29, 2016

Copy link
Copy Markdown
Member

Anyway now it's available in https://github.com/stomp-php/stomp-php/releases/tag/4.1.3 thanks!

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