Skip to content
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.

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

PEAR/ScopeClosingBrace: prevent fixer conflict with itself #423

Merged
merged 1 commit into from
Apr 3, 2024

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Mar 29, 2024

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)

Copy link
Member

@fredden fredden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

jrfnl commented Mar 30, 2024

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
Copy link
Member

fredden commented Mar 30, 2024

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 jrfnl modified the milestones: 3.9.1, 3.9.2 Mar 31, 2024
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.

Related to 152
@jrfnl jrfnl force-pushed the feature/pear-scopeclosingbrace-fix-fixer-conflict branch from 7903dd7 to 436d243 Compare April 3, 2024 00:49
@jrfnl
Copy link
Member Author

jrfnl commented Apr 3, 2024

Rebased without changes, merging once the build has passed.

@jrfnl jrfnl enabled auto-merge April 3, 2024 00:52
@jrfnl jrfnl merged commit 872add0 into master Apr 3, 2024
44 checks passed
@jrfnl jrfnl deleted the feature/pear-scopeclosingbrace-fix-fixer-conflict branch April 3, 2024 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants