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] Allow guzzlehttp/psr7 ^1.7 or ^2.0 #11240

Merged
merged 1 commit into from
Jan 7, 2022
Merged

[PHP] Allow guzzlehttp/psr7 ^1.7 or ^2.0 #11240

merged 1 commit into from
Jan 7, 2022

Conversation

AndreasA
Copy link
Contributor

@AndreasA AndreasA commented Jan 6, 2022

Fix #11152

Allows the use of guzzlehttp/psr7 in versions 1.8.x and v2.0.x

This ensures that the created package can be used in combination with libraries that still require 1.x of guzzlehttp/psr7 and as all the necessary functions of 2.x already exists in 1.8.x, it requires only a composer.json change.

Furthermore, it is also what is required by guzzlehttp itself in 7.3.0 or higher itself: https://github.com/guzzle/guzzle/blob/7.3.0/composer.json

PR checklist

  • Read the contribution guidelines.
  • 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.
  • 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.
  • File the PR against the correct branch: master (5.3.0), 6.0.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, @dkarlovi, @mandrean, @jfastnacht, @ackintosh, @ybelenko, @renepardon

@ybelenko
Copy link
Contributor

ybelenko commented Jan 6, 2022

@AndreasA Thanks for pr.

Is this really so important? That dependency definition ^1.7 || ^2.1 can break our CI builds and it will be hard to trace because you cannot say which version will be installed on our server without checking docs of all related packages including php itself. It means that after this change we have to test and maintain at least 2 different builds(1.7, 2.1 psr implementation).

I would keep the old constraint(^2.1) to avoid extra bug tickets. User can overwrite that composer.mustache in five seconds when needed.

@AndreasA
Copy link
Contributor Author

AndreasA commented Jan 6, 2022

@ybelenko
Well, I created it more because I currently need it with a project.
Of course, I can also change the composer.mustache and it will probably be what I do anyway as I need it now, but in case there is an issue with the code and 1.8.x that alone wouldn't be enough.
In that project we use PHP 8.1.x (so I cannot just use an older openapi generator version either) and a library that in its latest version currently only allows 1.8.x of guzzlehttp/psr7 but it will probably switch to 2.x with the next major.

Regarding breaking builds, it shouldn't as composer always installs the latest possible version. It can only happen if you add another package that requires 1.8.x (or because of a potential composer.lock file) which however means that a build that only allows 2.1.x will fail anyway as the dependency cannot be resolved.

But if you think it is too much effort or something like that, I undersand if you do not want to change it because as you mentioned it is possible to override it.

However, guzzle http client itself uses the 1.8.x or 2.1.x in its latest version still, so I think it would kind of make sense to keep in sync with the client requirements: https://packagist.org/packages/guzzlehttp/guzzle#7.4.1

But I can understand that it might be too much effort.

@ybelenko
Copy link
Contributor

ybelenko commented Jan 6, 2022

@AndreasA let's wait for a @wing328 decision.

@wing328 wing328 added this to the 5.4.0 milestone Jan 7, 2022
@wing328 wing328 changed the title Issue 11152: Allow guzzlehttp/psr7 ^1.7 or ^2.0 [PHP] Allow guzzlehttp/psr7 ^1.7 or ^2.0 Jan 7, 2022
@wing328
Copy link
Member

wing328 commented Jan 7, 2022

I'm ok with the change as AWS PHP SDK is doing exactly that: https://github.com/aws/aws-sdk-php/blob/master/composer.json#L21

@wing328 wing328 merged commit ef882a4 into OpenAPITools:master Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants