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

Handle stream_socket_get_name return "\0" #92

Closed
andig opened this issue Apr 17, 2017 · 5 comments · Fixed by #100
Closed

Handle stream_socket_get_name return "\0" #92

andig opened this issue Apr 17, 2017 · 5 comments · Fixed by #100

Comments

@andig
Copy link
Contributor

andig commented Apr 17, 2017

https://github.com/reactphp/socket/blob/master/src/Connection.php#L44 should not only check for falsebut also for "\0" according to https://bugs.php.net/bug.php?id=74458 https://bugs.php.net/bug.php?id=74556.

Discovered through reactphp/http#160 (comment). Since ratchet still uses 0.5 it would be great if this could also be back ported to 0.5...0.7.

@clue
Copy link
Member

clue commented Apr 17, 2017

Thanks for spotting and filing both an upstream bug report and this issue!

This is something I've also spotted a couple of months ago when these methods have been added but didn't get to look into this in-depth. I agree that this is something that should be fixed (worked around) here and PRs would be much appreciated 👍

I'd like to address this in the master branch first and then check whether we should either backport this to previous release branches or (which I prefer) encourage consumers to upgrade to the latest version if this is reasonable 👍

@andig
Copy link
Contributor Author

andig commented Apr 17, 2017

I can add the PR for you and potentially even a test (need to check). I'd need it backported too since I'm using ppm and react/http and ratchet which forces me to stay on 0.5 for time being due to ratchet.

@WyriHaximus
Copy link
Member

@andig that would be most appreciated 👍

@andig
Copy link
Contributor Author

andig commented Apr 17, 2017

No idea about the tests, sorry. If you want more PRs for the different branches please let me know.

@clue
Copy link
Member

clue commented Apr 17, 2017

I'd need it backported too since I'm using ppm and react/http and ratchet which forces me to stay on 0.5 for time being due to ratchet.

I've just updated ratchetphp/Ratchet#485 to make sure Ratchet will also support Socket v0.7 and v0.6 :shipit:

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

Successfully merging a pull request may close this issue.

3 participants