Rewrite PSR12.Files.DeclareStatement sniff

[fredden]

Description

While investigating a fixer conflict within the PSR12 standard (see #152), I noticed that the PSR12.Files.DeclareStatement sniff was doing the wrong thing for this test file: src/Standards/Generic/Tests/PHP/RequireStrictTypesUnitTest.5.inc. Specifically, it was processing tokens well beyond the end of the declare() statement and conflicting with the PSR12.Operators.OperatorSpacing sniff for code after the declare() statement. While attempting to fix this particular issue, I decided that a complete rewrite would be easier than trying to squeeze a patch in for the specific conflict.

I have copied this test file so that we do not get any regressions in this sniff.

Suggested changelog entry

Complete rewrite of PSR12.Files.DeclareStatement. Error codes have been preserved where feasible.

Related issues/external references

• QA: standards should not have conflicting rules / fix fixer conflicts #152
• PSR12.Files.DeclareStatement - bow out when parse error detected #335
• https://www.php-fig.org/psr/psr-12/#3-declare-statements-namespace-and-import-statements

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.

Table of differences between sniffs

Scenario	Old Sniff Behaviour	New Sniff Changes
Well-formed declare statement with semicolon.	Nothing reported.	No change.
Well-formed declare statement with code block.	Nothing reported.	No change.
Well-formed declare statement with closing PHP tag (instead of semicolon)	Nothing reported.	No change.
Well-formed declare statement with integer value	Nothing reported.	No change.
Well-formed declare statement with string value	Error reported (‘SpaceFoundBeforeDirectiveValue’) on value token. Error reported (‘SpaceFoundBeforeSemicolon’) on the first token in the file. Error reported ('SpaceFoundAfterDeclare’) on the first token in the file.	Nothing reported.
Space between ‘declare’ and ‘(‘	Error reported (‘SpaceFoundAfterDeclare’) on ‘declare’ token. Fixer available.	Error reported (same code) on space token. Better grammar in message.
Comment between ‘declare’ and ‘(‘	Error reported (‘SpaceFoundAfterDeclare’) on comment token. Not fixable.	Better grammar in message.
Space between ‘(‘ and directive (eg, ‘ticks’)	Error reported (‘SpaceFoundBeforeDirective’) on open parenthesis token. Fixer available.	Error reported (same code) on space token. Better grammar in message.
Comment between ‘(‘ and directive (eg, ‘ticks’)	Error reported ('SpaceFoundBeforeDirective’) on comment token. Not fixable.	Better grammar in message.
Directive not in lowercase.	Error reported (‘DirectiveNotLowercase’) on directive token. Fixer available.	No change.
Space between directive and ‘=’	Error reported (‘SpaceFoundAfterDirective’) on equals token. Fixer available.	Error reported (same code) on space token. Better grammar in message.
Comment between directive and ‘=’	Error reported ('SpaceFoundAfterDirective’) on comment token. Not fixable.	Better grammar in message.
Space between ‘=’ and value	Error reported (‘SpaceFoundBeforeDirectiveValue’) on value token. Fixer available.	Error reported (same code) on space token. Better grammar in message.
Comment between ‘=’ and value	Error reported on ('SpaceFoundBeforeDirectiveValue’) on comment token. Not fixable.	Better grammar in message.
Value is integer	No errors detected.	No change.
Value is fixed string		No errors detected.
Space between value and ’)’	Error reported (‘SpaceFoundAfterDirectiveValue’) on close parenthesis token. Fixer available.	Error reported (same code) on space token. Better grammar in message.
Comment between value and ’)’	Error reported (‘SpaceFoundAfterDirectiveValue’) on comment token. Not fixable.	Better grammar in message.
Space between ’)’ and ‘;’	Error reported ('SpaceFoundBeforeSemicolon’) on close parenthesis token. Fixer available.	Error reported (same code) on space token. Better grammar in message.
Comment between ’)’ and ‘;’	Error reported ('SpaceFoundBeforeSemicolon’) on comment token. Not fixable.	Better grammar in message.
No space between ’)’ and ‘?>’	No errors detected.	Error reported ('NoSpaceFoundBeforeClosePHPTag’) on closing PHP tag token. Fixer available.
Single space between ’)’ and ‘?>’	No errors detected.	No change.
Multiple spaces between ’)’ and ‘?>’	No errors detected.	Error reported ('ExtraSpaceFoundBeforeClosePHPTag’) on space token. Fixer available.
Comment between ’)’ and ‘?>’	Error reported ('SpaceFoundBeforeSemicolon’) on close parenthesis token. Not fixable.	Error reported (‘NonSpaceFoundBeforeClosePHPTag’) on comment token. Better message.
Various parse errors	Various errors reported, at least one PHP Notice for undefined array index.	No errors detected.
No space between ’)’ and ‘{‘	Error reported (‘ExtraSpaceFoundAfterBracket’) on opening curly bracket. Fixer available.	Error reported (‘NoSpaceFoundBeforeCurlyBracket’) on opening curly bracket. Better grammar in message.
More than one space between ’)’ and ‘{‘	Error reported ('ExtraSpaceFoundAfterBracket’) on opening curly bracket. Fixer available.	Error reported ('ExtraSpaceFoundBeforeCurlyBracket’) on space token. Better grammar in message.
Comment on same line after ‘{‘	No errors reported.	No change.
PHP statement on same line after ‘{‘	Error reported ('CodeFoundAfterCurlyBracket’) on opening curly bracket. Fixer available.	Error reported ('CodeFoundAfterOpenCurlyBracket’) on PHP statement.
PHP statement on same line before ’}'	Error reported ('CurlyBracketNotOnNewLine’) on closing curly bracket. Fixer available.	No change.
PHP statement on same line after ’}’	No errors reported.	Error reported (‘CodeFoundAfterCloseCurlyBracket’) on PHP statement. Fixer available.

[jrfnl]

@fredden I thought we'd discussed before that I was already working on a rewrite of this one ? [Edit: we did, also see #335, which you referenced yourself above]

As things are, I don't know where to even start reviewing this PR as other than "there are fixer conflicts", it is completely unclear what you are trying fix....

[fredden]

The same things are being detected here. The difference is that we are highlighting the problem token, not the correct token. This is a consistency benefit from using the new private method on the sniff.

[fredden]

Here we are now reporting fewer problems. We used to report that the opening bracket should be the last thing on the line, and that the closing bracket needs to be on a line by itself. Now we're only reporting about the closing bracket. This is an implementation detail. If there were some PHP statements (or a comment) between the { and }, then both would be reported again.

[fredden]

Two changes here: one is the same as lines 13 & 14, and the other is the same as line 16 above.

[fredden]

This is the same as lines 13 & 14 above.

[fredden]

This is the same as line 16 above.

[fredden]

This is an error that the previous version of this sniff didn't detect.

[fredden]

This is a copy of src/Standards/Generic/Tests/PHP/RequireStrictTypesUnitTest.5.inc to ensure that the sniff does not act on line 6.

[jrfnl]

@fredden I appreciate the comments, they help a little, but they still do not explain enough as they are vague and non-specific.

I know exactly what the problems are with this sniff (and there are plenty) and I have no clue which of those problems this PR is supposed to address.

To give an example of the "non-specific":

Error codes have been preserved where feasible.

Which error codes have changed ? This needs to be detailed in the changelog as end-users may need to update their ruleset.

As for me, my questions regarding this statement are: Why were they changed/Why couldn't the change be avoided ?

This is just one example of the non-specific.

As things are, to review this, I will basically have to go through the complete dev journey you've gone through to figure out what you found, what the problem was, how it should be fixed and then to review if the fix made is correct.
And then I will still need to fix everything else (and rewrite this sniff once again).

[fredden]

@jrfnl let's discuss this on our next call (tomorrow). I started the work before I remembered that you said you wanted to do this. I have added a table of differences between the old & new sniffs to the pull request description. You mentioned problems that you are aware of and that you would need to rewrite the sniff again anyway, but I don't see those details documented here on GitHub. Perhaps I'm not looking in the right places / using the search effectively enough.

I've updated the code to include more test cases (the vast majority of which the previous sniff fails). I would like to get some XML documentation added too.

[jrfnl]

@jrfnl let's discuss this on our next call (tomorrow).

@fredden Let's.

I have added a table of differences between the old & new sniffs to the pull request description.

That is helpful, but underlines the opinion that this PR should have been split into multiple commits or even better, multiple PRs as a number of these changes are highly disputable (like the checks for spacing for the PHP close tag as those will very quickly conflict with a sniff which is dedicated to PHP close tags).

You mentioned problems that you are aware of and that you would need to rewrite the sniff again anyway, but I don't see those details documented here on GitHub. Perhaps I'm not looking in the right places / using the search effectively enough.

Correct, I didn't open an issue for this as there is so much and I intended to just fix it. I did mention the rewrite though whenever the sniff was brought up.

To give you some idea of the more pertinent problems:

• The sniff doesn't handle multi-directive declare statements.
• The sniff breaks hard on an encoding directive as it doesn't allow for string values.
• The token walking contains way too many assumptions. This is largely the reason why the sniff will conflict with other sniffs.
• The logic, error messages and fixers do not take comments in unexpected places into account correctly.
• All fixers are highly inefficient as they only fix one whitespace token at a time, meaning the fixer could need multiple loops to fix one error.
• The errors are thrown on the wrong token in most cases (based on your table, this is part of what you are addressing in this PR).
• End of statement semi-colon vs PHP close tag (based on your table, I have a feeling this is part of what you are addressing in this PR).

I've updated the code to include more test cases (the vast majority of which the previous sniff fails).

Making the PR even larger is not going to help at this time.

I would like to get some XML documentation added too.

As noted in the docs issue (#148), that one is on hold until after the sniff rewrite.