Remove dependency on Guzzle #41
Conversation
|
Hi, Thanks for your contribution. It looks like a great change! I'm not à PHP developer, but what will happen to code passing a Guzzle instance to the constructor which now expects an adapter ? Is it possible to handle this case to avoid breaking the API ? |
|
Hello! Insuring non BC break here is a nice idea but it force to include Guzzle as a required dependency. With that we are not taking all the benefits from the decoupling... I saw that OVH Api change its major version number from Guzzle5 to Guzzle6. Maybe we can use the same here because the HTTP adapter is the hearth of the lib. If you really want to avoid BC and staying on a 2.x, we can make a decorator to handle the previous Guzzle behaviour and make that decorator the default implementation... |
|
Can I assist somehow to make sure this PR get merged? @yadutaf are you 👍 for a change like this? |
|
Any news on that PR ? |
|
ping @VincentCasse |
`php-http/guzzle-adapter` is still required as dev dependency because it is used as the base adapter. All the unit tests required that client to work.
All the data are computed before `Request` creation. It allow to perform a simple `new Request` with all the analysis result. Headers are set with authentication, URI is cleaned...
All the calls go through that method, it'll allow extensibility.
Guzzle is still used inside the test logic with middlewares.
Allow to perform a dynamic client resolution based on the current context. Require the puli composer extension to be registered : https://php-http.readthedocs.org/en/latest/discovery.html#installation
shulard
force-pushed
the
feature/httplug
branch
from
f386e48
to
139b36d
Compare
|
Hello guys, I've rebased on master and fixed the conflicts. Are you open to accept a change like this ? |
|
I would also be very interesting in having this merged. I'm trying to integrate this OVH library within a project already using Guzzle5: it's quite problematic. These changes would make it possible to avoid these conflicts. |
|
Interested too, with Symfony 4, PSR7, HttPlug is recommended. |
|
Some files have the conflict because the PR is older than master branch. These files should be fixed. |
|
Of course, this PR is 2 years old 😄. I can update the content, but are the maintainers ok with that kind of update ? If yes I'll work on the rebase asap. |
👋 hello :) we at Nextcloud have a community PR pending that wants to integrate with OVH: nextcloud/twofactor_gateway#316. Thus I would kindly like to know if there is any progress on this matter and/or if anything is missing that could be helped with. What is the status here? :) Cheers |
|
Some news about this PR ? Is there any chance that is merged ? |
|
I think merging would solve a lot of compatibility issues. |
|
Hello ! This is a 6 years old PR, I think that this change will never be merged inside this library. |
We discussed in #34 about the fact that this library is fully coupled to Guzzle. I've worked on that PR to use the php-http/httplug interfaces which allow to decouple from any HTTP client.
In the dev dependencies I still require the Guzzle6 adapter to make all the tests passing.
With that update, it's possible to use the OVH API with all the Httplug adapters (ReactPHP, Socket, Curl, Guzzle5, Guzzle6, Buzz...).
All the projects which requires the OVH API must also require an adapter. It also allow to use the same HTTP client than in the project (just require the adapter).
I've also implemented the HttplugDiscovery plugin which allow to automatically resolve the currently available package through Puli. The only requirement is that the user include the puli composer plugin.
If you have any questions about the implementation, just ask 😄.
The library usage is the same as before, we can pass an
HttpClientinstance to the Api client or let the Discovery package resolve the current client.