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

Resolve PHPStan issue with deprecated parameter order in PHP 8.1 #2963

Merged
merged 3 commits into from Mar 16, 2024

Conversation

sayuprc
Copy link
Contributor

@sayuprc sayuprc commented Mar 9, 2024

Since PHP 8.1, a nullable, optional parameter with a default value of null followed by a required parameter will generate a warning.
The current PHPStan did not recognize such code as deprecated.
This PR updates PHPStan to properly handle such cases.
Please review.
Thank you.

resolve phpstan/phpstan#6668

@phpstan-bot
Copy link
Collaborator

You've opened the pull request against the latest branch 1.11.x. If your code is relevant on 1.10.x and you want it to be released sooner, please rebase your pull request and change its target to 1.10.x.

@sayuprc sayuprc changed the base branch from 1.11.x to 1.10.x March 10, 2024 02:23
Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

I'm worried this is a lot more complex topic. For example: Why is this reported only on PHP 8.3+? https://3v4l.org/e6n6Q

Also - your message says "Deprecated in 8.0" but in code your condition says >= 80100.

@sayuprc
Copy link
Contributor Author

sayuprc commented Mar 13, 2024

Thank you for the review.

I was aware that this code was generating deprecated warnings from PHP 8.1. https://3v4l.org/0VAUk
It seems that there are differences in behavior with patterns like int|null, so I will investigate it again.

@sayuprc
Copy link
Contributor Author

sayuprc commented Mar 16, 2024

@ondrejmirtes
Since PHP 8.3, the deprecated conditions have changed, and warnings for union types and mixed types are also issued.

For this update, I am considering issuing warnings for nullable types deprecated since PHP 8.1 and for union types and mixed types deprecated since PHP 8.3.

I would like the message format to only replace the version number.
For example, in the case of mixed $a = null, it would display "Deprecated in PHP 8.3".

@ondrejmirtes
Copy link
Member

@sayuprc Yes, that would be okay 👍

@ondrejmirtes ondrejmirtes merged commit 79aed74 into phpstan:1.10.x Mar 16, 2024
440 checks passed
@ondrejmirtes
Copy link
Member

Awesome, thank you!

@ondrejmirtes
Copy link
Member

I just noticed this doesn't make any tests fail - is that right? 6512a84

@sayuprc
Copy link
Contributor Author

sayuprc commented Mar 16, 2024

@ondrejmirtes
If you remove $targetPhpVersion = null;, you won't be able to display the correct PHP version in the warning message for code like fn (int|null $a = null, int $b, int $c = 0, $d): int => 1;.

@ondrejmirtes
Copy link
Member

@sayuprc Please send a fix with a test as a new PR. Thank you.

@sayuprc sayuprc deleted the resolve-6668 branch March 16, 2024 18:27
@sayuprc
Copy link
Contributor Author

sayuprc commented Mar 19, 2024

@ondrejmirtes
I have created a new pull request. #2971

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