New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

QA: standards should not have conflicting rules / fix fixer conflicts #152

Open

jrfnl opened this issue Dec 10, 2023 · 4 comments

Labels

Focus: Fixer Conflicts Status: help wanted Status: needs investigation Type: chores/QA

Member

jrfnl commented Dec 10, 2023 •

edited

Loading

Premise

A standard should be able to be used without leading to fixer conflicts.

Now, as code can be written in lots of different ways, it is not always straight forward to anticipate potential conflicts, however, PHPCS contains a wealth of test code, so by running the fixers for a standard over all test case files, we should be able to detect a lot of potential fixer conflicts.

Current status

While I have the workflow and the set up for this check ready, it cannot currently be turned on as all standards, except PSR1, lead to fixer conflicts.

Note: as CSS/JS support will be dropped in v 4.0, I've not included the CSS/JS test case files in the set up for the fixer conflict check.

Also note that Generic is not a standard but a sniff collection containing conflicting rules by design. For that reason, the fixer conflict check is not relevant.

Last update date for the "current status": 2024-07-29
Please feel free to ping @jrfnl for an update if the list is outdated.

Details of current fixer conflicts for the PEAR standard

Command:

phpcbf -p . --standard=PEAR --ignore=*/build/*,*/vendor/* --basepath=. --extensions=inc --suffix=.conflictcheck

Test case files leading to fixer conflicts

PHPCBF RESULT SUMMARY
------------------------------------------------------------------------------------------------------------------
FILE                                                                                              FIXED  REMAINING
------------------------------------------------------------------------------------------------------------------
src\Standards\Generic\Tests\ControlStructures\DisallowYodaConditionsUnitTest.inc                  FAILED TO FIX
src\Standards\Generic\Tests\WhiteSpace\ScopeIndentUnitTest.1.inc                                  FAILED TO FIX
src\Standards\Generic\Tests\WhiteSpace\ScopeIndentUnitTest.2.inc                                  FAILED TO FIX
src\Standards\PEAR\Tests\NamingConventions\ValidVariableNameUnitTest.inc                          FAILED TO FIX
src\Standards\PEAR\Tests\WhiteSpace\ScopeClosingBraceUnitTest.inc                                 FAILED TO FIX
src\Standards\PSR12\Tests\Functions\ReturnTypeDeclarationUnitTest.inc                             FAILED TO FIX
src\Standards\PSR2\Tests\ControlStructures\SwitchDeclarationUnitTest.inc                          FAILED TO FIX
src\Standards\Squiz\Tests\Commenting\LongConditionClosingCommentUnitTest.inc                      FAILED TO FIX
src\Standards\Squiz\Tests\PHP\NonExecutableCodeUnitTest.1.inc                                     FAILED TO FIX
src\Standards\Squiz\Tests\WhiteSpace\ScopeClosingBraceUnitTest.inc                                FAILED TO FIX
tests\Core\File\FindEndOfStatementTest.inc                                                        FAILED TO FIX
tests\Core\File\FindStartOfStatementTest.inc                                                      FAILED TO FIX
tests\Core\File\GetMethodPropertiesTest.inc                                                       FAILED TO FIX **NEW 2024-03
tests\Core\Tokenizer\PHP\DefaultKeywordTest.inc                                                   FAILED TO FIX
tests\Core\Tokenizer\PHP\DNFTypesParseError1Test.inc                                              FAILED TO FIX **NEW 2024-07
tests\Core\Tokenizer\PHP\EnumCaseTest.inc                                                         FAILED TO FIX
tests\Core\Tokenizer\Tokenizer\RecurseScopeMapCaseKeywordConditionsTest.inc                       FAILED TO FIX **NEW 2024-07
tests\Core\Tokenizer\Tokenizer\RecurseScopeMapDefaultKeywordConditionsTest.inc                    FAILED TO FIX **NEW 2024-07
------------------------------------------------------------------------------------------------------------------
A TOTAL OF 9881 ERRORS WERE FIXED IN 315 FILES
------------------------------------------------------------------------------------------------------------------
PHPCBF FAILED TO FIX 18 FILES
------------------------------------------------------------------------------------------------------------------

Details of current fixer conflicts for the PSR2 standard

Command:

phpcbf -p . --standard=PSR2 --ignore=*/build/*,*/vendor/* --basepath=. --extensions=inc --suffix=.conflictcheck

Test case files leading to fixer conflicts

PHPCBF RESULT SUMMARY
------------------------------------------------------------------------------------------------------------------
FILE                                                                                              FIXED  REMAINING
------------------------------------------------------------------------------------------------------------------
src\Standards\Generic\Tests\CodeAnalysis\JumbledIncrementerUnitTest.4.inc                         FAILED TO FIX **NEW 2024-07
src\Standards\PEAR\Tests\Functions\FunctionCallSignatureUnitTest.inc                              FAILED TO FIX
src\Standards\PSR12\Tests\Functions\ReturnTypeDeclarationUnitTest.inc                             FAILED TO FIX
src\Standards\Squiz\Tests\Classes\SelfMemberReferenceUnitTest.inc                                 FAILED TO FIX **NEW 2024-07
src\Standards\Squiz\Tests\ControlStructures\ForEachLoopDeclarationUnitTest.inc                    FAILED TO FIX
tests\Core\File\GetMethodPropertiesTest.inc                                                       FAILED TO FIX **NEW 2024-03
tests\Core\Tokenizer\PHP\DNFTypesParseError1Test.inc                                              FAILED TO FIX **NEW 2024-07
------------------------------------------------------------------------------------------------------------------
A TOTAL OF 9637 ERRORS WERE FIXED IN 366 FILES
------------------------------------------------------------------------------------------------------------------
PHPCBF FAILED TO FIX 7 FILES
------------------------------------------------------------------------------------------------------------------

Details of current fixer conflicts for the PSR12 standard

Command:

phpcbf -p . --standard=PSR12 --ignore=*/build/*,*/vendor/* --basepath=. --extensions=inc --suffix=.conflictcheck

Test case files leading to fixer conflicts

PHPCBF RESULT SUMMARY
-----------------------------------------------------------------------------------------------------------
FILE                                                                                       FIXED  REMAINING
-----------------------------------------------------------------------------------------------------------
src\Standards\Generic\Tests\PHP\RequireStrictTypesUnitTest.13.inc                          FAILED TO FIX
src\Standards\Generic\Tests\PHP\RequireStrictTypesUnitTest.5.inc                           FAILED TO FIX
src\Standards\PEAR\Tests\Functions\FunctionCallSignatureUnitTest.inc                       FAILED TO FIX
src\Standards\PSR12\Tests\Functions\ReturnTypeDeclarationUnitTest.inc                      FAILED TO FIX
src\Standards\Squiz\Tests\ControlStructures\ForEachLoopDeclarationUnitTest.inc             FAILED TO FIX
src\Standards\Squiz\Tests\ControlStructures\ForLoopDeclarationUnitTest.inc                 FAILED TO FIX
tests\Core\File\GetMethodParametersTest.inc                                                FAILED TO FIX
tests\Core\Tokenizer\ScopeSettingWithNamespaceOperatorTest.inc                             FAILED TO FIX
tests\Core\Tokenizer\UndoNamespacedNameSingleTokenTest.inc                                 FAILED TO FIX
-----------------------------------------------------------------------------------------------------------
A TOTAL OF 10594 ERRORS WERE FIXED IN 372 FILES
-----------------------------------------------------------------------------------------------------------
PHPCBF FAILED TO FIX 9 FILES
-----------------------------------------------------------------------------------------------------------

Details of current fixer conflicts for the Squiz standard

Command:

phpcbf -p . --standard=Squiz --ignore=*/build/*,*/vendor/* --basepath=. --extensions=inc --suffix=.conflictcheck

Test case files leading to fixer conflicts

PHPCBF RESULT SUMMARY
------------------------------------------------------------------------------------------------------------------
FILE                                                                                              FIXED  REMAINING
------------------------------------------------------------------------------------------------------------------
src\Standards\Generic\Tests\Arrays\ArrayIndentUnitTest.inc                                        FAILED TO FIX
src\Standards\Generic\Tests\Classes\DuplicateClassNameUnitTest.3.inc                              FAILED TO FIX
src\Standards\Generic\Tests\CodeAnalysis\EmptyPHPStatementUnitTest.inc                            FAILED TO FIX
src\Standards\Generic\Tests\CodeAnalysis\JumbledIncrementerUnitTest.4.inc                         FAILED TO FIX **NEW 2024-07
src\Standards\Generic\Tests\CodeAnalysis\UselessOverridingMethodUnitTest.1.inc                    FAILED TO FIX **NEW 2024-03
src\Standards\Generic\Tests\ControlStructures\DisallowYodaConditionsUnitTest.inc                  FAILED TO FIX
src\Standards\Generic\Tests\ControlStructures\InlineControlStructureUnitTest.1.inc                FAILED TO FIX **NEW 2024-07
src\Standards\Generic\Tests\Functions\FunctionCallArgumentSpacingUnitTest.1.inc                   FAILED TO FIX **NEW 2024-07
src\Standards\Generic\Tests\NamingConventions\AbstractClassNamePrefixUnitTest.inc                 FAILED TO FIX
src\Standards\Generic\Tests\PHP\CharacterBeforePHPOpeningTagUnitTest.1.inc                        FAILED TO FIX
src\Standards\Generic\Tests\WhiteSpace\ScopeIndentUnitTest.1.inc                                  FAILED TO FIX
src\Standards\Generic\Tests\WhiteSpace\ScopeIndentUnitTest.2.inc                                  FAILED TO FIX
src\Standards\PEAR\Tests\Classes\ClassDeclarationUnitTest.1.inc                                   FAILED TO FIX
src\Standards\PEAR\Tests\Classes\ClassDeclarationUnitTest.2.inc                                   FAILED TO FIX
src\Standards\PEAR\Tests\Commenting\ClassCommentUnitTest.inc                                      FAILED TO FIX
src\Standards\PEAR\Tests\Commenting\FunctionCommentUnitTest.inc                                   FAILED TO FIX
src\Standards\PEAR\Tests\ControlStructures\MultiLineConditionUnitTest.inc                         FAILED TO FIX
src\Standards\PEAR\Tests\Functions\FunctionCallSignatureUnitTest.inc                              FAILED TO FIX
src\Standards\PEAR\Tests\Functions\FunctionDeclarationUnitTest.1.inc                              FAILED TO FIX
src\Standards\PEAR\Tests\NamingConventions\ValidVariableNameUnitTest.inc                          FAILED TO FIX
src\Standards\PEAR\Tests\WhiteSpace\ScopeClosingBraceUnitTest.inc                                 FAILED TO FIX **NEW 2024-03
src\Standards\PSR1\Tests\Files\SideEffectsUnitTest.1.inc                                          FAILED TO FIX
src\Standards\PSR12\Tests\ControlStructures\BooleanOperatorPlacementUnitTest.inc                  FAILED TO FIX
src\Standards\PSR12\Tests\Functions\ReturnTypeDeclarationUnitTest.inc                             FAILED TO FIX
src\Standards\PSR12\Tests\Operators\OperatorSpacingUnitTest.1.inc                                 FAILED TO FIX
src\Standards\PSR2\Tests\Classes\ClassDeclarationUnitTest.inc                                     FAILED TO FIX
src\Standards\PSR2\Tests\ControlStructures\ControlStructureSpacingUnitTest.inc                    FAILED TO FIX
src\Standards\PSR2\Tests\Methods\FunctionCallSignatureUnitTest.inc                                FAILED TO FIX
src\Standards\Squiz\Tests\Arrays\ArrayDeclarationUnitTest.1.inc                                   FAILED TO FIX
src\Standards\Squiz\Tests\Arrays\ArrayDeclarationUnitTest.2.inc                                   FAILED TO FIX
src\Standards\Squiz\Tests\Classes\SelfMemberReferenceUnitTest.inc                                 FAILED TO FIX
src\Standards\Squiz\Tests\Commenting\ClassCommentUnitTest.inc                                     FAILED TO FIX
src\Standards\Squiz\Tests\Commenting\FunctionCommentThrowTagUnitTest.inc                          FAILED TO FIX
src\Standards\Squiz\Tests\Commenting\FunctionCommentUnitTest.inc                                  FAILED TO FIX
src\Standards\Squiz\Tests\Commenting\VariableCommentUnitTest.inc                                  FAILED TO FIX
src\Standards\Squiz\Tests\ControlStructures\ControlSignatureUnitTest.1.inc                        FAILED TO FIX
src\Standards\Squiz\Tests\ControlStructures\ForEachLoopDeclarationUnitTest.inc                    FAILED TO FIX
src\Standards\Squiz\Tests\ControlStructures\ForLoopDeclarationUnitTest.1.inc                      FAILED TO FIX
src\Standards\Squiz\Tests\Objects\ObjectInstantiationUnitTest.inc                                 FAILED TO FIX **NEW 2024-03
src\Standards\Squiz\Tests\PHP\EmbeddedPhpUnitTest.1.inc                                           FAILED TO FIX
src\Standards\Squiz\Tests\PHP\EmbeddedPhpUnitTest.19.inc                                          FAILED TO FIX **NEW 2024-03
src\Standards\Squiz\Tests\PHP\EmbeddedPhpUnitTest.5.inc                                           FAILED TO FIX **NEW 2024-03
src\Standards\Squiz\Tests\PHP\InnerFunctionsUnitTest.inc                                          FAILED TO FIX
src\Standards\Squiz\Tests\Scope\MemberVarScopeUnitTest.inc                                        FAILED TO FIX
src\Standards\Squiz\Tests\Scope\MethodScopeUnitTest.inc                                           FAILED TO FIX
src\Standards\Squiz\Tests\WhiteSpace\ControlStructureSpacingUnitTest.inc                          FAILED TO FIX
src\Standards\Squiz\Tests\WhiteSpace\FunctionSpacingUnitTest.1.inc                                FAILED TO FIX
src\Standards\Squiz\Tests\WhiteSpace\ScopeClosingBraceUnitTest.inc                                FAILED TO FIX
src\Standards\Squiz\Tests\WhiteSpace\ScopeKeywordSpacingUnitTest.1.inc                            FAILED TO FIX
src\Standards\Squiz\Tests\WhiteSpace\ScopeKeywordSpacingUnitTest.3.inc                            FAILED TO FIX
tests\Core\File\FindExtendedClassNameTest.inc                                                     FAILED TO FIX **NEW 2024-03
tests\Core\File\FindStartOfStatementTest.inc                                                      FAILED TO FIX
tests\Core\File\GetConditionTest.inc                                                              FAILED TO FIX **NEW 2024-03
tests\Core\File\GetDeclarationNameTest.inc                                                        FAILED TO FIX **NEW 2024-03
tests\Core\File\GetMemberPropertiesTest.inc                                                       FAILED TO FIX
tests\Core\File\GetMethodParametersTest.inc                                                       FAILED TO FIX
tests\Core\File\GetMethodPropertiesTest.inc                                                       FAILED TO FIX **NEW 2024-03
tests\Core\Tokenizer\PHP\AttributesTest.inc                                                       FAILED TO FIX
tests\Core\Tokenizer\PHP\BackfillMatchTokenTest.inc                                               FAILED TO FIX
tests\Core\Tokenizer\PHP\BackfillReadonlyTest.inc                                                 FAILED TO FIX **NEW 2024-03
tests\Core\Tokenizer\PHP\BitwiseOrTest.inc                                                        FAILED TO FIX
tests\Core\Tokenizer\PHP\ContextSensitiveKeywordsTest.inc                                         FAILED TO FIX
tests\Core\Tokenizer\PHP\DefaultKeywordTest.inc                                                   FAILED TO FIX
tests\Core\Tokenizer\PHP\DNFTypesParseError1Test.inc                                              FAILED TO FIX **NEW 2024-07
tests\Core\Tokenizer\PHP\DNFTypesTest.inc                                                         FAILED TO FIX **NEW 2024-07
tests\Core\Tokenizer\PHP\DoubleArrowTest.inc                                                      FAILED TO FIX
tests\Core\Tokenizer\PHP\EnumCaseTest.inc                                                         FAILED TO FIX
tests\Core\Tokenizer\PHP\TypeIntersectionTest.inc                                                 FAILED TO FIX
tests\Core\Tokenizer\Tokenizer\CreateParenthesisNestingMapDNFTypesTest.inc                        FAILED TO FIX **NEW 2024-07
tests\Core\Tokenizer\Tokenizer\RecurseScopeMapCaseKeywordConditionsTest.inc                       FAILED TO FIX **NEW 2024-07
tests\Core\Tokenizer\Tokenizer\RecurseScopeMapDefaultKeywordConditionsTest.inc                    FAILED TO FIX **NEW 2024-07
------------------------------------------------------------------------------------------------------------------
A TOTAL OF 26620 ERRORS WERE FIXED IN 471 FILES
------------------------------------------------------------------------------------------------------------------
PHPCBF FAILED TO FIX 71 FILES
------------------------------------------------------------------------------------------------------------------

Details of current fixer conflicts for the Zend standard

Command:

phpcbf -p . --standard=Zend --ignore=*/build/*,*/vendor/* --basepath=. --extensions=inc --suffix=.conflictcheck

Test case files leading to fixer conflicts

PHPCBF RESULT SUMMARY
-----------------------------------------------------------------------------------------------------------
FILE                                                                                       FIXED  REMAINING
-----------------------------------------------------------------------------------------------------------
src\Standards\Generic\Tests\Functions\FunctionCallArgumentSpacingUnitTest.1.inc            FAILED TO FIX
src\Standards\Generic\Tests\NamingConventions\ConstructorNameUnitTest.inc                  FAILED TO FIX
src\Standards\Generic\Tests\WhiteSpace\ScopeIndentUnitTest.1.inc                           FAILED TO FIX
src\Standards\Generic\Tests\WhiteSpace\ScopeIndentUnitTest.2.inc                           FAILED TO FIX
src\Standards\Squiz\Tests\WhiteSpace\ScopeClosingBraceUnitTest.inc                         FAILED TO FIX
-----------------------------------------------------------------------------------------------------------
A TOTAL OF 6354 ERRORS WERE FIXED IN 328 FILES
-----------------------------------------------------------------------------------------------------------
PHPCBF FAILED TO FIX 5 FILES
-----------------------------------------------------------------------------------------------------------

Caveat

There are some files which should probably be excluded from a fixer conflict check like this ticket proposes. Most notably test case files which contain git merge conflict markers.

For the above lists in the fold-outs, this was already done.

What needs to be done

For each of the files listed above leading to a fixer conflict, it needs to be determined whether:

The file should be excluded from the fixer conflict check; or
The fixer conflict is a realistic situation which should be handled.

How to go about this

Add *.conflictcheck to your .git/info/exclude (will be added to .gitignore when the fixer conflict check will be added for real).
Run the command listed against one particular file from the conflict list with the -vvv flag.
The output will be long, but at the bottom you will find information about the conflicting sniffs.
Identify the piece of code in the test case file which is causing the conflict.
Look critically at the piece of code causing the conflict to determine whether this is a situation which:
- should be handled by the sniffs;
- whether the code snippet is an unintentional parse error, which should be fixed. This applies to unintentional typos in a test case in contrast to a test case specifically created as a parse error to test the handling within the sniff.
- whether the code snippet is an intentional parse error and that particular piece of code should be moved to its own test case file and that file should be excluded (also see Tests: parse error tests for sniffs should be in their own file #143);
- whether the existing test case file should be excluded.
If the sniffs should handle this gracefully, add the piece of code to the test case files for all sniffs involved.
Decide for each sniff how the issue should be handled. Oftentimes, the answer is adding more defensive coding and bowing out in certain circumstances, meaning one sniff takes precedence over another.

When in doubt, share your findings and discuss the options for the conflict you are working on in a comment on this ticket or create a separate ticket to discuss solution directions if the conflict is a complex one (and link that ticket to this one).

PRs for this task should only contain the solution for one fixer conflict per PR and if the conflict can be isolated enough, the PR should be limited to fixing the issue for only one of the sniffs involved, with, if necessary, separate PRs for the other sniff(s) involved.

The text was updated successfully, but these errors were encountered:

jrfnl added Type: chores/QA Focus: Fixer Conflicts Status: needs investigation Status: help wanted labels

This was referenced Dec 13, 2023

Avoid incompatibility between Generic and PSR2 standards for function call indentation #38

Open

Fix PHP Notice in Squiz.WhiteSpace.ScopeKeywordSpacing #160

Closed

Fix PHP Notice in PSR12.Functions.ReturnTypeDeclaration #161

Closed

Fix PHP Notice in Squiz.PHP.NonExecutableCode #162

Closed

Member Author

jrfnl commented Dec 13, 2023

To prevent confusion/wasting people's time I reiterate that the task is not to get the conflict check running. I have the set up for that ready already.

Also note that if you would try to run the mentioned command without limiting them to an individual conflict which you want to investigate, there are three PHP notices which will crash the runs.

There is a PR already open to fix the first: Squiz/ScopeKeywordSpacing: add more defensive coding + extra tests #150
The second one is related to the above mentioned git merge conflict test code, which, as mentioned, will be excluded from the fixer check.
The third points to an issue with the Tokenizer and needs further investigation. Ticket Tokenizer/scope mapping: scope owner not correctly set with heredoc in condition #149 is open for that one.

Member

fredden commented Dec 13, 2023

For the PSR2 standard, the conflict for src/Standards/PSR12/Tests/Functions/ReturnTypeDeclarationUnitTest.inc seems to be between Squiz.Functions.MultiLineFunctionDeclaration.BraceIndent and Generic.WhiteSpace.ScopeIndent.Incorrect on (new) line 79. From what I can tell, it looks like Squiz.Functions.MultiLineFunctionDeclaration.BraceIndent should win in this case. This feels related to (but is not fixed by) #38.

fredden mentioned this issue

Improve context detection for PSR12.Operators.OperatorSpacing / Squiz.WhiteSpace.OperatorSpacing - avoids fixer conflict, bow out on parse error #289

Merged

6 tasks

jrfnl mentioned this issue

PSR12.Files.DeclareStatement - bow out when parse error detected #335

Merged

6 tasks

This was referenced Mar 29, 2024

PEAR/Functions/FunctionDeclaration: add extra defensive coding #420

Merged

PEAR/ScopeClosingBrace: prevent fixer conflict with itself #423

Merged

PSR2/ClassDeclaration: support namespace relative names / prevent fixer conflict #424

Merged

IsReferenceTest: fix invalid test case #426

Merged

Squiz/OperatorSpacing: bug fix - prevent fixer conflict #427

Merged

PSR2/ClassDeclaration: bug fix - prevent fixer conflict #431

Merged

Member

fredden commented Jul 14, 2024 •

edited

Loading

I've looked into the PSR12 conflict with src/Standards/Squiz/Tests/ControlStructures/ForEachLoopDeclarationUnitTest.inc. This is a conflict between Squiz.ControlStructures.ForEachLoopDeclaration and PSR2.ControlStructures.ControlStructureSpacing when certain configuration options are set on the Squiz sniff. Looking at the ruleset.xml for PSR12, I can see that the intention is to ignore parts of the Squiz sniff by marking the error codes as severity=0.

PHP_CodeSniffer/src/Standards/PSR12/ruleset.xml

Lines 294 to 300 in 071344b

    
           <!-- exclude these messages as they are already checked by PSR2.ControlStructures.ControlStructureSpacing --> 
        
           <rule ref="Squiz.ControlStructures.ForEachLoopDeclaration.SpaceAfterOpen"> 
        
               <severity>0</severity> 
        
           </rule> 
        
           <rule ref="Squiz.ControlStructures.ForEachLoopDeclaration.SpaceBeforeClose"> 
        
               <severity>0</severity> 
        
           </rule>

However, there's an inconsistency within Squiz.ControlStructures.ForEachLoopDeclaration which I believe is the cause of this conflict. The sniff can be configured with properties, and there's an if/else block to handle the two options (no space / some number of spaces).

For "space before close", the two branches report the same error code: SpaceBeforeClose. This is what's listed in the ruleset.xml above. See lines 107 and 123.

For "space after open", the two branches report different error codes: SpaceAfterOpen and SpacingAfterOpen. Only one of these is listed in ruleset.xml above. See lines 77 (space) and 93 (spacing).

PHP_CodeSniffer/src/Standards/Squiz/Sniffs/ControlStructures/ForEachLoopDeclarationSniff.php

Lines 75 to 133 in 071344b

    
           if ($this->requiredSpacesAfterOpen === 0 && $tokens[($openingBracket + 1)]['code'] === T_WHITESPACE) { 
        
               $error = 'Space found after opening bracket of FOREACH loop'; 
        
               $fix   = $phpcsFile->addFixableError($error, $stackPtr, 'SpaceAfterOpen'); 
        
               if ($fix === true) { 
        
                   $phpcsFile->fixer->replaceToken(($openingBracket + 1), ''); 
        
               } 
        
           } else if ($this->requiredSpacesAfterOpen > 0) { 
        
               $spaceAfterOpen = 0; 
        
               if ($tokens[($openingBracket + 1)]['code'] === T_WHITESPACE) { 
        
                   $spaceAfterOpen = $tokens[($openingBracket + 1)]['length']; 
        
               } 
        
               if ($spaceAfterOpen !== $this->requiredSpacesAfterOpen) { 
        
                   $error = 'Expected %s spaces after opening bracket; %s found'; 
        
                   $data  = [ 
        
                       $this->requiredSpacesAfterOpen, 
        
                       $spaceAfterOpen, 
        
                   ]; 
        
                   $fix   = $phpcsFile->addFixableError($error, $stackPtr, 'SpacingAfterOpen', $data); 
        
                   if ($fix === true) { 
        
                       $padding = str_repeat(' ', $this->requiredSpacesAfterOpen); 
        
                       if ($spaceAfterOpen === 0) { 
        
                           $phpcsFile->fixer->addContent($openingBracket, $padding); 
        
                       } else { 
        
                           $phpcsFile->fixer->replaceToken(($openingBracket + 1), $padding); 
        
                       } 
        
                   } 
        
               } 
        
           }//end if 
        
           if ($this->requiredSpacesBeforeClose === 0 && $tokens[($closingBracket - 1)]['code'] === T_WHITESPACE) { 
        
               $error = 'Space found before closing bracket of FOREACH loop'; 
        
               $fix   = $phpcsFile->addFixableError($error, $stackPtr, 'SpaceBeforeClose'); 
        
               if ($fix === true) { 
        
                   $phpcsFile->fixer->replaceToken(($closingBracket - 1), ''); 
        
               } 
        
           } else if ($this->requiredSpacesBeforeClose > 0) { 
        
               $spaceBeforeClose = 0; 
        
               if ($tokens[($closingBracket - 1)]['code'] === T_WHITESPACE) { 
        
                   $spaceBeforeClose = $tokens[($closingBracket - 1)]['length']; 
        
               } 
        
               if ($spaceBeforeClose !== $this->requiredSpacesBeforeClose) { 
        
                   $error = 'Expected %s spaces before closing bracket; %s found'; 
        
                   $data  = [ 
        
                       $this->requiredSpacesBeforeClose, 
        
                       $spaceBeforeClose, 
        
                   ]; 
        
                   $fix   = $phpcsFile->addFixableError($error, $stackPtr, 'SpaceBeforeClose', $data); 
        
                   if ($fix === true) { 
        
                       $padding = str_repeat(' ', $this->requiredSpacesBeforeClose); 
        
                       if ($spaceBeforeClose === 0) { 
        
                           $phpcsFile->fixer->addContentBefore($closingBracket, $padding); 
        
                       } else { 
        
                           $phpcsFile->fixer->replaceToken(($closingBracket - 1), $padding); 
        
                       } 
        
                   } 
        
               } 
        
           }//end if

To fix this, I can see several options. I'd like an opinion about what is the "best" way forward here.

Update the ruleset.xml for PSR12 to exclude all three error codes. Ignore the inconsistency within the sniff.
Fix the inconsistency in the sniff: change line 93 from SpacingAfterOpen to SpaceAfterOpen. No change to PSR12 ruleset required.
Fix the inconsistency in the sniff: change line 123 from SpaceBeforeClose to SpacingBeforeClose. Update the PSR12 ruleset to exclude all four codes.
Fix the inconsistency in the sniff: use "spacing..." not "space..." in all sniff codes. Update the PSR12 ruleset to match.

I'd like to proceed with option 2 (or maybe 4), but recognise that this can be seen as a backwards-incompatible change.

For reference, these are all the codes reported by this sniff: AsNotLower, MissingAs, MissingCloseParenthesis, MissingOpenParenthesis, NoSpaceAfterArrow, NoSpaceAfterAs, NoSpaceBeforeArrow, NoSpaceBeforeAs, SpaceAfterOpen, SpacingAfterOpen, SpaceBeforeClose, SpaceBeforeClose, SpacingAfterArrow, SpacingAfterAs, SpacingBeforeArrow, SpacingBeforeAs

@jrfnl what are your thoughts on this?

fredden mentioned this issue

Rewrite PSR12.Files.DeclareStatement sniff #578

Open

11 tasks

Member Author

jrfnl commented Jul 29, 2024 •

edited

Loading

@fredden We've looked at this together today and in my opinion, option 2 is the way to go.

If I look at both the Squiz ForLoop and ForeachLoop sniffs, it looks like the intention was for those errors to have the same error code anyway (no space/too much space, still both SpaceAfterOpen issue).

If I then look at the ForeachLoop sniff, the "after open"/"before close" error codes have 3 x Space... and 1 x Spacing..., so I think SpaceAfterOpen should prevail.

While this removes an error code, I would not consider this a breaking change which needs to be in a major, though I would put it in a minor.

It will not break rulesets (as invalid error codes are just disregarded).
It may flag errors previously ignored, but I expect that to be exceedingly rare as ignoring things which are auto-fixable is rare in the first place.
Additionally, like PSR12, the chances are that more rulesets only excluded two of the three error codes and fixing the error code will (in most cases) fix the ruleset for those projects, so should be seen as a bug fix.

This was referenced Jul 29, 2024

AbstractPatternSniff: prevent PHP notice #579

Merged

Squiz/OperatorBracket: prevent PHP notices during live coding #580

Merged

This was referenced Jul 29, 2024

Fix conflict within PSR12.ControlStructures.ControlStructureSpacing #581

Merged

Correct error code in Squiz.ControlStructures.ForEachLoopDeclaration #582

Merged

fredden mentioned this issue

Fix fixer conflict: PSR12 / Squiz.Functions.FunctionDeclarationArgumentSpacing #620

Open

11 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment