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

[PHP] 6.0.x make php implementation depend on meta packages for http client #9772

Closed

Conversation

olexiyk
Copy link

@olexiyk olexiyk commented Jun 15, 2021

Instead of relying on Guzzle directly, depend on a psr/http-client-implementation (AKA PSR 18). This is a metapackage which implementations can declare they provide, all the major ones already do (including Guzzle, Symfony, etc).

Inspired by discussion: #7518

PR checklist

  • [ x ] Read the contribution guidelines.
  • [ x ] Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • [ x ] Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • [ x ] File the PR against the correct branch: master, 5.1.x, 6.0.x
  • [ x ] If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@jebentier (2017/07), @dkarlovi (2017/07), @mandrean (2017/08), @jfastnacht (2017/09), @ackintosh (2017/09) ❤️, @ybelenko (2018/07), @renepardon (2018/12)

Copy link
Contributor

@dkarlovi dkarlovi left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for working on this! <3

Some questions I'd like to discuss:

olexiyk added 2 commits June 18, 2021 13:32
Added missing composer requirements
Calling parse_url once
@olexiyk olexiyk marked this pull request as draft June 18, 2021 15:39
@olexiyk
Copy link
Author

olexiyk commented Jun 18, 2021

Update from 2021-07-05 redirect problem was solved by upgrading php-http/client-common, PHP Unit tests are passing now


Some tests are failing and I figured out the reason, I don't have a solution for it yet.

http://petstore.swagger.io/v2 is a base url we are running tests against
any request to that host redirects to https://petstore.swagger.io/v2

The redirect plugin from here https://github.com/php-http/plugins/blob/master/src/RedirectPlugin.php#L197
when doing a redirect is updating method from POST to GET. In the end, the response to the second request is 405 Method not allowed.

I read a bit about that and to me it looks that probably RedirectPlugin should not change POST to GET, but that is a common implementation bug

https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.3

If the 302 status code is received in response to a request other than GET or HEAD, the user agent MUST NOT automatically redirect the request unless it can be confirmed by the user, since this might change the conditions under which the request was issued.

Note: RFC 1945 and RFC 2068 specify that the client is not allowed
to change the method on the redirected request. However, most
existing user agent implementations treat 302 as if it were a 303
response, performing a GET on the Location field-value regardless
of the original request method. The status codes 303 and 307 have
been added for servers that wish to make unambiguously clear which
kind of reaction is expected of the client.

As well seen at
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/302

Even if the specification requires the method (and the body) not to be altered when the redirection is performed, not all user-agents conform here - you can still find this type of bugged software out there. It is therefore recommended to set the 302 code only as a response for GET or HEAD methods and to use 307 Temporary Redirect instead, as the method change is explicitly prohibited in that case.

I am not sure on how to solve that issue yet, I will get back to the PR after one week

… for tests to pass

Previously POST requests were redirected as GET requests and that was causing tests to fail
@olexiyk olexiyk marked this pull request as ready for review July 5, 2021 12:09
@olexiyk
Copy link
Author

olexiyk commented Jul 5, 2021

Could you take a look again? The issue with redirect was fixed by using an updated php-http/client-common package. guzzlehttp/psr7 is now either ^1.8 or ^2.0 because the used classes are the same in both

I have seen that build was failing but could not really understand what exactly was wrong

@olexiyk olexiyk changed the title 6.0.x make php implementation depend on meta packages for http client [PHP] 6.0.x make php implementation depend on meta packages for http client Jul 5, 2021
@dkarlovi
Copy link
Contributor

dkarlovi commented Oct 1, 2021

@wing328 is the issue with Petstore tests fixed? I remember it's quite flaky.

@wing328 wing328 added this to the 6.0.0 milestone Apr 13, 2022
@wing328 wing328 modified the milestones: 6.0.0, 6.1.0, 6.0.1 May 26, 2022
@wing328 wing328 modified the milestones: 6.0.1, 6.1.0 Jul 5, 2022
@hanovruslan
Copy link

Hi! Does this issue have some forecasts for release?

@wing328 wing328 modified the milestones: 6.1.0, 6.1.1 Sep 11, 2022
@wing328 wing328 modified the milestones: 6.1.1, 6.2.1 Sep 24, 2022
@wing328 wing328 changed the base branch from 6.0.x to master October 15, 2022 08:51
@wing328 wing328 modified the milestones: 6.2.1, 6.3.0 Nov 1, 2022
@wing328 wing328 modified the milestones: 6.3.0, 6.3.1 Jan 20, 2023
@wing328 wing328 modified the milestones: 6.4.0, 6.5.0 Feb 19, 2023
@wing328 wing328 modified the milestones: 6.5.0, 6.6.0 Apr 1, 2023
@wing328 wing328 modified the milestones: 6.6.0, 7.0.0 May 11, 2023
@Argentum88
Copy link

Argentum88 commented May 12, 2023

@dkarlovi @wing328 Are there any reasons for the merge delay?

@deluxetom
Copy link

it would be great to have this merged, let us know if help is needed

@wing328
Copy link
Member

wing328 commented Aug 19, 2023

@olexiyk can you please resolve the merge conflicts when you've time?

@wing328
Copy link
Member

wing328 commented Aug 20, 2023

UPDATE: I've filed #16368 to add the implementation via the library option (e.g. --library psr-18 via CLI)

@wing328 wing328 modified the milestones: 7.0.0, 7.1.0, 7.0.1 Aug 25, 2023
@wing328 wing328 closed this Aug 25, 2023
@wing328
Copy link
Member

wing328 commented Aug 25, 2023

Included #16368 in v7.0.0 release.

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

Successfully merging this pull request may close these issues.

7 participants