Generic/UnnecessaryStringConcat: improve code coverage

src/Standards/Generic/Sniffs/Strings/UnnecessaryStringConcatSniff.php

@@ -70,54 +70,58 @@ public function register()
      */
     public function process(File $phpcsFile, $stackPtr)
     {
-        // Work out which type of file this is for.
         $tokens = $phpcsFile->getTokens();
-        if ($tokens[$stackPtr]['code'] === T_STRING_CONCAT) {
-            if ($phpcsFile->tokenizerType === 'JS') {
-                return;
-            }
-        } else {
-            if ($phpcsFile->tokenizerType === 'PHP') {
-                return;
-            }
+
+        if ($tokens[$stackPtr]['code'] === T_STRING_CONCAT && $phpcsFile->tokenizerType === 'JS') {
+            // JS uses T_PLUS for string concatenation, not T_STRING_CONCAT.
+            return;
+        } else if ($tokens[$stackPtr]['code'] === T_PLUS && $phpcsFile->tokenizerType === 'PHP') {
+            // PHP uses T_STRING_CONCAT for string concatenation, not T_PLUS.
+            return;
         }
 
         $prev = $phpcsFile->findPrevious(T_WHITESPACE, ($stackPtr - 1), null, true);
         $next = $phpcsFile->findNext(T_WHITESPACE, ($stackPtr + 1), null, true);
-        if ($prev === false || $next === false) {
+        if ($next === false) {
+            return;
+        }
+
+        if (isset(Tokens::$stringTokens[$tokens[$prev]['code']]) === false
+            || isset(Tokens::$stringTokens[$tokens[$next]['code']]) === false
+        ) {
+            // Bow out as at least one of the two tokens being concatenated is not a string.
             return;
         }
 
-        if (isset(Tokens::$stringTokens[$tokens[$prev]['code']]) === true
-            && isset(Tokens::$stringTokens[$tokens[$next]['code']]) === true
+        if ($tokens[$prev]['content'][0] !== $tokens[$next]['content'][0]) {
+            // Bow out as the two strings are not of the same type.
+            return;
+        }
+
+        // Before we throw an error for PHP, allow strings to be
+        // combined if they would have < and ? next to each other because
+        // this trick is sometimes required in PHP strings.
+        if ($phpcsFile->tokenizerType === 'PHP') {
+            $prevChar = substr($tokens[$prev]['content'], -2, 1);
+            $nextChar = $tokens[$next]['content'][1];
+            $combined = $prevChar.$nextChar;
+            if ($combined === '?'.'>' || $combined === '<'.'?') {
+                return;
+            }
+        }
+
+        if ($this->allowMultiline === true
+            && $tokens[$prev]['line'] !== $tokens[$next]['line']
         ) {
-            if ($tokens[$prev]['content'][0] === $tokens[$next]['content'][0]) {
-                // Before we throw an error for PHP, allow strings to be
-                // combined if they would have < and ? next to each other because
-                // this trick is sometimes required in PHP strings.
-                if ($phpcsFile->tokenizerType === 'PHP') {
-                    $prevChar = substr($tokens[$prev]['content'], -2, 1);
-                    $nextChar = $tokens[$next]['content'][1];
-                    $combined = $prevChar.$nextChar;
-                    if ($combined === '?'.'>' || $combined === '<'.'?') {
-                        return;
-                    }
-                }
-
-                if ($this->allowMultiline === true
-                    && $tokens[$prev]['line'] !== $tokens[$next]['line']
-                ) {
-                    return;
-                }
-
-                $error = 'String concat is not required here; use a single string instead';
-                if ($this->error === true) {
-                    $phpcsFile->addError($error, $stackPtr, 'Found');
-                } else {
-                    $phpcsFile->addWarning($error, $stackPtr, 'Found');
-                }
-            }//end if
-        }//end if
+            return;
+        }
+
+        $error = 'String concat is not required here; use a single string instead';
+        if ($this->error === true) {
+            $phpcsFile->addError($error, $stackPtr, 'Found');
+        } else {
+            $phpcsFile->addWarning($error, $stackPtr, 'Found');
+        }
 
     }//end process()
 

src/Standards/Generic/Tests/Strings/UnnecessaryStringConcatUnitTest.1.inc

@@ -0,0 +1,34 @@
+<?php
+$x = 'My '.'string';
+$x = 'My '. 1234;
+$x = 'My '.$y.' test';
+
+echo $data['my'.'index'];
+echo $data['my'. 4];
+echo $data['my'.$x];
+echo $data[$x.$y.'My'.'String'];
+
+$code = '$actions = array();'."\n";
+$code = "$actions = array();"."\n";
+
+// No errors for these because they are needed in some cases.
+$code = ' ?'.'>';
+$code = '<'.'?php ';
+
+$string = 'This is a really long string. '
+        . 'It is being used for errors. '
+        . 'The message is not translated.';
+
+$shouldBail = 1 + 1;
+
+$shouldNotTrigger = 'My' . /* comment */ 'string';
+$shouldNotTrigger = 'My' /* comment */ . 'string';
+
+// phpcs:set Generic.Strings.UnnecessaryStringConcat allowMultiline true
+$string = 'Multiline strings are allowed '
+        . 'when setting is enabled.';
+// phpcs:set Generic.Strings.UnnecessaryStringConcat allowMultiline false
+
+// phpcs:set Generic.Strings.UnnecessaryStringConcat error false
+$throwWarning = 'My' . 'string';
+// phpcs:set Generic.Strings.UnnecessaryStringConcat error true

src/Standards/Generic/Tests/Strings/UnnecessaryStringConcatUnitTest.2.inc

@@ -0,0 +1,7 @@
+<?php
+
+// Intentional parse error (only empty tokens after T_STRING_CONCAT).
+// This should be the only test in this file.
+// Testing that the sniff is *not* triggered.
+
+$parseError = 'String' .

src/Standards/Generic/Tests/Strings/UnnecessaryStringConcatUnitTest.php

@@ -33,7 +33,7 @@ final class UnnecessaryStringConcatUnitTest extends AbstractSniffUnitTest
     public function getErrorList($testFile='')
     {
         switch ($testFile) {
-        case 'UnnecessaryStringConcatUnitTest.inc':
+        case 'UnnecessaryStringConcatUnitTest.1.inc':
             return [
                 2  => 1,
                 6  => 1,
@@ -65,11 +65,21 @@ public function getErrorList($testFile='')
      * The key of the array should represent the line number and the value
      * should represent the number of warnings that should occur on that line.
      *
+     * @param string $testFile The name of the file being tested.
+     *
      * @return array<int, int>
      */
-    public function getWarningList()
+    public function getWarningList($testFile='')
     {
-        return [];
+        switch ($testFile) {
+        case 'UnnecessaryStringConcatUnitTest.1.inc':
+            return [
+                33 => 1,
+            ];
+
+        default:
+            return [];
+        }
 
     }//end getWarningList()