Add new sniff for PHP 8.4 deprecation - implicit nullable parameter types by fredden · Pull Request #1689 · PHPCompatibility/PHPCompatibility

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 ?