PHPCSStandards
PSR2/NamespaceDeclaration: do not enforce new line in inline HTML
Description
If a namespace declaration has a PHP close tag on the same line, the new line would be added as T_INLINE_HTML and therefore not be recognized as a blank line as the sniff solely looks for T_WHITESPACE. This then leads to a fixer conflict where the sniff just keeps adding new lines until it runs out of loops.
=> Fixing file: 0/6 violations remaining [made 46 passes]...
* fixed 0 violations, starting loop 47 *
PSR2.Namespaces.NamespaceDeclaration:81 replaced token 118 (T_INLINE_HTML on line 32) "\n\n" => "\n\n\n"
PSR2.Namespaces.NamespaceDeclaration:81 replaced token 153 (T_INLINE_HTML on line 58) "\n\n" => "\n\n\n"
=> Fixing file: 2/6 violations remaining [made 47 passes]...
* fixed 2 violations, starting loop 48 *
**** PSR2.Namespaces.NamespaceDeclaration:81 has possible conflict with another sniff on loop 46; caused by the following change ****
**** replaced token 118 (T_INLINE_HTML on line 32) "\n\n" => "\n\n\n" ****
**** ignoring all changes until next loop ****
=> Fixing file: 0/6 violations remaining [made 48 passes]...
* fixed 0 violations, starting loop 49 *
In my opinion, the sniff should bow out in that situation and should not enforce a new line as it may impact HTML display.
This commit implements this.
Includes a test safeguarding the fix.
Suggested changelog entry
PSR2.Namespaces.NamespaceDeclaration: prevent fixer conflict when the next thing after a namespace declaration is a PHP close tag
Related issues/external references
Related to #152
Types of changes
- [x] Bug fix (non-breaking change which fixes an issue)
FYI: Build failure is unrelated to this PR, but has to do with an outage at Coveralls. See https://github.com/lemurheavy/coveralls-public/issues/1791 and https://status.coveralls.io/
@fredden Thanks for thinking about this. Unfortunately that is not how it works. We cannot interpret PSR's, we have to follow them to the letter. And there is nowhere in PSR2 where it spells out that no other code is allowed on the same line as a namespace declaration.
So before we can change the behaviour of the sniff regarding this, whether or not other code (and/or comments) is allowed on the same line as a namespace declaration would need to be discussed in the PSR mailinglist or, more likely at present time: the PSR-PER repo. This will need to be clarified via the meta doc/addendum data for PSR-2 and only after that, would we be clear to make a change.
To be honest, I'm perfectly happy for someone to go down that rabbit hole, but it won't be me and it is out of scope for this PR in my opinion.
The rule says that "there MUST be one blank line after the namespace declaration." A "blank line" means two consecutive newline characters (otherwise the line between these two characters is not blank). "one blank line" means that three consecutive newline characters is not allowed (as that would be "two blank lines", not the prescribed "one"). I'll concede that "after the namespace declaration" could strictly be satisfied several lines further down the file, but I think that's stretching the literalism too far in this case. I interpret that "after" as "immediately following".
I would argue that if another PHP statement were to be between the namespace declaration and its required blank line, then PSR-2 is not being respected, but I can see this is an interpretation.
If we're going to go with an only-literal interpretation of the rule, then the sniff is broken differently. It should be searching for two consecutive newline characters in all the rest of the file (in HTML or between PHP statements or in a comment), and not complaining if any such cases are found anywhere in the file after the declaration.
@fredden The way PSRs have been implemented in sniffs has always been strictly to the letter (including addendums) in this repo. This is not new. I'm just following Greg's lead in this and in the old repo, you can find numerous discussions demonstrating this.
For both of your suggestions, the discussion should be had with the PSR people. Not here.
Given the controversy around changing the behaviour of this sniff, let's focus on and fix the fixer conflict only. I think a8aaa01eb4db281a60eea4847da7024852c996e9 might be enough to solve the fixer conflict.