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

Removed obsolete RingCentral/Psr7 dependency #485

Closed

Conversation

vanodevium
Copy link

ringcentral/psr7 hasn't been updated in over 5 years.

Need to keep up with the times.

None of the existing PSR7 implementations work on PHP >=5.3

So I collect code from the latest versions of guzzle/psr7 and nyholm/psr7 and made own local realization which can work on PHP 5.3 and upper.

This code passes all tests.

if you're interested, I can continue work on modifying any existing implementation enough to create your own React/Psr7 package.

@SimonFrings
Copy link
Member

@vanodevium thanks for bringing this up, always happy about contributions 👍

I agree that it makes sense to add this as a part of ReactPHP at some point and I really appreciate the effort you put into this, but I don't think this is the best way to do it for various reasons. Using existing code from other projects running under the MIT license need to be addressed, otherwise we would violate this license.

Additionally I think we should also add only the necessary classes and functions that we use in our APIs, we don't want to bloat the project with unneeded functionality (which is also marked internal, so if we don't use it, who will).

Apart from that we also need to add tests for the new added classes. Updating the existing tests to use the newer classes is correct in itself, but it doesn't cover every new function added in this PR.

We're aware that we can't use guzzle/psr7 and nyholm/psr7 because of old PHP, but this won't be the case once ReactPHP v3 is out which gets rid of "old" PHP. Maybe it makes sense to wait for v3 to come out, which will remove a lot of the old stuff and then focus on implementing this as a part of ReactPHP.

What do you think?

@vanodevium
Copy link
Author

I'm glad that you like it.

With this code, I just showed that, in general, you can get rid of the old dependency and make your own internal implementation.

If you are interested, just let me know and I will write an implementation with my own hands that will not violate any licenses.

Regarding the tests, I do not quite agree: I only changed the base classes that the project classes inherit, but they are already completely covered by tests and there is no new functionality.

Of course, if it is possible to wait for a new version of your project, then it is better to wait.

Thank you for paying attention to my work. Always glad to take part and contribute to the development of great projects!

@vanodevium
Copy link
Author

one more thing

I absolutely fully understand what I did in this code, because ringcentral/psr7 is actually a very old fork of the guzzle/psr7

Technically, I just updated it :) with NO breaking changes

And last thing:
maybe it will be nice to integrate psr7-integration-tests

@SimonFrings
Copy link
Member

If you are interested, just let me know and I will write an implementation with my own hands that will not violate any licenses.

I like your motivation 👍 @clue told me about his vision on how we can integrate this into ReactPHP's HTTP component, but I think we have to work out the details first and come up with a more solid plan for this. As I said above, we're currently busy with the ReactPHP v3 development, which I think will be a great foundation for a future PSR-7 implementation.

Better support for PSRs is also one of our goals for ReactPHP v3.x as described in our plans for the upcoming major version, not sure how much of it will make it in time for the v3 release tho.

@SimonFrings
Copy link
Member

The RingCentral dependency has now officially been removed in #522 and we're now using our own PSR-7 implementation instead (see #518, #519, #520, #521)

This means there's nothing more to do here and I'll close this pull request. Thank you @vanodevium for working on this! ❤️

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

Successfully merging this pull request may close these issues.

None yet

2 participants