voryx / Thruway

PHP Client and Router Library for Autobahn and WAMP (Web Application Messaging Protocol) for Real-Time Application Messaging
MIT License
676 stars 117 forks source link

added support for new Symfony 5 #331

Closed kuraobi closed 4 years ago

kuraobi commented 4 years ago

This PR adds support for Symfony 5 and the new EventDispatcher, without breaking compatibility with ^2.7|^3.0|^4.0.

This is a bit tricky because Thruway\Event\Event and Thruway\Event\EventDispatcher both extend/implement Symfony versions which are not consistent across all supported versions.

So my reasoning is:

People using the lib can either continue to use dispatch(), which will behave like the one from the Symfony version they use, or use the backwardsCompatibleDispatch() which will behave like the newer version of dispatch(), even on older versions of Symfony, if they want to keep support for both older and newer versions of Symfony.

If they don't use Symfony, they can avoid a breaking change with their code by requiring "symfony/event-dispatcher": "^4", or even "symfony/event-dispatcher": "^3" for older PHP versions.

I also added the 2 classes from thruway/ratchet-transport because unless I missed something, they can't work without voryx/Thruway and voryx/Thruway requires them too, so it's a bidirectional dependency.

kuraobi commented 4 years ago

Tests are OK but I get some random travis errors

kuraobi commented 4 years ago

Complete support of Symfony 5 will also need upstream: https://github.com/ratchetphp/Ratchet/pull/766

mbonneau commented 4 years ago

@kuraobi - Thank you for the PR. On this issue it is better if we part ways with the Symfony EventDispatcher. We originally used this because it was available and easy, but if the API has changed, we should just pull the working code from Symfony (with the LICENSE and attribution in the source) and put it into Thruway.

This way people will be free to use Thruway without it interfering with Symfony (as they change much faster than Thruway does).

Would you mind reworking the PR?

kuraobi commented 4 years ago

@kuraobi - Thank you for the PR. On this issue it is better if we part ways with the Symfony EventDispatcher. We originally used this because it was available and easy, but if the API has changed, we should just pull the working code from Symfony (with the LICENSE and attribution in the source) and put it into Thruway.

This way people will be free to use Thruway without it interfering with Symfony (as they change much faster than Thruway does).

Would you mind reworking the PR?

Well actually, because Thruway directly extends and publicly exposes Symfony's Event and EventDispatcher APIs, unsyncing with Symfony would be a breaking change, no matter how you do it. So I wouldn't do that personnally, but it's your decision.

If you do want to part from Symfony, then you should fork symfony/event-dispatcher as voryx/event-dispatcher, then in voryx/thruway, replace "symfony/event-dispatcher" : "^2.7|^3.0|^4.0" with "voryx/event-dispatcher": "^2.8|^3.4", and you're done. The nice thing is that if you eventually decide to catch up with Symfony, you can just update your fork. I can't do it for you because I don't have the rights to create the fork.

davidwdan commented 4 years ago

Replaced by #343