Refactor OptionalToRequiredFunctionParameterSniff and RequiredToOptionalFunctionParametersSniff by jrfnl · Pull Request #1273 · PHPCompatibility/PHPCompatibility
…FunctionParametersSniff
This is a technical refactor and contains no significant functional changes.
The `OptionalToRequiredFunctionParameterSniff` class previously extended the `RequiredToOptionalFunctionParametersSniff` class, which in turn extended the `AbstractComplexVersionSniff`.
The sniff has now been refactored to directly extend the `AbstractFunctionCallParameterSniff` class instead.
The purpose of this refactor is three-fold:
1. Multiple people have reported issues with fatal "class already declared" errors for this sniff extending another sniff.
While this is primarily an issue with the autoloader in PHPCS, the issue can be avoided by not allowing one sniff to directly extend another.
Note: the same problem does not exist when extending an _abstract_ sniff, so this solution will be fine.
2. The (global) function call determination in the `AbstractFunctionCallParameterSniff` sniff is better than the one which was previously contained in the `RequiredToOptionalFunctionParametersSniff`, preventing some potential false positives as demonstrated by the additional unit tests.
3. In the (near) future, PHPCSUtils is expected to contain a version of the `AbstractFunctionCallParameterSniff` which is better still.
By decoupling this sniff from the `RequiredToOptionalFunctionParametersSniff` and by extension from the `AbstractComplexVersionSniff` sniff, we pave the way for the breaking change of removing the `AbstractComplexVersionSniff` class.
The switch over to the PHPCSUtils version of the `AbstractFunctionCallParameterSniff` in itself is not a breaking change and can therefore safely be done in a future minor release.
Note: Aside from the tests still passing, I have also verified that the sniff error codes are not affected by this change, so from a "custom ruleset"/"ignore annotations" point of view this refactor does not contain any breaking changes.
Fixes 608
Fixes 638
Fixes 793
Fixes 1042
…rsionSniff
This is a technical refactor and contains no significant functional changes.
The `RequiredToOptionalFunctionParametersSniff` class previously extended the `AbstractComplexVersionSniff`.
The sniff has now been refactored to extend the `AbstractFunctionCallParameterSniff` class instead.
The purpose of this refactor is two-fold:
1. The (global) function call determination in the `AbstractFunctionCallParameterSniff` sniff is better than the one which was previously contained in the `RequiredToOptionalFunctionParametersSniff`, preventing some potential false positives as demonstrated by the additional unit tests.
2. In the (near) future, PHPCSUtils is expected to contain a version of the `AbstractFunctionCallParameterSniff` which is better still.
By decoupling this sniff from the `RequiredToOptionalFunctionParametersSniff` and by extension from the `AbstractComplexVersionSniff` sniff, we pave the way for the breaking change of removing the `AbstractComplexVersionSniff` class.
The switch over to the PHPCSUtils version of the `AbstractFunctionCallParameterSniff` in itself is not a breaking change and can therefore safely be done in a future minor release.
Note: Aside from the tests still passing, I have also verified that the sniff error codes are not affected by this change, so from a "custom ruleset"/"ignore annotations" point of view this refactor does not contain any breaking changes.
wimg approved these changes May 9, 2021
wimg
deleted the
feature/608-638-793-1042-decouple-required-to-optional-param-sniffs
branch
jrfnl
mentioned this pull request
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters