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

wimg approved these changes May 9, 2021

@wimg wimg deleted the feature/608-638-793-1042-decouple-required-to-optional-param-sniffs branch

May 9, 2021 15:59

@jrfnl jrfnl mentioned this pull request

Dec 26, 2022