Skip to content

Commit

Permalink
Squiz/EmbeddedPhp: fix false positive when handling short open tags
Browse files Browse the repository at this point in the history
The content of a PHP long open tag token is `<?php ` (note
the space after the tag). The content of a PHP short open tag is
`<?` (no space after the tag). The sniff did not account correctly for
this difference when checking the expected number of spaces after a
short open tag, resulting in false positives and incorrect fixes.

For example, the code below:

```
<? echo 'one space after short open tag'; ?>
```

Resulted in the error (there is just one space after the opening tag and
not two, as stated in the error):

```
Expected 1 space after opening PHP tag; 2 found (Squiz.PHP.EmbeddedPhp.SpacingAfterOpen)
```

And the incorrect fix:

```
<?echo 'without space after short open tag'; ?>
```

This commit fixes this problem by changing the sniff code to consider
that only long open tags contain a space after the tag in the `content`
key of the token array.
  • Loading branch information
rodrigoprimo authored and jrfnl committed Aug 13, 2024
1 parent c76678a commit 0ed5577
Show file tree
Hide file tree
Showing 6 changed files with 78 additions and 4 deletions.
12 changes: 8 additions & 4 deletions src/Standards/Squiz/Sniffs/PHP/EmbeddedPhpSniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -379,10 +379,14 @@ private function validateInlineEmbeddedPhp($phpcsFile, $stackPtr, $closeTag)
}

// Check that there is one, and only one space at the start of the statement.
$leadingSpace = 0;
if ($tokens[$stackPtr]['code'] === T_OPEN_TAG) {
$leadingSpace = 0;
$isLongOpenTag = false;
if ($tokens[$stackPtr]['code'] === T_OPEN_TAG
&& stripos($tokens[$stackPtr]['content'], '<?php') === 0
) {
// The long open tag token in a single line tag set always contains a single space after it.
$leadingSpace = 1;
$leadingSpace = 1;
$isLongOpenTag = true;
}

if ($tokens[($stackPtr + 1)]['code'] === T_WHITESPACE) {
Expand All @@ -394,7 +398,7 @@ private function validateInlineEmbeddedPhp($phpcsFile, $stackPtr, $closeTag)
$data = [$leadingSpace];
$fix = $phpcsFile->addFixableError($error, $stackPtr, 'SpacingAfterOpen', $data);
if ($fix === true) {
if ($tokens[$stackPtr]['code'] === T_OPEN_TAG) {
if ($isLongOpenTag === true) {
$phpcsFile->fixer->replaceToken(($stackPtr + 1), '');
} else if ($tokens[($stackPtr + 1)]['code'] === T_WHITESPACE) {
// Short open tag with too much whitespace.
Expand Down
4 changes: 4 additions & 0 deletions src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.1.inc
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,10 @@ echo 'the PHP tag is correctly indented as an indent less than the previous code
echo 'the PHP tag is incorrectly indented as the indent is more than 4 different from the indent of the previous code';
?>

<?PHP echo 'Uppercase long open tag'; ?>

<?PHP echo 'Extra space after uppercase long open tag '; ?>

<?php
// This test case file MUST always end with an unclosed long open PHP tag (with this comment) to prevent
// the tests running into the "last PHP closing tag excepted" condition breaking tests.
Expand Down
4 changes: 4 additions & 0 deletions src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.1.inc.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,10 @@ echo 'the PHP tag is correctly indented as an indent less than the previous code
echo 'the PHP tag is incorrectly indented as the indent is more than 4 different from the indent of the previous code';
?>

<?PHP echo 'Uppercase long open tag'; ?>

<?PHP echo 'Extra space after uppercase long open tag '; ?>

<?php
// This test case file MUST always end with an unclosed long open PHP tag (with this comment) to prevent
// the tests running into the "last PHP closing tag excepted" condition breaking tests.
Expand Down
25 changes: 25 additions & 0 deletions src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.24.inc
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?

// This test case file MUST always start with a open PHP tag set (with this comment) to prevent
// the tests running into the "first PHP open tag excepted" condition breaking the tests.
// Tests related to that "first PHP open tag excepted" condition should go in separate files.

// This test case file only deals with SHORT OPEN TAGS.

?>

<?
/* Contrary to the long open tag token, the short open tag token does not contain a space after the
tag and the sniff should handle it accordingly. The test below protects against regressions
related to https://github.com/PHPCSStandards/PHP_CodeSniffer/issues/588. */
?>
<? echo 'one space after short open tag'; ?>

<? echo 'two spaces after short open tag'; ?>

<?echo 'without space after short open tag'; ?>

<?
// This test case file MUST always end with an unclosed open PHP tag (with this comment) to prevent
// the tests running into the "last PHP closing tag excepted" condition breaking tests.
// Tests related to that "last PHP closing tag excepted" condition should go in separate files.
25 changes: 25 additions & 0 deletions src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.24.inc.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?

// This test case file MUST always start with a open PHP tag set (with this comment) to prevent
// the tests running into the "first PHP open tag excepted" condition breaking the tests.
// Tests related to that "first PHP open tag excepted" condition should go in separate files.

// This test case file only deals with SHORT OPEN TAGS.

?>

<?
/* Contrary to the long open tag token, the short open tag token does not contain a space after the
tag and the sniff should handle it accordingly. The test below protects against regressions
related to https://github.com/PHPCSStandards/PHP_CodeSniffer/issues/588. */
?>
<? echo 'one space after short open tag'; ?>

<? echo 'two spaces after short open tag'; ?>

<? echo 'without space after short open tag'; ?>

<?
// This test case file MUST always end with an unclosed open PHP tag (with this comment) to prevent
// the tests running into the "last PHP closing tag excepted" condition breaking tests.
// Tests related to that "last PHP closing tag excepted" condition should go in separate files.
12 changes: 12 additions & 0 deletions src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ public function getErrorList($testFile='')
258 => 1,
263 => 1,
264 => 1,
270 => 1,
];

case 'EmbeddedPhpUnitTest.2.inc':
Expand Down Expand Up @@ -190,6 +191,17 @@ public function getErrorList($testFile='')
22 => 2,
];

case 'EmbeddedPhpUnitTest.24.inc':
$shortOpenTagDirective = (bool) ini_get('short_open_tag');
if ($shortOpenTagDirective === true) {
return [
18 => 1,
20 => 1,
];
}

return [];

default:
return [];
}//end switch
Expand Down

0 comments on commit 0ed5577

Please sign in to comment.