From ae6bd698d81f8e355ce08127fefbb105e90c520f Mon Sep 17 00:00:00 2001
From: Rodrigo Primo <rodrigosprimo@gmail.com>
Date: Fri, 9 Aug 2024 15:05:40 -0300
Subject: [PATCH 1/2] Squiz/EmbeddedPhp: fix false positive when handling short
 open tags

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.
---
 .../Squiz/Sniffs/PHP/EmbeddedPhpSniff.php     |  9 ++++---
 .../Squiz/Tests/PHP/EmbeddedPhpUnitTest.1.inc |  4 +++
 .../Tests/PHP/EmbeddedPhpUnitTest.1.inc.fixed |  4 +++
 .../Tests/PHP/EmbeddedPhpUnitTest.24.inc      | 25 +++++++++++++++++++
 .../PHP/EmbeddedPhpUnitTest.24.inc.fixed      | 25 +++++++++++++++++++
 .../Squiz/Tests/PHP/EmbeddedPhpUnitTest.php   | 12 +++++++++
 6 files changed, 76 insertions(+), 3 deletions(-)
 create mode 100644 src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.24.inc
 create mode 100644 src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.24.inc.fixed

diff --git a/src/Standards/Squiz/Sniffs/PHP/EmbeddedPhpSniff.php b/src/Standards/Squiz/Sniffs/PHP/EmbeddedPhpSniff.php
index 0be6118e43..184924b3e7 100644
--- a/src/Standards/Squiz/Sniffs/PHP/EmbeddedPhpSniff.php
+++ b/src/Standards/Squiz/Sniffs/PHP/EmbeddedPhpSniff.php
@@ -379,8 +379,11 @@ 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 = $tokens[$stackPtr]['code'] === T_OPEN_TAG
+                         && stripos($tokens[$stackPtr]['content'], '<?php') === 0;
+
+        if ($isLongOpenTag === true) {
             // The long open tag token in a single line tag set always contains a single space after it.
             $leadingSpace = 1;
         }
@@ -394,7 +397,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.
diff --git a/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.1.inc b/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.1.inc
index f28c559894..015468357d 100644
--- a/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.1.inc
+++ b/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.1.inc
@@ -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.
diff --git a/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.1.inc.fixed b/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.1.inc.fixed
index f309d439ff..a0b6a30ca9 100644
--- a/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.1.inc.fixed
+++ b/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.1.inc.fixed
@@ -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.
diff --git a/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.24.inc b/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.24.inc
new file mode 100644
index 0000000000..dc1749c330
--- /dev/null
+++ b/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.24.inc
@@ -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.
diff --git a/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.24.inc.fixed b/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.24.inc.fixed
new file mode 100644
index 0000000000..8e0f87a4f8
--- /dev/null
+++ b/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.24.inc.fixed
@@ -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.
diff --git a/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.php b/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.php
index 29111c8736..7454859269 100644
--- a/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.php
+++ b/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.php
@@ -100,6 +100,7 @@ public function getErrorList($testFile='')
                 258 => 1,
                 263 => 1,
                 264 => 1,
+                270 => 1,
             ];
 
         case 'EmbeddedPhpUnitTest.2.inc':
@@ -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,
+                ];
+            } else {
+                return [];
+            }
+
         default:
             return [];
         }//end switch

From 9bc7b0b9383a11f863d8a51b47fde1afce3b3c2c Mon Sep 17 00:00:00 2001
From: Rodrigo Primo <rodrigosprimo@gmail.com>
Date: Tue, 13 Aug 2024 13:13:31 -0300
Subject: [PATCH 2/2] Apply suggestions from code review

Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com>
---
 src/Standards/Squiz/Sniffs/PHP/EmbeddedPhpSniff.php   | 11 ++++++-----
 src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.php |  3 +--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/Standards/Squiz/Sniffs/PHP/EmbeddedPhpSniff.php b/src/Standards/Squiz/Sniffs/PHP/EmbeddedPhpSniff.php
index 184924b3e7..63a1cdd09a 100644
--- a/src/Standards/Squiz/Sniffs/PHP/EmbeddedPhpSniff.php
+++ b/src/Standards/Squiz/Sniffs/PHP/EmbeddedPhpSniff.php
@@ -380,12 +380,13 @@ private function validateInlineEmbeddedPhp($phpcsFile, $stackPtr, $closeTag)
 
         // Check that there is one, and only one space at the start of the statement.
         $leadingSpace  = 0;
-        $isLongOpenTag = $tokens[$stackPtr]['code'] === T_OPEN_TAG
-                         && stripos($tokens[$stackPtr]['content'], '<?php') === 0;
-
-        if ($isLongOpenTag === true) {
+        $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) {
diff --git a/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.php b/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.php
index 7454859269..410f938918 100644
--- a/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.php
+++ b/src/Standards/Squiz/Tests/PHP/EmbeddedPhpUnitTest.php
@@ -198,9 +198,8 @@ public function getErrorList($testFile='')
                     18 => 1,
                     20 => 1,
                 ];
-            } else {
-                return [];
             }
+            return [];
 
         default:
             return [];