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

Implement an HTTP client based on Httplug rather than Guzzle 3 #16

Merged
merged 1 commit into from
May 22, 2017

Conversation

stof
Copy link
Contributor

@stof stof commented Apr 29, 2017

People wanting to upgrade to the new version will need to require a HTTPlug client implementation. Otherwise, Composer will reject the update (if you update the requirement in your composer.json, composer will fail saying it cannot find an implementation for the virtual package. If you don't bump the min version, it will use the older version of panda-client).

Once you have it, there is no change for people using the Api class, as it creates the client internally. It will use the new HttplugClient.

For people instantiating the Cloud themselves (hint: PandaBundle), the old Guzzle 3 client is still there, but deprecated. However, Guzzle 3 is not installed automatically anymore. Projects using the client will need to require guzzle/guzzle themselves or switch to the new HttplugClient. This is a small BC break currently (unfortunately, it may affect the bundle).

Unfortunately, the only way to be fully BC would be to keep the Guzzle 3 requirement, as this is where the BC break occurs: the legacy HTTP client depends on Guzzle 3 but we don't require it anymore. And I don't want to keep the requirement as Guzzle 3 is abandoned and incompatible with Symfony 3.
Fortunately, if your project does not already use HTTPlug, Composer will not update to the new version of panda client automatically (as you need to select a client implementation). So we may document it in the release notes.

Closes #6

@stof
Copy link
Contributor Author

stof commented Apr 29, 2017

@xabbuh do you agree with dropping support for PHP 5.3 and 5.4, which are long EOLed ? IF no, we cannot switch to HTTPlug.

@stof
Copy link
Contributor Author

stof commented Apr 30, 2017

@xabbuh I'm all for dropping support for PHP 5.3 and 5.4 for that. IMO, switching from the unmaintained Guzzle 3 to HTTPlug is better than keeping support for unmaintained PHP versions. People wanting to keep using such old PHP can also keep using an unmaintained version of panda-client based on Guzzle 3.
But I will let you decide what the new minimum version should be (5.5, 5.6, 7.0 or 7.1)

@stof
Copy link
Contributor Author

stof commented Apr 30, 2017

@xabbuh can you take a decision about the min PHP version ? This will allow me to finish this PR.

@xabbuh
Copy link
Owner

xabbuh commented Apr 30, 2017

@stof I created a 1.3 branch in which I dropped support for all PHP versions before 5.6. I'd like to merge this PR there and then get rid of the legacy Guzzle client in the master branch (I will probably also drop support for PHP 5.6 there, but I am not completely sure about that yet). Can you retarget your PR to the 1.3 branch?

@stof stof changed the base branch from master to 1.3 April 30, 2017 15:53
@stof
Copy link
Contributor Author

stof commented Apr 30, 2017

@xabbuh why having the 1.3, 1.x and master branches ? We don't need 2 separate dev branches for 1.x IMO (especially when they don't have diverging history).

Thus, are you starting the work on a 2.0 version ? IF no, splitting 1.x and master branches now is just making maintenance harder.

@stof
Copy link
Contributor Author

stof commented Apr 30, 2017

PR is now ready though.

The old Guzzle 3 client is still there, but deprecated. However, Guzzle 3
is not installed automatically anymore. Projects using the client
automatically will need to require guzzle/guzzle themselves.
@stof
Copy link
Contributor Author

stof commented May 2, 2017

and --prefer-lowest is now passing

@xabbuh xabbuh merged commit 0a04534 into xabbuh:1.3 May 22, 2017
xabbuh added a commit that referenced this pull request May 22, 2017
…zle 3 (stof)

This PR was merged into the 1.3 branch.

Discussion
----------

Implement an HTTP client based on Httplug rather than Guzzle 3

People wanting to upgrade to the new version will need to require a HTTPlug client implementation. Otherwise, Composer will reject the update (if you update the requirement in your composer.json, composer will fail saying it cannot find an implementation for the virtual package. If you don't bump the min version, it will use the older version of panda-client).

Once you have it, there is no change for people using the `Api` class, as it creates the client internally. It will use the new `HttplugClient`.

For people instantiating the Cloud themselves (hint: PandaBundle), the old Guzzle 3 client is still there, but deprecated. However, Guzzle 3 is not installed automatically anymore. Projects using the client will need to require `guzzle/guzzle` themselves or switch to the new HttplugClient. This is a small BC break currently (unfortunately, it may affect the bundle).

Unfortunately, the only way to be fully BC would be to keep the Guzzle 3 requirement, as this is where the BC break occurs: the legacy HTTP client depends on Guzzle 3 but we don't require it anymore. And I don't want to keep the requirement as Guzzle 3 is abandoned and incompatible with Symfony 3.
Fortunately, if your project does not already use HTTPlug, Composer will not update to the new version of panda client automatically (as you need to select a client implementation). So we may document it in the release notes.

Closes #6

Commits
-------

0a04534 Implement an HTTP client based on Httplug rather than Guzzle 3
@xabbuh
Copy link
Owner

xabbuh commented May 22, 2017

Thank you @stof.

@stof stof deleted the httplug branch May 22, 2017 17:20
xabbuh added a commit to xabbuh/PandaBundle that referenced this pull request May 30, 2017
This PR was squashed before being merged into the 1.3.x-dev branch (closes #21).

Discussion
----------

Update the bundle to use the HTTPlug integration

Depends on xabbuh/panda-client#16

The work needed to support it is actually quite small. It is the first commit of this PR (and currently, it contains extra changes to use my fork until the PR is merged, but I will amend it once the PR is merged in panda-client).

The third commit is about making it possible to provide your own HTTPlug client (and also the message factory and the stream factory, even though it is less needed for them as the factory themselves are not configurable so the option is useful only to force a different factory than the one selected by the discovery layer when you have multiple ones installed).
The second commit is the preparatory work for the third commit: moving the instantiation of the HttplugClient to the DIC rather than using the CloudFactory, to be able to inject extra services easily. I deprecated the CloudFactory in the process (it is not tagged as `@internal`, but it clearly looks like an internal API as it is meant to be a factory service, and I got rid of it).

Note that configuring the HTTPlug client applies to all clouds. I have not found it useful to allow separate configuration per cloud (but it would be possible to implement it in a BC way in the future).

Commits
-------

78bd121 Update the bundle to use the HTTPlug integration
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