Shoot. I actually already wrote a sniff for this two days ago, but hadn't pulled it yet as I'm waiting for 3v4l to update their PHP git.master, so I can validate the tests I created and verify the sniff matches PHP exactly...

Okay, guess we'll have to see about merging the two sniffs. Will have to have a look at this next week.

As a side-note, I will most definitely not accept an auto-fixer for this issue as there are multiple fixes possible and the sniff can not decide which is the most appropriate fix.

As a side-note, I will most definitely not accept an auto-fixer for this issue as there are multiple fixes possible and the sniff can not decide which is the most appropriate fix.

Are you referring to ?string versus string|null, or something else? In my opinion, that particular difference should be covered by another sniff in the ruleset if people are particular about which of those two possible forms should be used in a codebase.

I actually already wrote a sniff for this two days ago, but hadn't pulled it yet

I win. 😄

we'll have to see about merging the two sniffs

It's probably worth at least comparing the two sets of tests. Is your version publicly available, or only accessible to you?

As a side-note, I will most definitely not accept an auto-fixer for this issue as there are multiple fixes possible and the sniff can not decide which is the most appropriate fix.

Are you referring to ?string versus string|null, or something else? In my opinion, that particular difference should be covered by another sniff in the ruleset if people are particular about which of those two possible forms should be used in a codebase.

No, I'm not.

While, in the fast majority of cases, adding the nullability operator will be the "simple" fix, there are other options and only someone who is familiar with how the project under scan is used (open/closed source, code is being extended/methods overloaded or not) can take the decision what the correct fix is and what fix is chosen may also depend on whether the next version of the project is a major or not, as some of the fixes constitute breaking changes to one degree or another.

Aside from adding the nullability operator, these are various other possible choices to bypass/fix the deprecation:

