Skip to content

Conversation

@clue
Copy link
Member

@clue clue commented Feb 6, 2017

Filing this as a WIP until the v0.5 release has actually been tagged and released.

Refs reactphp/http#90

composer.json Outdated
, "require": {
"php": ">=5.3.9"
, "react/socket": "^0.3 || ^0.4"
, "react/socket": "dev-master as 0.5"
Copy link
Member Author

Choose a reason for hiding this comment

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

This should be ^0.5 once the release is out.

@clue clue changed the title [WIP] Update Socket component to v0.5 Update Socket component to v0.5 Feb 14, 2017
@clue
Copy link
Member Author

clue commented Feb 14, 2017

Updated and rebased now that the Socket v0.5.0 release is out :shipit:

@cboden cboden mentioned this pull request Feb 17, 2017
@cboden cboden added this to the 0.4 milestone Feb 17, 2017
@cboden
Copy link
Member

cboden commented Feb 17, 2017

The good news: I've merged this into the 0.4 branch

The bad news: As ServerInterface is a parameter for Ratchet's Server constructor signature this is a BC break so I can't merge/tag it into an 0.3 release (master).

@clue
Copy link
Member Author

clue commented Feb 17, 2017

I understand that it may be safer to bump the major version here, but I still think my argument holds that this is actually not a BC break at all, see reactphp/http#90 (comment) (and following) for more details.

I'll leave this up to you 👍

Is there anything I can do to help this PR progress?

Technically, it's possible to support the new Socket API as well as the old Socket API at the same time by using duck-typing and/or reflection. Would you be interested in this here?

@andig
Copy link

andig commented Feb 26, 2017

Any chance to get this going? Currently its impossible to use php-pm together with ratchet :(

@clue
Copy link
Member Author

clue commented Feb 27, 2017

See my comment above, I still think my arguments holds that this is actually not a BC break at all and I'm open to support both Socket versions if there's demand 👍

@andig What do you suggest here? Is this really something that ought to be fixed in Ratchet? If so, how?

@andig
Copy link

andig commented Feb 28, 2017

@andig What do you suggest here? Is this really something that ought to be fixed in Ratchet? If so, how?

My proposal/wish would be for a ratchet release supporting socket 0.5. For our application based on php-pmand hence react/http 0.5 is a reasonable minimum required version. Right now we're in a bit of a hole as we can't use ppm and ratchet at the same time due to ratchet being locked at 0.4.

A first 0.4 release of ratchet should be suitable to address any type of BC break?, potentially movong other breaking changes to 0.5 then?

@clue
Copy link
Member Author

clue commented Mar 10, 2017

Updated to fix event arguments and explicitly pass required arguments, see also reactphp/stream#69 for rationale.

This failing tests on Travis are unrelated, it looks like PHP 5.6 and PHP 7 picks an incompatible PHPUnit version.

@andig
Copy link

andig commented Mar 11, 2017

ping @cboden is there any chance to get 0.4 rolling- if thats the way to go- with socket 0.5 support? Working on http://volkszaehler.org which uses both ratchet and react/http (ancient commit unfortunately due to backed out code) I'd highly appreciate if all libraries were installable again with socket 0.5 as least common denominator.

@maciejmrozinski
Copy link

@cboden bump :) seeing socket 0.5 support merged will be great.

Socket v0.7 and v0.6 contain some major changes, but this does not
affect usage within Ratchet, so it's actually compatible with all latest
releases.
@clue
Copy link
Member Author

clue commented Apr 17, 2017

I've updated this PR to include support for Socket v0.7 and v0.6 releases. Socket v0.7 and v0.6 contain some major changes, but this does not affect usage within Ratchet, so it's actually compatible with all latest releases.

Is there anything I can do to help this PR progress?

Technically, it's possible to support the new Socket API as well as the old Socket API at the same time by using duck-typing and/or reflection. Would you be interested in this here?

@cboden What's the status on this PR? Will this be cherry-picked over to your release branch and/or will this PR ever be merged into the current release branch or do you plan to close this eventually?

@cboden
Copy link
Member

cboden commented Apr 19, 2017

Because Ratchet's __construct accepts a React Socket instance and I can't guarantee everyone uses ::factory and Socket's signature changed this is indeed an API break. The expectation is when someone requires Ratchet ^0.3 and run their server it will still work as it did before, whereas that would not be the case with this PR.

However, I've spent a bit of time recently, and just need a bit more time to finish documenting 0.4 then I can release it with this PR and #500

@clue
Copy link
Member Author

clue commented Apr 19, 2017

@cboden I still think that the above argument still holds that this does in fact not break Ratchet's API, but actually only the API of a dependency of Ratchet. This is also pointed out in the related discussion for react/http linked above (reactphp/http#90 (comment)). See also http://semver.org/#what-should-i-do-if-i-update-my-own-dependencies-without-changing-the-public-api

Anyway, I agree that we can play "safe" here just in case and bump Ratchet to 0.4, but this will mean that users with ancient versions of PHP will not receive any updates because of other changes in the 0.4 release branch. I'll leave this up to you to decide 👍

@clue
Copy link
Member Author

clue commented Jun 26, 2017

Is there anything I can do to help this PR progress? :shipit:

I'd like to move forward here and either get this in or close this, how can I help?

Socket v0.8 only contains some minor breaking changes, which can be
circumvented by ignoring URI schemes here.
Future Socket v1.0 will not contain any BC breaks, so it's actually
compatible with the last release.
@clue
Copy link
Member Author

clue commented Jul 19, 2017

I've just updated this to also support upcoming react/socket v1.0 and current v0.8 :shipit:

@clue clue changed the title Update Socket component to v0.5 Update Socket component to support v0.5 through upcoming v1.0 Jul 19, 2017
@cboden cboden merged commit a86be3c into ratchetphp:master Sep 14, 2017
@clue clue deleted the socket branch September 14, 2017 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants