Merge pull request doctrine#3491 from morozov/get-trim-expr

Changed the type of $char in AbstractPlatform::getTrimExpression()`from string|false to ?string
morozov · May 31, 2019 · e971ff5 · e971ff5
2 parents dc93277 + f0c7010
commit e971ff5
Show file tree

Hide file tree

Showing 7 changed files with 89 additions and 45 deletions.
diff --git a/UPGRADE.md b/UPGRADE.md
@@ -1,5 +1,9 @@
 # Upgrade to 3.0
 
+## BC BREAK The type of `$char` in `AbstractPlatform::getTrimExpression()` changed from `string|false` to `?string`
+
+The default value of `$char` is now `null`, not `false`. Additionally, the method will throw an `InvalidArgumentException` in an invalid value of `$mode` is passed.
+
 ## BC BREAK `Statement::quote()` only accepts strings.
 
 `Statement::quote()` and `ExpressionBuilder::literal()` no longer accept arguments of an arbitrary type and and don't implement type-specific handling. Only strings can be quoted.

diff --git a/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php b/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php
@@ -753,37 +753,50 @@ public function getModExpression($expression1, $expression2)
      *
      * @param string      $str  The expression to apply the trim to.
      * @param int         $mode The position of the trim (leading/trailing/both).
-     * @param string|bool $char The char to trim, has to be quoted already. Defaults to space.
+     * @param string|null $char The char to trim, has to be quoted already. Defaults to space.
      *
-     * @return string
+     * @throws InvalidArgumentException
      */
-    public function getTrimExpression($str, $mode = TrimMode::UNSPECIFIED, $char = false)
+    public function getTrimExpression(string $str, int $mode = TrimMode::UNSPECIFIED, ?string $char = null) : string
     {
-        $expression = '';
+        $tokens = [];
 
         switch ($mode) {
+            case TrimMode::UNSPECIFIED:
+                break;
+
             case TrimMode::LEADING:
-                $expression = 'LEADING ';
+                $tokens[] = 'LEADING';
                 break;
 
             case TrimMode::TRAILING:
-                $expression = 'TRAILING ';
+                $tokens[] = 'TRAILING';
                 break;
 
             case TrimMode::BOTH:
-                $expression = 'BOTH ';
+                $tokens[] = 'BOTH';
                 break;
+
+            default:
+                throw new InvalidArgumentException(
+                    sprintf(
+                        'The value of $mode is expected to be one of the TrimMode constants, %d given',
+                        $mode
+                    )
+                );
         }
 
-        if ($char !== false) {
-            $expression .= $char . ' ';
+        if ($char !== null) {
+            $tokens[] = $char;
         }
 
-        if ($mode || $char !== false) {
-            $expression .= 'FROM ';
+        if (count($tokens) > 0) {
+            $tokens[] = 'FROM';
         }
 
-        return 'TRIM(' . $expression . $str . ')';
+        $tokens[] = $str;
+
+        return sprintf('TRIM(%s)', implode(' ', $tokens));
     }
 
     /**

diff --git a/lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php b/lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php
@@ -24,6 +24,7 @@
 use function func_get_args;
 use function get_class;
 use function implode;
+use function in_array;
 use function is_string;
 use function preg_match;
 use function sprintf;
@@ -1124,10 +1125,16 @@ public function getTimeTypeDeclarationSQL(array $fieldDeclaration)
     /**
      * {@inheritdoc}
      */
-    public function getTrimExpression($str, $pos = TrimMode::UNSPECIFIED, $char = false)
+    public function getTrimExpression(string $str, int $mode = TrimMode::UNSPECIFIED, ?string $char = null) : string
     {
-        if (! $char) {
-            switch ($pos) {
+        if (! in_array($mode, [TrimMode::UNSPECIFIED, TrimMode::LEADING, TrimMode::TRAILING, TrimMode::BOTH], true)) {
+            throw new InvalidArgumentException(
+                sprintf('The value of $mode is expected to be one of the TrimMode constants, %d given', $mode)
+            );
+        }
+
+        if ($char === null) {
+            switch ($mode) {
                 case TrimMode::LEADING:
                     return $this->getLtrimExpression($str);
                 case TrimMode::TRAILING:
@@ -1139,7 +1146,7 @@ public function getTrimExpression($str, $pos = TrimMode::UNSPECIFIED, $char = fa
 
         $pattern = "'%[^' + " . $char . " + ']%'";
 
-        switch ($pos) {
+        switch ($mode) {
             case TrimMode::LEADING:
                 return 'SUBSTR(' . $str . ', PATINDEX(' . $pattern . ', ' . $str . '))';
             case TrimMode::TRAILING:

diff --git a/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php b/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php
@@ -20,6 +20,7 @@
 use function explode;
 use function func_get_args;
 use function implode;
+use function in_array;
 use function is_array;
 use function is_bool;
 use function is_numeric;
@@ -1030,23 +1031,25 @@ public function getModExpression($expression1, $expression2)
     /**
      * {@inheritDoc}
      */
-    public function getTrimExpression($str, $pos = TrimMode::UNSPECIFIED, $char = false)
+    public function getTrimExpression(string $str, int $mode = TrimMode::UNSPECIFIED, ?string $char = null) : string
     {
-        if (! $char) {
-            switch ($pos) {
+        if (! in_array($mode, [TrimMode::UNSPECIFIED, TrimMode::LEADING, TrimMode::TRAILING, TrimMode::BOTH], true)) {
+            throw new InvalidArgumentException(
+                sprintf('The value of $mode is expected to be one of the TrimMode constants, %d given', $mode)
+            );
+        }
+
+        if ($char === null) {
+            switch ($mode) {
                 case TrimMode::LEADING:
-                    $trimFn = 'LTRIM';
-                    break;
+                    return 'LTRIM(' . $str . ')';
 
                 case TrimMode::TRAILING:
-                    $trimFn = 'RTRIM';
-                    break;
+                    return 'RTRIM(' . $str . ')';
 
                 default:
                     return 'LTRIM(RTRIM(' . $str . '))';
             }
-
-            return $trimFn . '(' . $str . ')';
         }
 
         /** Original query used to get those expressions
@@ -1060,11 +1063,11 @@ public function getTrimExpression($str, $pos = TrimMode::UNSPECIFIED, $char = fa
          */
         $pattern = "'%[^' + " . $char . " + ']%'";
 
-        if ($pos === TrimMode::LEADING) {
+        if ($mode === TrimMode::LEADING) {
             return 'stuff(' . $str . ', 1, patindex(' . $pattern . ', ' . $str . ') - 1, null)';
         }
 
-        if ($pos === TrimMode::TRAILING) {
+        if ($mode === TrimMode::TRAILING) {
             return 'reverse(stuff(reverse(' . $str . '), 1, patindex(' . $pattern . ', reverse(' . $str . ')) - 1, null))';
         }
 

diff --git a/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php b/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php
@@ -12,6 +12,7 @@
 use Doctrine\DBAL\Schema\TableDiff;
 use Doctrine\DBAL\TransactionIsolationLevel;
 use Doctrine\DBAL\Types;
+use InvalidArgumentException;
 use function array_merge;
 use function array_unique;
 use function array_values;
@@ -59,11 +60,14 @@ public function getNowExpression($type = 'timestamp')
     /**
      * {@inheritDoc}
      */
-    public function getTrimExpression($str, $pos = TrimMode::UNSPECIFIED, $char = false)
+    public function getTrimExpression(string $str, int $mode = TrimMode::UNSPECIFIED, ?string $char = null) : string
     {
-        $trimChar = $char !== false ? (', ' . $char) : '';
+        switch ($mode) {
+            case TrimMode::UNSPECIFIED:
+            case TrimMode::BOTH:
+                $trimFn = 'TRIM';
+                break;
 
-        switch ($pos) {
             case TrimMode::LEADING:
                 $trimFn = 'LTRIM';
                 break;
@@ -73,10 +77,21 @@ public function getTrimExpression($str, $pos = TrimMode::UNSPECIFIED, $char = fa
                 break;
 
             default:
-                $trimFn = 'TRIM';
+                throw new InvalidArgumentException(
+                    sprintf(
+                        'The value of $mode is expected to be one of the TrimMode constants, %d given',
+                        $mode
+                    )
+                );
+        }
+
+        $arguments = [$str];
+
+        if ($char !== null) {
+            $arguments[] = $char;
         }
 
-        return $trimFn . '(' . $str . $trimChar . ')';
+        return sprintf('%s(%s)', $trimFn, implode(', ', $arguments));
     }
 
     /**

diff --git a/tests/Doctrine/Tests/DBAL/Functional/DataAccessTest.php b/tests/Doctrine/Tests/DBAL/Functional/DataAccessTest.php
@@ -18,6 +18,7 @@
 use Doctrine\DBAL\Statement;
 use Doctrine\DBAL\Types\Types;
 use Doctrine\Tests\DbalFunctionalTestCase;
+use InvalidArgumentException;
 use PDO;
 use const CASE_LOWER;
 use const PHP_EOL;
@@ -530,10 +531,10 @@ public function testTrimExpression(string $value, int $position, $char, string $
     public static function getTrimExpressionData() : iterable
     {
         return [
-            ['test_string', TrimMode::UNSPECIFIED, false, 'foo'],
-            ['test_string', TrimMode::LEADING, false, 'foo'],
-            ['test_string', TrimMode::TRAILING, false, 'foo'],
-            ['test_string', TrimMode::BOTH, false, 'foo'],
+            ['test_string', TrimMode::UNSPECIFIED, null, 'foo'],
+            ['test_string', TrimMode::LEADING, null, 'foo'],
+            ['test_string', TrimMode::TRAILING, null, 'foo'],
+            ['test_string', TrimMode::BOTH, null, 'foo'],
             ['test_string', TrimMode::UNSPECIFIED, "'f'", 'oo'],
             ['test_string', TrimMode::UNSPECIFIED, "'o'", 'f'],
             ['test_string', TrimMode::UNSPECIFIED, "'.'", 'foo'],
@@ -546,10 +547,10 @@ public static function getTrimExpressionData() : iterable
             ['test_string', TrimMode::BOTH, "'f'", 'oo'],
             ['test_string', TrimMode::BOTH, "'o'", 'f'],
             ['test_string', TrimMode::BOTH, "'.'", 'foo'],
-            ["' foo '", TrimMode::UNSPECIFIED, false, 'foo'],
-            ["' foo '", TrimMode::LEADING, false, 'foo '],
-            ["' foo '", TrimMode::TRAILING, false, ' foo'],
-            ["' foo '", TrimMode::BOTH, false, 'foo'],
+            ["' foo '", TrimMode::UNSPECIFIED, null, 'foo'],
+            ["' foo '", TrimMode::LEADING, null, 'foo '],
+            ["' foo '", TrimMode::TRAILING, null, ' foo'],
+            ["' foo '", TrimMode::BOTH, null, 'foo'],
             ["' foo '", TrimMode::UNSPECIFIED, "'f'", ' foo '],
             ["' foo '", TrimMode::UNSPECIFIED, "'o'", ' foo '],
             ["' foo '", TrimMode::UNSPECIFIED, "'.'", ' foo '],
@@ -569,6 +570,12 @@ public static function getTrimExpressionData() : iterable
         ];
     }
 
+    public function testTrimExpressionInvalidMode() : void
+    {
+        $this->expectException(InvalidArgumentException::class);
+        $this->connection->getDatabasePlatform()->getTrimExpression('Trim me!', 0xBEEF);
+    }
+
     /**
      * @group DDC-1014
      */

diff --git a/tests/Doctrine/Tests/DBAL/Platforms/SQLAnywherePlatformTest.php b/tests/Doctrine/Tests/DBAL/Platforms/SQLAnywherePlatformTest.php
@@ -697,11 +697,6 @@ public function testGeneratesSQLSnippets() : void
             "REVERSE(SUBSTR(REVERSE(column), PATINDEX('%[^' + c + ']%', REVERSE(column))))",
             $this->platform->getTrimExpression('column', TrimMode::TRAILING, 'c')
         );
-        self::assertEquals(
-            "REVERSE(SUBSTR(REVERSE(SUBSTR(column, PATINDEX('%[^' + c + ']%', column))), PATINDEX('%[^' + c + ']%', " .
-            "REVERSE(SUBSTR(column, PATINDEX('%[^' + c + ']%', column))))))",
-            $this->platform->getTrimExpression('column', null, 'c')
-        );
         self::assertEquals(
             "REVERSE(SUBSTR(REVERSE(SUBSTR(column, PATINDEX('%[^' + c + ']%', column))), PATINDEX('%[^' + c + ']%', " .
             "REVERSE(SUBSTR(column, PATINDEX('%[^' + c + ']%', column))))))",