Skip to content

Test on PHP 8.1 #135

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 2 commits into from
Closed

Test on PHP 8.1 #135

wants to merge 2 commits into from

Conversation

simPod
Copy link
Contributor

@simPod simPod commented Dec 2, 2021

Replaces #143 that should be merged to php-openapi version that does not support old PHP versions.

@simPod
Copy link
Contributor Author

simPod commented Feb 9, 2022

@cebe I've resolved conflict

@@ -183,6 +183,7 @@ public function getErrors(): array
* @return boolean true on success or false on failure.
* The return value will be casted to boolean if non-boolean was returned.
*/
#[\ReturnTypeWillChange]
Copy link
Owner

Choose a reason for hiding this comment

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

given that this library is not compatible with PHP 5, as far as I see it would be better to make return types compatible instead of adding the annotation which only hides the deprecation warning but does not actually fix the problem.

see https://php.watch/versions/8.1/internal-method-return-types

Copy link
Owner

Choose a reason for hiding this comment

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

just seen that this is what #143 does, will check that in more detail later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intended this as a middle step in order to keep BC.

After this there's #143 to be merged

@vitxd
Copy link

vitxd commented Apr 8, 2022

Is there any update about this PR. I would really need it 😅

@gniewomir-mohawk
Copy link

Hi, I want to avoid forking, thought about contributing, but I see fix is already here. Can I do anything to speed up 8.1 compatibility being merged?

@wlodekj
Copy link

wlodekj commented Apr 11, 2022

Upvote for this one to be merged sooner than later. Thanks!

@conserj
Copy link

conserj commented Apr 11, 2022

waiting for this one to be merged too

@cebe cebe mentioned this pull request Apr 20, 2022
@cebe
Copy link
Owner

cebe commented Apr 20, 2022

I merged a different solution in #162, thanks for your contribution anyway!

@cebe cebe closed this Apr 20, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants