search

Automattic / VIP-Coding-Standards

PHP_CodeSniffer ruleset to enforce WordPress VIP coding standards.
https://wpvip.com/documentation/how-to-install-php-code-sniffer-for-wordpress-com-vip/
Other
236 stars 40 forks source link

ProperEscapingFunction: Fix short tag detection #748

Closed GaryJones closed 1 year ago

GaryJones commented 1 year ago

Suggested fix for #739. Includes unit tests. First commit is separate just to keep the noise down for the actual fix in the second commit.

Applying the suggested fix (reset tracking variable to false in process_token() method) from #739 caused a failure of an existing unit test - presumably because this meant it would get reset when processing a T_STRING token, and not just a T_OPEN_TAG_WITH_ECHO token (both are returned from register()), leading to a case where the tracking variable was incorrect.

I've also made an assumption, that numbered .inc files are always processed in the same logical order (1, then 2, then 3). I tested alternative versions, and could successfully NOT trigger the bug when the new unit test files were numbered in the reverse order (and 1.inc was temporarily moved to 4.inc, as that seemed to trigger the bug as well coincidentally).

ProperEscapingFunction: Prep for multi-file tests

Renames the singular test .inc file to include a number, to allow for more incoming test files.

ProperEscapingFunction: Fix short tag detection

The tracking variable $in_short_echo was never reset when checking different files, meaning that the property would carry over and provide the wrong context to the next file.

By adding a process() method to the ProperEscapingFunctionSniff, we can reset the tracking variable at the start of each file by comparing the currently processing file to the last one stored in a static variable.

Includes two unit test files, numbered in the order needed to trigger the bug if the fix wasn't present.

Fixes #739.

GaryJones commented 1 year ago

IMO, we don't actually need the process() method, this snippet can just as easily be added at the top of the process_token() method. The parent process() method only assigns the $phpcsFile and $tokens properties, it doesn't do anything else.

@jrfnl I was under the impression, probably wrongly, that process() ran once per file, and process_token() ran once per registered token, and therefore less work would be done by adding the reset logic to process().

jrfnl commented 1 year ago

IMO, we don't actually need the process() method, this snippet can just as easily be added at the top of the process_token() method. The parent process() method only assigns the $phpcsFile and $tokens properties, it doesn't do anything else.

@jrfnl I was under the impression, probably wrongly, that process() ran once per file, and process_token() ran once per registered token, and therefore less work would be done by adding the reset logic to process().

It's the register() method which is only called once per sniff. The process() method is called for each token register-ed to be listened to. Also see: https://github.com/WordPress/WordPress-Coding-Standards/blob/546f59c67854589bb8f6b49a30e642e75ff419ad/WordPress/Sniff.php#L418-L433

jrfnl commented 1 year ago

Follow up on my earlier remark about the test case file sorting: https://github.com/squizlabs/PHP_CodeSniffer/pull/3775