Conversation
…ag implicitly nullable parameters Follow up on 1692 and 1694 :point_right: **Opinion needed.** > - Core: > . Implicitly nullable parameter types are now deprecated. > RFC: https://wiki.php.net/rfc/deprecate-implicitly-nullable-types The above mentioned deprecation also impacts this sniff, as implicitly nullable parameters were previously the one exception for the "removed optional before required parameter" deprecation. While PHP 8.4 will only flag implicitly nullable parameters with a non-nullable type with a deprecation notice about these being implicitly nullable and needing a nullable type, as soon as the nullability operator is added or the type is changed to a union type which includes `null` (and the `null` default value not removed), PHP will start throwing the deprecation notice about an optional parameter being declared before a required parameter. So, in PHP itself, one deprecation notice is effectively "hiding" behind another one. In terms of sniffs, it is considered bad practice to hide one notice behind another one as it makes the sniff output less immediately actionable. With that in mind, I'm proposing to flag implicitly nullable parameters which are declared before required parameters anyway. While this doesn't match PHP exactly, I still feel this is justified so as to provide end-users with all the information they need (both deprecations) to fix the issue properly in one go. If this would turn out to be controversial, I'd suggest lowering the severity of the issue to `3` rather than removing the change. A lower severity will hide the warning from _most_ users, but will allow discerning users to still receive it. Includes updated tests. Includes an update to the docs. Refs: * https://wiki.php.net/rfc/deprecate-implicitly-nullable-types * https://github.com/php/php-src/blob/330cc5cdb2096c5d41041eaae1f1cc0ed7e36898/UPGRADING#L271C1-L273C70 * php/php-src 12959 * php/php-src@330cc5c
From my point of view, the most important thing is that running PHPCompatibility on a particularly codebase and specifying a particular range of PHP versions the code needs to run on (implicitly or explicitly) should report any code that has potential compatibility issues with any of those PHP versions[1]. Correct me if I'm wrong, but I think you are asking "If a bit of code has issue A and issue B, is it OK only report issue B". Or maybe you are asking "is it OK to report both issues, even though they are kind of the same/overlapping". I think either approach is acceptable. If the code suffers from issues A and B I don't mind whether we report only A, only B or both A and B. However, if only A is reported and we fix A in such a way that doesn't fix B, I would expect B to be reported on a subsequent run. Alternatively, if you're asking whether it's OK to report both errors using the same sniff code, then I'd say it would be preferable to have two distinct codes, but a single code may be reasonable if the two issues are sufficiently related. I don't know if I've managed to answer your question, here - if not, I'll be happy to have another run at it... [1] (to the extent that this is possible in a static-analysis tool) |
What I'm really asking is: if the code has issue A and issue B, but PHP itself would initially only report issue A and would only report issue B once issue A has been fixed, is it okay for PHPCompatibility to report both issues from the get-go ? The issues are not the same, but they are closely related / have a partial overlap.
That is the current situation (without this PR).
As the issues are two distinct issues, they most definitely do not have the same sniff code, they are not even in the same sniff. The new PHP 8.4 deprecation is reported via the new Basically as soon as the "simple" fix for the The adjustment in this PR is that - via a separate Makes sense ? |
|
My opinion of this has changed while writing this out. Initially I was comfortable keeping one deprecation hidden behind the other. A significant contributing factor for this opinion is that there are multiple ways of fixing the implicitly-nullable deprecation, and not all of these lead to the optional-before-required deprecation. And the same reasons that #1692 landed instead of #1669 (ie, let's match PHP's behaviour). I thought that showing two warnings seemed like noise when one code change could solve both complaints. I have now worked through some examples, and thought about all the options for how to resolve the implicitly-nullable deprecation. All of the code changes that I could come up with just now to fix the implicitly-nullable deprecation either reveal the optional-before-required deprecation, or implicitly resolve it. I would expect the workflow for resolving PHP compatibility issues to include multiple runs of I've also checked the proposed code here to ensure that we only adjust the behaviour on PHP 8.4+, which is the case. |
wimg
deleted the
php-8.4/removedoptionalbeforerequired-update-for-implicitly-nullable
branch
4 participants
Follow up on #1692 and #1694
👉 Opinion needed. /cc @fredden @MarkMaldaba
The above mentioned deprecation also impacts this sniff, as implicitly nullable parameters were previously the one exception for the "removed optional before required parameter" deprecation.
While PHP 8.4 will only flag implicitly nullable parameters with a non-nullable type with a deprecation notice about these being implicitly nullable and needing a nullable type, as soon as the nullability operator is added or the type is changed to a union type which includes
null(and thenulldefault value not removed), PHP will start throwing the deprecation notice about an optional parameter being declared before a required parameter.So, in PHP itself, one deprecation notice is effectively "hiding" behind another one. In terms of sniffs, it is considered bad practice to hide one notice behind another one as it makes the sniff output less immediately actionable.
With that in mind, I'm proposing to flag implicitly nullable parameters which are declared before required parameters anyway. While this doesn't match PHP exactly, I still feel this is justified so as to provide end-users with all the information they need (both deprecations) to fix the issue properly in one go.
If this would turn out to be controversial, I'd suggest lowering the severity of the issue to
3rather than removing the change. A lower severity will hide the warning from most users, but will allow discerning users to still receive it.Includes updated tests.
Includes an update to the docs.
Refs: