Faulty Core Fixes: Comment inside a single line embedded PHP block

[pento]

After a173ae7, this code will get stuck in a loop:

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

It works correctly if the comment is removed. (The a is so that the ?> isn't removed by PSR2.Files.ClosingTag.NotAllowed.)

In Core, this can be seen in src/wp-admin/custom-background.php.

[jrfnl]

@pento Thanks for reporting this one. Lovely simple example & the fixer conflict is not between two different sniffs, but within the EmbeddedPhp sniff 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 loop.

This is an upstream issue and we'll need to keep our fingers crossed for a fix to still be accepted for PHPCS 2.x. There are two ways this could be fixed: either in the tokenizer or in the sniff, I will open an issue upstream to see what can be done.

In the mean time: Have you got any idea whether there is more code in WP where this is happening ?
If it's only the one instance, it might be simpler (and save us the headache of trying to still get a fix into the 2.x branch) to just change the comment style in this case to /* ... */ which will not suffer this problem.

[pento]

There are 30 instances, across 19 files.

I'm globally excluding the Squiz.PHP.EmbeddedPhp.SpacingBeforeClose rule in my testing, but I could pretty easily change that to excluding it for just those files, too.

Changing the comment style for those instances feels kind of weird.

[jrfnl]

Changing the comment style for those instances feels kind of weird.

If it was just the one instance it would be a quick fix, with 30, I agree that's not a solution.

I've opened an issue upstream to report the conflict and to discuss which solution would be acceptable and whether it can still go into PHPCS 2.x: squizlabs/PHP_CodeSniffer#1549

In the mean time, I've also created a patch. The patch is for the sniff-based solution.
If the patch would not be accepted anymore for PHPCS 2.x, we could choose to temporarily add an adjusted copy of the upstream sniff - which includes the fix - to WPCS until such time when the minimum PHPCS requirement for WPCS has gone up to whatever 3.x version will include the patch.

[jrfnl]

FYI: Patches for both the PHPCS 2.x & 3.x branches have been pulled upstream in the mean time. Fingers crossed they'll both get accepted.

[jrfnl]

@pento My PRs upstream have been merged - both the 2.x as well as the 3.x versions! 🎉

Are you testing with the 2.9.1 release or with the 2.9 branch of PHPCS ? If the latter, you should be able to remove the exclusions related to this sniff and run PHPCS to see the fix in action.

[pento]

I'm on the 2.9.1 release - I don't suppose there's an easy way to switch VVV to the 2.9 branch? :)

[jrfnl]

I don't use VVV myself much and haven't used it for quite a while anyhow, so I'm not the best person to ask.

I suppose you could check the VVV provision scripts to see how VVV pulls in PHPCS - Composer/Git.

• If git, just open up the repository, fetch the latest changes and make sure you are on the 2.9 branch.
• If Composer, you could change the composer.json to point to the 2.9 branch instead of a release. You may also need to remove a prefer-stable directive if it's included, though I'm not sure whether that would really be needed.

After that you will probably need to reprovision the box.

[pento]

Got it, it works! 😀

[ntwb]

I had "squizlabs/php_codesniffer": "*", in my composer.json yet phpcs --version report 2.9.1

/shrug

I've now updated that to "squizlabs/php_codesniffer": "2.9.x-dev", and phpcs --version now reports PHP_CodeSniffer version 2.9.2 (stable)

[jrfnl]

@ntwb That sounds about right as PHPCS 2.9.2 has not been released yet. (and I'm presuming you also have WPCS develop as a dependency as otherwise you would have gotten PHPCS 3.x)

[ntwb]

Yup: "wp-coding-standards/wpcs": "dev-develop"