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

Squiz/EmbeddedPhp: bug fix - fixer conflict with itself #833

Merged
merged 1 commit into from
Feb 20, 2025

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Feb 18, 2025

Description

For multi-line PHP snippets, the Squiz.PHP.EmbeddedPhp sniff expects the open and close tag to each be on their own line. For single-line PHP snippets, it does not.

Now, the sniff already handled a close tag of a previous multi-line snippet and the open tag of a next multi-line snippet being on the same line correctly and prevented a fixer conflict for that, but it did not correctly handle the open tag of a multi-line snippet being on the same line after a single-line PHP snippet.

In that case, it would not recognize that the open tag had to be moved to its own line and it would also calculate the expected indent for both the PHP open tag as well as the first line of the content within the multi-line snippet incorrectly, which in turn would lead to fixer conflicts with the Generic.WhiteSpace.ScopeIndent sniff.

I.e. for the new test added:

<?php echo 'Open tag after code'; ?>            <?php
echo $j;
?>

... the sniff would previously throw the following error:

ERROR | [x] Opening PHP tag indent incorrect; expected no more than 4 spaces but found 48 (Squiz.PHP.EmbeddedPhp.OpenTagIndent)
ERROR | [x] First line of embedded PHP code must be indented 11 spaces; 0 found (Squiz.PHP.EmbeddedPhp.Indent)

... and the fixer would conflict and try to add the same indent to the open tag time and time again, but without adding a new line, which meant it was replacing the token content with the existing content, not fixing anything. Hence, the fixer conflict with itself.

Also take note of the incorrect indent expectation for the next line (11 spaces).

This commit fixes both issues by:

  1. Improving the "does this line containing a PHP open tag have content on it before" verification for the ContentBeforeOpen error and
  2. Fixing the "what should the indent be" calculation for both the ContentBeforeOpen error (for the indent when the tag is moved to the next line), as well as for the Indent error via a new calculateLineIndent() method which takes a lot more potential "first token on a line which may contain whitespace" situations into account.

With the fix in place, it will now show the following error:

ERROR | [x] Opening PHP tag must be on a line by itself (Squiz.PHP.EmbeddedPhp.ContentBeforeOpen)

Includes implementing the use of the new calculateLineIndent() method in other places in the sniff to make sure this calculation is consistent throughout the sniff.

Includes tests, also specifically for the new method.

The change does mean that some existing snippets now get two errors instead of one when a close tag + an open tag for multi-line snippets are on the same line. In my opinion, this should be regarded as a bug fix for the second error previously not showing. As for the fixer, the end-result for those snippets is the same, so it doesn't result in a new conflict (as the sniff already contains protection against that specific conflict).

For the reviewers: I've verified that the error messages for pre-existing tests involving indent calculations are 100% the same before and after the change.

Suggested changelog entry

Squiz.PHP.EmbeddedPhp: fixer conflict when a PHP open tag for a multi-line snippet is found on the same line as a single-line embedded PHP snippet
Squiz.PHP.EmbeddedPhp: fixed incorrect indent calculation in certain specific situations

Related issues/external references

Related to #152

Types of changes

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

For multi-line PHP snippets, the `Squiz.PHP.EmbeddedPhp` sniff expects the open and close tag to each be on their own line.
For single-line PHP snippets, it does not.

Now, the sniff already handled a close tag of a previous multi-line snippet and the open tag of a next multi-line snippet being on the same line correctly and prevented a fixer conflict for that, but it did not correctly handle the open tag of a multi-line snippet being on the same line after a single-line PHP snippet.

In that case, it would not recognize that the open tag had to be moved to its own line and it would also calculate the expected indent for both the PHP open tag as well as the first line of the content within the multi-line snippet incorrectly, which in turn would lead to fixer conflicts with the `Generic.WhiteSpace.ScopeIndent` sniff.

I.e. for the new test added:
```php
<?php echo 'Open tag after code'; ?>            <?php
echo $j;
?>
```
... the sniff would previously throw the following error:
```
ERROR | [x] Opening PHP tag indent incorrect; expected no more than 4 spaces but found 48 (Squiz.PHP.EmbeddedPhp.OpenTagIndent)
ERROR | [x] First line of embedded PHP code must be indented 11 spaces; 0 found (Squiz.PHP.EmbeddedPhp.Indent)
```
... and the fixer would conflict and try to add the same indent to the open tag time and time again, but without adding a new line, which meant it was replacing the token content with the existing content, not fixing anything. Hence, the fixer conflict with itself.

Also take note of the incorrect indent expectation for the next line (11 spaces).

This commit fixes both issues by:
1. Improving the "does this line containing a PHP open tag have content on it before" verification for the `ContentBeforeOpen` error and
2. Fixing the "what should the indent be" calculation for both the `ContentBeforeOpen` error (for the indent when the tag is moved to the next line), as well as for the `Indent` error via a new `calculateLineIndent()` method which takes a lot more potential "first token on a line which may contain whitespace" situations into account.

With the fix in place, it will now show the following error:
```
ERROR | [x] Opening PHP tag must be on a line by itself (Squiz.PHP.EmbeddedPhp.ContentBeforeOpen)
```

Includes implementing the use of the new `calculateLineIndent()` method in other places in the sniff to make sure this calculation is consistent throughout the sniff.

Includes tests, also specifically for the new method.

The change does mean that some existing snippets now get two errors instead of one when a close tag + an open tag for multi-line snippets are on the same line.
In my opinion, this should be regarded as a bug fix for the second error previously not showing.
As for the fixer, the end-result for those snippets is the same, so it doesn't result in a new conflict (as the sniff already contains protection against that specific conflict).

For the reviewers: I've verified that the error messages for pre-existing tests involving indent calculations are 100% the same before and after the change.
@jrfnl jrfnl merged commit f668a5b into master Feb 20, 2025
59 checks passed
@jrfnl jrfnl deleted the feature/squiz-embeddedphp-fixer-conflict branch February 20, 2025 15:01
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