• Remove the default value and make the parameter required (which it often already is in practice anyway). This would make it non-nullable, but the null still being there is half the time an historic oversight anyway.
• Remove the type declaration and do in-function parameter validation (for a parameter which needs to stay optional, but can't get the nullable operator).
• Deprecate the function and introduce a replacement with a different signature (possibly with the parameters in a different order, but there are other options possible)
• Take this deprecation as the final straw to get dependency injection set up for the code base.
• Change the default value from null to new MyClass if the object doesn't need variable parameters (possible since PHP 8.1, new in initializers).

Also keep in mind the nullability operator can only be used when the minimum supported PHP version is PHP 7.1 or higher. The fixer you wrote does not take the minimum PHP version of the code under scan into account, which means the fixer will introduce parse errors into projects (which have a minimum < 7.1).

I actually already wrote a sniff for this two days ago, but hadn't pulled it yet

I win. 😄

Eh... you know that's not how this works ?

we'll have to see about merging the two sniffs

It's probably worth at least comparing the two sets of tests. Is your version publicly available, or only accessible to you?

It's not public yet (as I wanted to validate against 3v4l first). I'll have a look see if they have updated the PHP version in the mean time, but I can push the branch for you to see anyway.

It's probably worth at least comparing the two sets of tests.

I just compared the tests. My sniff correctly handles your test file.
Your sniff throws two false positives and has two false negatives for my test file.

I win 😝

(that is, presuming of course, that our tests actually match the reality of PHP, which I still haven't been able to validate)

You can find my branch here: https://github.com/PHPCompatibility/PHPCompatibility/tree/php-8.4/new-removedimplicitlynullableparam-sniff

Based on the feedback in PHPCSStandards/PHPCSUtils#566 and PHPCSStandards/PHPCSUtils#567, it seems that we can't trust \PHPCSUtils\Utils\FunctionDeclarations::getParameters() until PHPCSStandards/PHP_CodeSniffer#387 has been resolved. Therefore this sniff needs to be rewritten to do its own parsing of method parameters (which sounds like a lot of unnecessary work, given the aforementioned method exists and will need to be fixed anyway), or we put this on hold until PHPCSStandards/PHP_CodeSniffer#387 gets sorted out. However, we only really need getParameters() to work properly if we want to run the auto-fixer. (The remaining bugs can be worked-around in the sniff code - for example see what @jrfnl has done in bab278d#diff-b7e990083026c116bca252bc68a81bcb367af3245c094d8a9c3f19a6f75c113cR89 and https://github.com/PHPCompatibility/PHPCompatibility/pull/1669/files#diff-86cd1c9d98cd9e8f0c57c7c576d36d0e2da1dceb77bfd7433663ade2cdd84d92R114.)

Also, as @jrfnl has already done the work in duplicate of writing a sniff for this PHP deprecation, it seems that /that/ sniff is more likely to be accepted in this project than /this/ sniff. With all this in mind, I think we should close this pull request in favour of the will-be-created pull request from @jrfnl for the same.

Based on the feedback in PHPCSStandards/PHPCSUtils#566 and PHPCSStandards/PHPCSUtils#567, it seems that we can't trust \PHPCSUtils\Utils\FunctionDeclarations::getParameters() until PHPCSStandards/PHP_CodeSniffer#387 has been resolved. Therefore this sniff needs to be rewritten to do its own parsing of method parameters (which sounds like a lot of unnecessary work, given the aforementioned method exists and will need to be fixed anyway), or we put this on hold until PHPCSStandards/PHP_CodeSniffer#387 gets sorted out. However, we only really need getParameters() to work properly if we want to run the auto-fixer.

We can trust getParameters(), it just work differently than you want it too, but that's by design and not something which needs to be, nor will be "fixed".

There will be other helpers available in Utils in the future for analyzing types and I'd be happy to ping you for feedback on the ones I have in mind, either before they are public, or once I've pulled the PR (but definitely before they are released).

Also, as @jrfnl has already done the work in duplicate of writing a sniff for this PHP deprecation, it seems that /that/ sniff is more likely to be accepted in this project than /this/ sniff. With all this in mind, I think we should close this pull request in favour of the will-be-created pull request from @jrfnl for the same.

Sorry, that's not how it works either. I recognize the work you put into this and (aside from a fixer not being acceptable) there are small other differences between our sniffs. In my mind, the next step is resolving those differences and updating one or the other of the sniffs to be the best of both worlds and making sure we are both recognized as co-authors of the sniff.

I have had a (private) response from 3v4l by now, so I expect that the tooling will be updated soonish to validate the tests. Let's discuss merging the other differences once the tests are validated. Is that okay with you ?

As a side-note and to (hopefully) avoid more duplicate work: the RemovedOptionalBeforeRequiredParam sniff also needs an update for this same RFC. I've already prepared that one too, so I'll be closing #1669 and replacing it with a new PR once I've been able to validate the tests.

The lesson for me from this ticket is that, if there are going to be more people actively contributing code to this project, we need to find a way to communicate work started/in progress better.

For RFC based sniffs, I normally open an "account for PHP x.x changes" ticket around the time the RFC window closes and annotate in that ticket what is being worked on/claimed by someone to prevent duplicate work. That time is still a few months away though and as this is such a high impact RFC, this one just felt like something to address earlier. Maybe either of us should have opened an issue before starting work on the sniff ? (to prevent the duplication)

What are your thoughts on this ?

We can trust getParameters(), it just work differently than you want it too, but that's by design and not something which needs to be, nor will be "fixed".

I expected the type_hint entry in the returned array to contain the type hint, but it does not do this reliably for DNF types.
I expected the type_hint_end_token entry in the returned array to contain a reference to the last token in the type hint, but it does not do this reliably for DNF types.
Both of these are being fixed over in PHPCSStandards/PHPCSUtils#567.

Let's pick this up on our call next week and we can write an update here following that with any noteworthy discussion points and decisions/actions.

@fredden The fact that PHPCS (and by extension PHPCSUtils) doesn't support DNF yet is a known fact and IMO irrelevant for this sniff. This is being discussed in PHPCSStandards/PHP_CodeSniffer#387, which is where that discussion belongs.

FYI: 3v4l has been updated, so I'll be verifying the tests later today.

Based on the tests I've been running so far:

• Properties in Constructor Property Promotion with a non-nullable type and a null default value are outside the scope of the deprecation as this was never allowed (i.e. always a fatal).
• Parameters with a default value using a constant expression resulting in null are currently not flagged by the deprecation. This may or may not be an oversight. I've opened an issue in the PHP repo about this.

@fredden 👆🏻 This is exactly the kind of thing why I waited to pull my version of the sniff and wanted to validate the test cases first.
I've seen these kind of runtime versus RFC spec/presumption differences too often by now and the sniffs in PHPCompatibility have to be correct, i.e. mirror the PHP behaviour and should not be based on presumption.

@fredden I've updated my version of the sniff based on the above findings and have added a few extra test cases based on the tests in your sniff.
I couldn't see anything else in your sniff which was missing from mine, aside from the PHP 9.0 removal notice, which is the kind of thing we generally only add once the commit for the removal has gone into the PHP source code. (I admit, there may be some stray removal notices in sniffs, in which case, those should be removed)

I've made the changes largely in new commits for the time being, so you can see what I've changed. Please let me know if you see anything else which should be included from your sniff.

If not, I'll squash the commits (and keep the joined credits) and I propose I pull that branch to replace this PR.

This pull request is on-hold while we work on #1694. It's likely that we'll merge #1694 and close this.