Conversation
…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.
4034c11 to
aede516
Compare
wimg
approved these changes
May 9, 2021
|
@jrfnl: I already had it by just referencing the PHPCompat ruleset alone and then looked into auto-loading in PHPCS and I think it can be prevented there. I filed a PR: squizlabs/PHP_CodeSniffer#3369 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
OptionalToRequiredFunctionParameter: decouple from RequiredToOptionalFunctionParametersSniff
This is a technical refactor and contains no significant functional changes.
The
OptionalToRequiredFunctionParameterSniffclass previously extended theRequiredToOptionalFunctionParametersSniffclass, which in turn extended theAbstractComplexVersionSniff.The sniff has now been refactored to directly extend the
AbstractFunctionCallParameterSniffclass instead.The purpose of this refactor is three-fold:
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.
AbstractFunctionCallParameterSniffsniff is better than the one which was previously contained in theRequiredToOptionalFunctionParametersSniff, preventing some potential false positives as demonstrated by the additional unit tests.AbstractFunctionCallParameterSniffwhich is better still.By decoupling this sniff from the
RequiredToOptionalFunctionParametersSniffand by extension from theAbstractComplexVersionSniffsniff, we pave the way for the breaking change of removing theAbstractComplexVersionSniffclass.The switch over to the PHPCSUtils version of the
AbstractFunctionCallParameterSniffin 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
RequiredToOptionalFunctionParameters: decouple from AbstractComplexVersionSniff
This is a technical refactor and contains no significant functional changes.
The
RequiredToOptionalFunctionParametersSniffclass previously extended theAbstractComplexVersionSniff.The sniff has now been refactored to extend the
AbstractFunctionCallParameterSniffclass instead.The purpose of this refactor is two-fold:
AbstractFunctionCallParameterSniffsniff is better than the one which was previously contained in theRequiredToOptionalFunctionParametersSniff, preventing some potential false positives as demonstrated by the additional unit tests.AbstractFunctionCallParameterSniffwhich is better still.By decoupling this sniff from the
RequiredToOptionalFunctionParametersSniffand by extension from theAbstractComplexVersionSniffsniff, we pave the way for the breaking change of removing theAbstractComplexVersionSniffclass.The switch over to the PHPCSUtils version of the
AbstractFunctionCallParameterSniffin 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.