Squiz.PHP.EmbeddedPhp: fixer conflict with `//` comment before PHP close tag

[jrfnl]

Summary

Code like this:

<?php true; // wat() ?>

will cause a fixer conflict within the Squiz.PHP.EmbeddedPhp sniff when trying to fix the Expected 1 space before closing PHP tag; error - errorcode Squiz.PHP.EmbeddedPhp.SpacingBeforeClose.

The issues exists in both PHPCS 2.x as well as 3.x.

If at all possible, if would be very much appreciated if any fixes for this would also be accepted for the PHPCS 2.x branch as this issue was discovered as part of the WP Core project to get the codebase up to scratch coding standards wise.

Problem analysis

The fixer conflicts with itself.
The tokenizer will tokenize whitespace at the end of a // comment as part of the T_COMMENT token, so the fixer just keeps adding more spaces between the T_COMMENT and the T_CLOSE_TAG which in the next fixer round is then tokenized again as being part of the T_COMMENT resulting in the infinite loop.

Possible solutions

There are two ways this could be fixed:

• Either in the tokenizer by detecting that a T_COMMENT is followed on the same line by a T_CLOSE_PHP token and in that case tokenizing any whitespace at the end of the T_COMMENT as T_WHITESPACE.
• Or it could be fixed in the sniff itself.

The sniff based fix is actually quite simple and I've got the code ready for this - for both the 2.x as well as the 3.x branch - if that solution would be considered acceptable and I'd be happy to send you the PRs.

Relevant part of the report-diff output

        *** START FILE FIXING ***
        E: [Line 10] Expected 1 space before closing PHP tag; 0 found (Squiz.PHP.EmbeddedPhp.Spacing
BeforeClose)
        Squiz_Sniffs_PHP_EmbeddedPhpSniff (line 395) replaced token 29 (T_CLOSE_TAG) "?>\n" => " ?>\
n"
        * fixed 1 violations, starting loop 2 *
        E: [Line 10] Expected 1 space before closing PHP tag; 0 found (Squiz.PHP.EmbeddedPhp.Spacing
BeforeClose)
        Squiz_Sniffs_PHP_EmbeddedPhpSniff (line 395) replaced token 29 (T_CLOSE_TAG) "?>\n" => " ?>\
n"
        * fixed 1 violations, starting loop 3 *
        E: [Line 10] Expected 1 space before closing PHP tag; 0 found (Squiz.PHP.EmbeddedPhp.Spacing
BeforeClose)
        **** Squiz_Sniffs_PHP_EmbeddedPhpSniff (line 395) has possible conflict with another sniff o
n loop 1; caused by the following change ****
        **** replaced token 29 (T_CLOSE_TAG) "?>\n" => " ?>\n" ****
        **** ignoring all changes until next loop ****
        * fixed 0 violations, starting loop 4 *

[jrfnl]

@gsherwood Any thoughts on which of the two possible solutions you'd prefer ?
As I mentioned above, if a sniff-based one would be acceptable, I have the branches for this ready to be pulled.

[gsherwood]

Sorry, I haven't had time to look at this at all, but a sniff-based fix would be preferred.

[jrfnl]

@gsherwood Thanks for letting me know. The sniff based fix is quite straight forward & simple.
I've pulled it now as #1553 (PHPCS 2.x) and #1554 (PHPCS 3.x). Both include unit tests demonstrating the issue and the fix.

[gsherwood]

Thanks for those PRs.

[jrfnl]

Thanks for merging them & allowing the fix to also go into the 2.x branch ❤️