swagger-codegen icon indicating copy to clipboard operation
swagger-codegen copied to clipboard

Make generic model compatible with PHP 8.1

Open BafS opened this issue 3 years ago • 2 comments

PR checklist

  • [x] Read the contribution guidelines.
  • [x] Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • [x] Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • [x] Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Add #[\ReturnTypeWillChange] (https://php.watch/versions/8.1/ReturnTypeWillChange) to avoid deprecation notices.

Fixes https://github.com/swagger-api/swagger-codegen/issues/11820

cc. @dkarlovi and @mandrean

BafS avatar Aug 03 '22 06:08 BafS

Hello, is there any way we can have it merged? It seems to be a valid workaround.

EgorBurykin avatar Aug 15 '22 11:08 EgorBurykin

This would certainly help us. The PR seems valid, could this be merged into the codebase please?

mbouwer avatar Sep 02 '22 14:09 mbouwer

@dkarlovi and @mandrean, would it be possible to merge this PR?

BafS avatar Oct 03 '22 03:10 BafS

or @gracekarina maybe

BafS avatar Oct 03 '22 03:10 BafS

Any news on this one, many projects are waiting to support PHP 8.1 as without this, it triggers deprecations.

deguif avatar Oct 03 '22 10:10 deguif

@HugoMario maybe you could help me to merge the PR? The PHP maintainers do not seems to reply. Thanks in advance!

BafS avatar Oct 03 '22 10:10 BafS

ping @frantuma maybe

deguif avatar Oct 04 '22 08:10 deguif

@char0n @gracekarina @ponelat @frantuma could anyone help us to merge this PR?

BafS avatar Oct 05 '22 11:10 BafS

@BafS in the meantime you can rebase the branch so that This branch is out-of-date with the base branch message disappears.

Let's hope that a core contributor will be able to have a look and merge this one soon as it's a real pain currently for PHP >= 8.1 and will become more and more annoying as PHP 8.2 is near to be released.

deguif avatar Oct 07 '22 08:10 deguif

Thanks for your patience folks, from my limited knowledge of PHP, ReturnTypeWillChange is backwards compatible with older versions of PHP? If so, I think it should be good to merge. @HugoMario if you're happy, I think lets get this merged. And yes please, could you rebase @BafS

ponelat avatar Oct 10 '22 12:10 ponelat

@ponelat thanks for your feedback. You're right, #[\ReturnTypeWillChange] is backward compatible as attributes are not interpreted on PHP versions lower than 8.x.

deguif avatar Oct 11 '22 15:10 deguif

@ponelat Thanks for your answer! PR synced (not sure why this is necessary as there is no conflicts), and yes as @deguif says, it is fully backward compatible.

BafS avatar Oct 12 '22 04:10 BafS

Thanks!

frantuma avatar Oct 13 '22 10:10 frantuma

Thank you! Do you know when the next release will be tagged?

BafS avatar Oct 14 '22 06:10 BafS

@frantuma any chance to create a new tag to have the release available to the world?

BafS avatar Nov 02 '22 15:11 BafS