fix: no_useless_concat_operator - do not break variable (#7827)

PHP-CS-Fixer · Mar 11, 2024 · b52108e · b52108e
1 parent 291fdbf
commit b52108e
Show file tree

Hide file tree

Showing 5 changed files with 125 additions and 38 deletions.
diff --git a/src/Fixer/Operator/NoUselessConcatOperatorFixer.php b/src/Fixer/Operator/NoUselessConcatOperatorFixer.php
@@ -26,6 +26,13 @@
 use PhpCsFixer\Tokenizer\Token;
 use PhpCsFixer\Tokenizer\Tokens;
 
+/**
+ * @phpstan-type _ConcatOperandType array{
+ *     start: int,
+ *     end: int,
+ *     type: self::STR_*,
+ * }
+ */
 final class NoUselessConcatOperatorFixer extends AbstractFixer implements ConfigurableFixerInterface
 {
     private const STR_DOUBLE_QUOTE = 0;
@@ -47,7 +54,7 @@ public function getDefinition(): FixerDefinitionInterface
      * {@inheritdoc}
      *
      * Must run before DateTimeCreateFromFormatCallFixer, EregToPregFixer, PhpUnitDedicateAssertInternalTypeFixer, RegularCallableCallFixer, SetTypeToCastFixer.
-     * Must run after NoBinaryStringFixer, SingleQuoteFixer.
+     * Must run after ExplicitStringVariableFixer, NoBinaryStringFixer, SingleQuoteFixer.
      */
     public function getPriority(): int
     {
@@ -105,16 +112,8 @@ protected function createConfigurationDefinition(): FixerConfigurationResolverIn
     }
 
     /**
-     * @param array{
-     *     start: int,
-     *     end: int,
-     *     type: self::STR_*,
-     * } $firstOperand
-     * @param array{
-     *     start: int,
-     *     end: int,
-     *     type: self::STR_*,
-     * } $secondOperand
+     * @param _ConcatOperandType $firstOperand
+     * @param _ConcatOperandType $secondOperand
      */
     private function fixConcatOperation(Tokens $tokens, array $firstOperand, int $concatIndex, array $secondOperand): void
     {
@@ -130,6 +129,10 @@ private function fixConcatOperation(Tokens $tokens, array $firstOperand, int $co
         }
 
         if (self::STR_DOUBLE_QUOTE_VAR === $firstOperand['type'] && self::STR_DOUBLE_QUOTE_VAR === $secondOperand['type']) {
+            if ($this->operandsCanNotBeMerged($tokens, $firstOperand, $secondOperand)) {
+                return;
+            }
+
             $this->mergeConstantEscapedStringVarOperands($tokens, $firstOperand, $concatIndex, $secondOperand);
 
             return;
@@ -146,6 +149,10 @@ private function fixConcatOperation(Tokens $tokens, array $firstOperand, int $co
             [$operand1, $operand2] = $operandPair;
 
             if (self::STR_DOUBLE_QUOTE_VAR === $operand1['type'] && self::STR_DOUBLE_QUOTE === $operand2['type']) {
+                if ($this->operandsCanNotBeMerged($tokens, $operand1, $operand2)) {
+                    return;
+                }
+
                 $this->mergeConstantEscapedStringVarOperands($tokens, $firstOperand, $concatIndex, $secondOperand);
 
                 return;
@@ -169,6 +176,10 @@ private function fixConcatOperation(Tokens $tokens, array $firstOperand, int $co
                 $operantContent = $tokens[$operand2['start']]->getContent();
 
                 if ($this->isSimpleQuotedStringContent($operantContent)) {
+                    if ($this->operandsCanNotBeMerged($tokens, $operand1, $operand2)) {
+                        return;
+                    }
+
                     $this->mergeConstantEscapedStringVarOperands($tokens, $firstOperand, $concatIndex, $secondOperand);
                 }
 
@@ -180,11 +191,7 @@ private function fixConcatOperation(Tokens $tokens, array $firstOperand, int $co
     /**
      * @param -1|1 $direction
      *
-     * @return null|array{
-     *     start: int,
-     *     end: int,
-     *     type: self::STR_*,
-     * }
+     * @return null|_ConcatOperandType
      */
     private function getConcatOperandType(Tokens $tokens, int $index, int $direction): ?array
     {
@@ -216,16 +223,8 @@ private function getConcatOperandType(Tokens $tokens, int $index, int $direction
     }
 
     /**
-     * @param array{
-     *     start: int,
-     *     end: int,
-     *     type: self::STR_*,
-     * } $firstOperand
-     * @param array{
-     *     start: int,
-     *     end: int,
-     *     type: self::STR_*,
-     * } $secondOperand
+     * @param _ConcatOperandType $firstOperand
+     * @param _ConcatOperandType $secondOperand
      */
     private function mergeConstantEscapedStringOperands(
         Tokens $tokens,
@@ -249,24 +248,16 @@ private function mergeConstantEscapedStringOperands(
     }
 
     /**
-     * @param array{
-     *     start: int,
-     *     end: int,
-     *     type: self::STR_*,
-     * } $firstOperand
-     * @param array{
-     *     start: int,
-     *     end: int,
-     *     type: self::STR_*,
-     * } $secondOperand
+     * @param _ConcatOperandType $firstOperand
+     * @param _ConcatOperandType $secondOperand
      */
     private function mergeConstantEscapedStringVarOperands(
         Tokens $tokens,
         array $firstOperand,
         int $concatOperatorIndex,
         array $secondOperand
     ): void {
-        // build uo the new content
+        // build up the new content
         $newContent = '';
 
         foreach ([$firstOperand, $secondOperand] as $operant) {
@@ -336,4 +327,36 @@ private function containsLinebreak(Tokens $tokens, int $startIndex, int $endInde
 
         return false;
     }
+
+    /**
+     * @param _ConcatOperandType $firstOperand
+     * @param _ConcatOperandType $secondOperand
+     */
+    private function operandsCanNotBeMerged(Tokens $tokens, array $firstOperand, array $secondOperand): bool
+    {
+        // If the first operand does not end with a variable, no variables would be broken by concatenation.
+        if (self::STR_DOUBLE_QUOTE_VAR !== $firstOperand['type']) {
+            return false;
+        }
+        if (!$tokens[$firstOperand['end'] - 1]->isGivenKind(T_VARIABLE)) {
+            return false;
+        }
+
+        $allowedPatternsForSecondOperand = [
+            '/^\s.*/', // e.g. " foo", ' bar', " $baz"
+            '/^-(?!\>)/', // e.g. "-foo", '-bar', "-$baz"
+        ];
+
+        // If the first operand ends with a variable, the second operand should match one of the allowed patterns.
+        // Otherwise, the concatenation can break a variable in the first operand.
+        foreach ($allowedPatternsForSecondOperand as $allowedPattern) {
+            $secondOperandInnerContent = substr($tokens->generatePartialCode($secondOperand['start'], $secondOperand['end']), 1, -1);
+
+            if (Preg::match($allowedPattern, $secondOperandInnerContent)) {
+                return false;
+            }
+        }
+
+        return true;
+    }
 }
diff --git a/src/Fixer/StringNotation/ExplicitStringVariableFixer.php b/src/Fixer/StringNotation/ExplicitStringVariableFixer.php
@@ -52,11 +52,12 @@ public function getDefinition(): FixerDefinitionInterface
     /**
      * {@inheritdoc}
      *
+     * Must run before NoUselessConcatOperatorFixer.
      * Must run after BacktickToShellExecFixer.
      */
     public function getPriority(): int
     {
-        return 0;
+        return 6;
     }
 
     public function isCandidate(Tokens $tokens): bool

diff --git a/tests/AutoReview/FixerFactoryTest.php b/tests/AutoReview/FixerFactoryTest.php
@@ -442,6 +442,9 @@ private static function getFixersPriorityGraph(): array
                 'heredoc_to_nowdoc',
                 'single_quote',
             ],
+            'explicit_string_variable' => [
+                'no_useless_concat_operator',
+            ],
             'final_class' => [
                 'protected_to_private',
                 'self_static_accessor',

diff --git a/tests/Fixer/Operator/NoUselessConcatOperatorFixerTest.php b/tests/Fixer/Operator/NoUselessConcatOperatorFixerTest.php
@@ -136,6 +136,53 @@ public static function provideFixCases(): iterable
             ',
         ];
 
+        yield 'do not fix if the execution result would be different' => [
+            '<?php
+                echo "abc $d" . "[$e]";
+                echo "abc $d" . "[3]";
+                echo "abc $d" . \'[3]\';
+                echo "abc $d" . "->e";
+                echo "abc $d" . \'->e\';
+                echo "abc $d" . "->$e";
+                echo "abc $d" . "?->e";
+                echo "abc $d" . "?->$e";
+                echo "abc $d" . \'?->e\';
+            ',
+        ];
+
+        yield 'do not fix if variables would be broken' => [
+            '<?php
+                echo "abc $d" . "e $f";
+                echo "abc $d" . "e";
+                echo "abc $d" . \'e\';
+                echo "abc $d" . "😃"; // with emoji
+                echo "私の名前は$name" . "です"; // multibyte characters (Japanese)
+            ',
+        ];
+
+        yield 'fix if variables would not be broken' => [
+            '<?php
+                echo "$a b";
+                echo "$a bcde";
+                echo "abc ${d}e";
+                echo "abc $d-efg";
+                echo "$a bcdef";
+                echo "abc ${d}ef";
+                echo "abc $d-efgh";
+                echo "abc $d-$e";
+            ',
+            '<?php
+                echo "$a" . " b";
+                echo "$a bc" . "de";
+                echo "abc ${d}" . "e";
+                echo "abc $d" . "-efg";
+                echo "$a bc" . \'def\';
+                echo "abc ${d}" . \'ef\';
+                echo "abc $d" . \'-efgh\';
+                echo "abc $d" . "-$e";
+            ',
+        ];
+
         yield 'single quote concat single quote but with line break after' => [
             "<?php \$fh = 'x'. // some comment
 'y';",

diff --git a/tests/Fixtures/Integration/priority/explicit_string_variable,no_useless_concat_operator.test b/tests/Fixtures/Integration/priority/explicit_string_variable,no_useless_concat_operator.test
@@ -0,0 +1,13 @@
+--TEST--
+Integration of fixers: explicit_string_variable,no_useless_concat_operator.
+--RULESET--
+{"no_useless_concat_operator": true, "explicit_string_variable": true}
+--EXPECT--
+<?php
+
+$foo = "bar {$baz}qux";
+
+--INPUT--
+<?php
+
+$foo = "bar $baz" . "qux";