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

[jrfnl]

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.

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\Tokenizer\DefaultKeywordTest.inc                                                FAILED TO FIX
tests\Core\Tokenizer\EnumCaseTest.inc                                                      FAILED TO FIX
-----------------------------------------------------------------------------------------------------------
A TOTAL OF 9100 ERRORS WERE FIXED IN 269 FILES
-----------------------------------------------------------------------------------------------------------
PHPCBF FAILED TO FIX 14 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\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
tests\Core\Tokenizer\ScopeSettingWithNamespaceOperatorTest.inc                             FAILED TO FIX
tests\Core\Tokenizer\UndoNamespacedNameSingleTokenTest.inc                                 FAILED TO FIX
-----------------------------------------------------------------------------------------------------------
A TOTAL OF 8898 ERRORS WERE FIXED IN 320 FILES
-----------------------------------------------------------------------------------------------------------
PHPCBF FAILED TO FIX 5 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\ControlStructures\DisallowYodaConditionsUnitTest.inc           FAILED TO FIX
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.inc                         FAILED TO FIX
src\Standards\PEAR\Tests\NamingConventions\ValidVariableNameUnitTest.inc                   FAILED TO FIX
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.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\PSR2\Tests\Methods\FunctionClosingBraceUnitTest.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.inc                   FAILED TO FIX
src\Standards\Squiz\Tests\ControlStructures\ForEachLoopDeclarationUnitTest.inc             FAILED TO FIX
src\Standards\Squiz\Tests\ControlStructures\ForLoopDeclarationUnitTest.inc                 FAILED TO FIX
src\Standards\Squiz\Tests\PHP\EmbeddedPhpUnitTest.inc                                      FAILED TO FIX
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\FindEndOfStatementTest.inc                                                 FAILED TO FIX
tests\Core\File\FindStartOfStatementTest.inc                                               FAILED TO FIX
tests\Core\File\GetMemberPropertiesTest.inc                                                FAILED TO FIX
tests\Core\File\GetMethodParametersTest.inc                                                FAILED TO FIX
tests\Core\Tokenizer\AttributesTest.inc                                                    FAILED TO FIX
tests\Core\Tokenizer\BackfillMatchTokenTest.inc                                            FAILED TO FIX
tests\Core\Tokenizer\BitwiseOrTest.inc                                                     FAILED TO FIX
tests\Core\Tokenizer\ContextSensitiveKeywordsTest.inc                                      FAILED TO FIX
tests\Core\Tokenizer\DefaultKeywordTest.inc                                                FAILED TO FIX
tests\Core\Tokenizer\DoubleArrowTest.inc                                                   FAILED TO FIX
tests\Core\Tokenizer\EnumCaseTest.inc                                                      FAILED TO FIX
tests\Core\Tokenizer\ScopeSettingWithNamespaceOperatorTest.inc                             FAILED TO FIX
tests\Core\Tokenizer\TypeIntersectionTest.inc                                              FAILED TO FIX
tests\Core\Tokenizer\UndoNamespacedNameSingleTokenTest.inc                                 FAILED TO FIX
-----------------------------------------------------------------------------------------------------------
A TOTAL OF 23586 ERRORS WERE FIXED IN 379 FILES
-----------------------------------------------------------------------------------------------------------
PHPCBF FAILED TO FIX 57 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.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 5760 ERRORS WERE FIXED IN 282 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:

1. The file should be excluded from the fixer conflict check; or
2. 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.

[jrfnl]

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.

[fredden]

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.