PHPCSStandards/PHP_CodeSniffer

Squiz.PHP.DisallowMultipleAssignments false positive for list assignments after embedded PHP without semicolon

biinari opened this issue · 1 comments

Describe the bug

Following a short echo tag with an expression, a false positive is given by Squiz.PHP.DisallowMultipleAssignments for destructured assignment using list()

Code sample

<?php
$a = 1;
$b = 2;
?>
<?= $a * $b ?>
<?php
list ($c, $d) = explode(',', '1,2');

Custom ruleset

<?xml version="1.0"?>
<ruleset name="My Custom Standard">
  <description>Ruleset to demonstrate this issue</description>
  <rule ref="Squiz">
    <exclude name="Generic.PHP.DisallowShortOpenTag.EchoFound" />
    <exclude name="Squiz.Commenting.FileComment" />
    <exclude name="Squiz.Formatting.OperatorBracket.MissingBrackets" />
    <exclude name="Squiz.PHP.EmbeddedPhp.ShortOpenEchoNoSemicolon" />
  </rule>
</ruleset>

To reproduce

Steps to reproduce the behaviour:

  1. Create a file called test.php with the code sample above
  2. Create a file called phpcs.xml with the custom ruleset above
  3. Run phpcs test.php
  4. See error message displayed
FILE: (path snipped)/test.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 7 | ERROR | Assignments must be the first block of code on a line
----------------------------------------------------------------------

Expected behavior

I expect no error to be found by Squiz.PHP.DisallowMultipleAssignments here.

Versions

Operating System [Ubuntu 22.04.4]
PHP version [7.4, 8.3]
PHP_CodeSniffer version [3.10.1, master]
Standard [Squiz, custom]
Install type [Composer (local)]

Additional context

The syntax given on the PHP tags documentation demonstrates use without a semicolon like:

<?= 'print this string' ?>

This leads me to believe that such usage should be allowed.

I tripped into this issue when removing a line of code that sat between the short echo tag with the expression and the destructuring assignment.

Further interesting factors:

  1. A block (e.g. if, while) can be added before line 7 and the sniff will still issue an error.
  2. Surrounding the short echo tag with brackets <?= ($a * $b) ?> resolves this issue. Enabling Squiz.Formatting.OperatorBracket.MissingBrackets would enforce this usage.
  3. Adding a semicolon to the short echo tag <?= $a * $b; ?> resolves this issue. Enabling Squiz.PHP.EmbeddedPhp.ShortOpenEchoNoSemicolon would enforce this usage.
  4. A single-line statement added before line 7 will resolve this issue.

Please confirm

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

@biinari Thanks for opening this bug report. Nice find!

I've had a quick look and have confirmed the bug. This is unrelated to the short echo tag though, the same behaviour can be observed when using a full PHP open tag for the embedded PHP snippet.

Basically, the sniff walks back too far and doesn't take code going in and out of PHP into account sufficiently.

PR #538 should fix this. Testing appreciated.