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

Added explicit nullable types to satisfy PHP 8.4 deprecation #369

Merged

Conversation

akinspe
Copy link
Contributor

@akinspe akinspe commented Dec 9, 2024

This PR fixes a variety of deprecation warnings found when running in PHP 8.4.

In particular it fixes "Implicitly nullable parameter types are now deprecated." by explicitly declaring nullable parameters.

@tvdijen
Copy link
Member

tvdijen commented Dec 9, 2024

Thanks! But I'm not sure we want to merge this. It's a change of signatures and we're already working on a complete rewrite in a new major version..

@tvdijen tvdijen requested a review from thijskh December 9, 2024 18:56
@akinspe
Copy link
Contributor Author

akinspe commented Dec 9, 2024

That's fair. Technically the signatures aren't different as PHP handles the type $param= null implicitly as a nullable type ?type $param=null . It just means that people will see MANY deprecation warnings in PHP 8.4. Given you had started to support 8.4 in a previous commit I thought you would want to fix this. Here's some more info:

https://php.watch/versions/8.4/implicitly-marking-parameter-type-nullable-deprecated

Thanks for your consideration.

@tvdijen
Copy link
Member

tvdijen commented Dec 9, 2024

Oh, since it appears to be completely backwards compatible, I have no problem merging this!
I'll await the tagged second pair of eyes to be on the safe side!

@tvdijen
Copy link
Member

tvdijen commented Dec 9, 2024

Actually, the unit tests speak for themselves.. I'll tag a new release

@tvdijen tvdijen merged commit 5dff6cf into simplesamlphp:release-4.x Dec 9, 2024
21 checks passed
tvdijen pushed a commit to simplesamlphp/saml2-legacy that referenced this pull request Dec 9, 2024
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.

2 participants