PEAR/ScopeClosingBrace: prevent fixer conflict with itself

[jrfnl]

Description

The PEAR.WhiteSpace.ScopeClosingBrace sniff could get into a conflict with itself when the scope closer was preceded by a PHP open tag and before that non-empty (i.e. non-indent) inline HTML.

In that case, the sniff would not recognize that the close brace was not on a line by itself, as the search for the last content before would disregard the non-empty HTML and would continue searching, which meant that the "last relevant content before" token would point to a token on a previous line.

This, in turn, then led to the sniff continuing on to the next error "Closing brace indented incorrectly", where the indent would now be incorrectly determined as -1, which in the fixer would lead to the original content and the replacement content being exactly the same, which created a fixer conflict.

Fixed now by improving the "last content before" determination.

Includes unit test.

Suggested changelog entry

Fixed PEAR.WhiteSpace.ScopeClosingBrace: when a close tag was preceded by non-empty inline HTML, the sniff would have a fixer conflict with itself.

Related issues/external references

Related to #152

Types of changes

• Bug fix (non-breaking change which fixes an issue)

[fredden]

These changes make sense and correctly cover the test case added. The test case makes sense to me and is distinct from line 146 as that line has a ?> token on the line too.

I've not spent time to understand the fixer conflict properly. The changes here look good to me. If they happen to resolve another related issue, then that's a bonus.

[jrfnl]

Thanks for looking this over @fredden !

These changes make sense and correctly cover the test case added. The test case makes sense to me and is distinct from line 146 as that line has a ?> token on the line too.

The biggest difference with the test case on line 146 (and why that case previously already worked correctly) is that line 146 contains a mix of non-empty inline HTML and PHP code before the <?php endif;, while the new test only contains (non-empty) inline HTML before the <?php endif;.

Leaving the ending ?> out of the new test case is arbitrary (I just did it as the test case is at the end of the file) and makes no difference to the test case.

The test case on line 146 is actually the reason for why I left the fixer alone.
I originally considered moving not just the endif;, but the <?php endif; for the Line ("Closing brace must be on a line by itself") error, but realized due to the test case on line 146 that that would break existing expectations for the fixer, which is why I didn't go that way in the end.

I've not spent time to understand the fixer conflict properly. The changes here look good to me. If they happen to resolve another related issue, then that's a bonus.

For the record: The fixer conflict was that the fixer for the Indent error on line 171 would try to replace </div> with </div> (same contents for both the original as well as the replacement) due to the presumption in the sniff that the indent would be an empty (indentation) T_INLINE_HTML token, not a non-empty T_INLINE_HTML token.

By fixing the Line error to work correctly, the Indent fixer never gets triggered for this code snippet.

[fredden]

Leaving the ending ?> out of the new test case is arbitrary

I meant that there was a ?> token on the same line before the <?php endif;. What comes after wasn't relevant for that part of this sniff.

[jrfnl]

Rebased without changes, merging once the build has passed.