Squiz.PHP.EmbeddedPhp false positive when using PHP short open tag

rodrigoprimo commented 1 month ago

Describe the bug

When the short_open_tag ini directive is enabled, the sniff Squiz.PHP.EmbeddedPhp raises a false positive error for code that uses a single-line statement with the short open tag.

Code sample

<? echo 'This is a short open tag'; ?>

To reproduce

Steps to reproduce the behavior:

Enable the short_open_tag ini directive in the PHP configuration file.
Create a file called test.php with the code sample above.
Run phpcs --standard=Squiz --sniffs=Squiz.PHP.EmbeddedPhp test.php

See the error message displayed

FILE: test.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
9 | ERROR | [x] Expected 1 space after opening PHP tag; 2 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Expected behavior

PHPCS should report no errors.

Versions (please complete the following information)


Operating System	Ubuntu 24.04
PHP version	8.3
PHP_CodeSniffer version	master
Standard	Squiz
Install type	git clone

Additional context

The sniff assumes that there is always a space after the PHP open tag that is part of the content of the T_OPEN_TAG token. That is true for the PHP long open tag, but not for the PHP short open tag. When processing a short open tag, the sniff "sees" an extra space that doesn't exist, causing it to report the error, saying that it expected one space but found two.

Here is the code which was added in 3172a21:

https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/ac5fd07fa2db2335fd739e19c34e39ad923bc758/src/Standards/Squiz/Sniffs/PHP/EmbeddedPhpSniff.php#L381-L386

I believe the sniff should be fixed by changing the if condition highlighted above to differentiate between the short open tag and the long open tag when the directive short_open_tag is enabled. This could be done by checking the value of $tokens[$stackPtr]['cotent']. I wonder if there is a better way to do it.

Please confirm

[x] I have searched the issue list and am not opening a duplicate issue.
[x] I have read the Contribution Guidelines and this is not a support question.
[x] I confirm that this bug is a bug in PHP_CodeSniffer and not in one of the external standards.
[x] I have verified the issue still exists in the master branch of PHP_CodeSniffer.

jrfnl commented 1 month ago

@rodrigoprimo Nice find. Low priority to fix as it is uncommon for short open tags (not echo) to be enabled, let alone used.

I believe the sniff should be fixed by changing the if condition highlighted above to differentiate between the short open tag and the long open tag when the directive short_open_tag is enabled. This could be done by checking the value of $tokens[$stackPtr]['cotent']. I wonder if there is a better way to do it.

I don't think the directive short_open_tag needs to be checked, as if it is off, <? would not be tokenized as T_OPEN_TAG and wouldn't trigger the sniff.

Checking with the current condition + a content check for stripos($content, '<?php') === 0 should be sufficient I think ? Note the use of stripos() - it should also be ensured that there are tests with <?PHP to safeguard that this new condition is properly covered.

rodrigoprimo commented 1 month ago

Thanks for checking, @jrfnl. I agree that this is a low-priority issue, as you said. It seems somewhat straightforward to fix it, so I might give it a try if I have some time in the next few days.

rodrigoprimo commented 1 month ago

Noting here that I found a scenario where this bug, combined with another sniff, can cause a parse error. Running the following PHPCBF command against the same code sample used in the description of the issue will result in invalid PHP file.

phpcbf test.php --standard=Generic,Squiz --sniffs=Squiz.PHP.EmbeddedPhp,Generic.PHP.DisallowShortOpenTag

Here is the resulting PHP file:

<?php phpecho 'This is a short open tag'; ?>

Here is the part of the phpcbf -vvv output that shows what is causing the problem:

---START FILE CONTENT---
1|<? echo 'This is a short open tag'; ?>
2|
--- END FILE CONTENT ---
    Generic.PHP.DisallowShortOpenTag:67 replaced token 0 (T_OPEN_TAG on line 1) "<?" => "<?php"
    Squiz.PHP.EmbeddedPhp:398 replaced token 1 (T_WHITESPACE on line 1) "·echo" => "echo"
        => Fixing file: 2/2 violations remaining [made 1 pass]...               
    * fixed 2 violations, starting loop 2 *
---START FILE CONTENT---
1|<?phpecho 'This is a short open tag'; ?>
2|
--- END FILE CONTENT ---
    Generic.PHP.DisallowShortOpenTag:67 replaced token 0 (T_OPEN_TAG on line 1) "<?" => "<?php·"
        => Fixing file: 1/2 violations remaining [made 2 passes]...             
    * fixed 1 violations, starting loop 3 *
---START FILE CONTENT---
1|<?php phpecho 'This is a short open tag'; ?>
2|
--- END FILE CONTENT ---
        => Fixing file: 0/2 violations remaining [made 3 passes]...             
DONE in 1ms
    => File was overwritten

On a first pass, Generic.PHP.DisallowShortOpenTag replaces <? with <?php. Then Squiz.PHP.EmbeddedPhp removes the whitespace before the echo keyword. Resulting in <?phpecho. On a second pass, <? is tokenized as T_OPEN_TAG and phpecho as T_STRING. So, Generic.PHP.DisallowShortOpenTag replaces <? with <?php again, resulting in <?php phpecho.

jrfnl commented 1 month ago

@rodrigoprimo Good catch, still low prio.

Having said that, might be good to check if the Generic.PHP.DisallowShortOpenTag sniff fixer checks whether there is a whitespace char after the short open tag before replacing it and adjusts the replacement value if not. That would be a separate issue though.

rodrigoprimo commented 1 month ago

Having said that, might be good to check if the Generic.PHP.DisallowShortOpenTag sniff fixer checks whether there is a whitespace char after the short open tag before replacing it and adjusts the replacement value if not. That would be a separate issue though.

@jrfnl, good point, I checked, and the sniff does check that:

https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/84acf4e56f110db8e75cb9a575c5727df637643c/src/Standards/Generic/Sniffs/PHP/DisallowShortOpenTagSniff.php#L61-L65 https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/8b7beb182c5db1026261c94eb8afa818a8fb6dbc/src/Standards/Generic/Tests/PHP/DisallowShortOpenTagUnitTest.2.inc#L7 https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/8b7beb182c5db1026261c94eb8afa818a8fb6dbc/src/Standards/Generic/Tests/PHP/DisallowShortOpenTagUnitTest.2.inc.fixed#L7

The problem only happens when the two sniffs are executed together. So it seems to be a case of conflict between the fixers. I'm assuming it is not worth investigating this further as the issue in Squiz.PHP.EmbeddedPhp will be fixed and, as you said, this is low priority as the PHP short open tag is not commonly used. But please let me know if you want me to look more into this.

jrfnl commented 1 week ago

Closing as fixed via #591

PHPCSStandards / PHP_CodeSniffer