Squiz/EmbeddedPhp: bug fix - fixer conflict with itself #833
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
... the sniff would previously throw the following error:
... 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:
ContentBeforeOpen
error andContentBeforeOpen
error (for the indent when the tag is moved to the next line), as well as for theIndent
error via a newcalculateLineIndent()
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:
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