Merge pull request #3494 from morozov/get-locate-expr

Modified AbstractPlatform::getLocateExpression() and ::getSubstringExpression() signatures

UPGRADE.md

 # Upgrade to 3.0
 
+## BC BREAK The type of `$start` in `AbstractPlatform::getLocateExpression()` changed from `string|false` to `?string`
+
+The default value of `$start` is now `null`, not `false`.
+
+## BC BREAK The types of `$start` and `$length` in `AbstractPlatform::getSubstringExpression()` changed from `int` and `?int` to `string` and `?string` respectively
+
+The platform abstraction allows building arbitrary SQL expressions, so even if the arguments represent numeric literals, they should be passed as a string.
+
 ## 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.

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

@@ -850,17 +850,16 @@ abstract class AbstractPlatform
     }
 
     /**
-     * Returns the SQL snippet to get the position of the first occurrence of substring $substr in string $str.
+     * Returns the SQL snippet to get the position of the first occurrence of the substring in the string.
      *
-     * @param string    $str      Literal string.
-     * @param string    $substr   Literal string to find.
-     * @param int|false $startPos Position to start at, beginning of string by default.
-     *
-     * @return string
+     * @param string      $string    SQL expression producing the string to locate the substring in.
+     * @param string      $substring SQL expression producing the substring to locate.
+     * @param string|null $start     SQL expression producing the position to start at.
+     *                               Defaults to the beginning of the string.
      *
      * @throws DBALException If not supported on this platform.
      */
-    public function getLocateExpression($str, $substr, $startPos = false)
+    public function getLocateExpression(string $string, string $substring, ?string $start = null) : string
     {
         throw DBALException::notSupported(__METHOD__);
     }
@@ -876,25 +875,22 @@ abstract class AbstractPlatform
     }
 
     /**
-     * Returns a SQL snippet to get a substring inside an SQL statement.
+     * Returns an SQL snippet to get a substring inside the string.
      *
      * Note: Not SQL92, but common functionality.
      *
-     * SQLite only supports the 2 parameter variant of this function.
-     *
-     * @param string   $value  An sql string literal or column name/alias.
-     * @param int      $from   Where to start the substring portion.
-     * @param int|null $length The substring portion length.
-     *
-     * @return string
+     * @param string      $string SQL expression producing the string from which a substring should be extracted.
+     * @param string      $start  SQL expression producing the position to start at,
+     * @param string|null $length SQL expression producing the length of the substring portion to be returned.
+     *                            By default, the entire substring is returned.
      */
-    public function getSubstringExpression($value, $from, $length = null)
+    public function getSubstringExpression(string $string, string $start, ?string $length = null) : string
     {
         if ($length === null) {
-            return 'SUBSTRING(' . $value . ' FROM ' . $from . ')';
+            return sprintf('SUBSTRING(%s FROM %s)', $string, $start);
         }
 
-        return 'SUBSTRING(' . $value . ' FROM ' . $from . ' FOR ' . $length . ')';
+        return sprintf('SUBSTRING(%s FROM %s FOR %s)', $string, $start, $length);
     }
 
     /**

lib/Doctrine/DBAL/Platforms/DB2Platform.php

@@ -814,25 +814,25 @@ class DB2Platform extends AbstractPlatform
     /**
      * {@inheritDoc}
      */
-    public function getLocateExpression($str, $substr, $startPos = false)
+    public function getLocateExpression(string $string, string $substring, ?string $start = null) : string
     {
-        if ($startPos === false) {
-            return 'LOCATE(' . $substr . ', ' . $str . ')';
+        if ($start === null) {
+            return sprintf('LOCATE(%s, %s)', $substring, $string);
         }
 
-        return 'LOCATE(' . $substr . ', ' . $str . ', ' . $startPos . ')';
+        return sprintf('LOCATE(%s, %s, %s)', $substring, $string, $start);
     }
 
     /**
      * {@inheritDoc}
      */
-    public function getSubstringExpression($value, $from, $length = null)
+    public function getSubstringExpression(string $string, string $start, ?string $length = null) : string
     {
         if ($length === null) {
-            return 'SUBSTR(' . $value . ', ' . $from . ')';
+            return sprintf('SUBSTR(%s, %s)', $string, $start);
         }
 
-        return 'SUBSTR(' . $value . ', ' . $from . ', ' . $length . ')';
+        return sprintf('SUBSTR(%s, %s, %s)', $string, $start, $length);
     }
 
     /**

lib/Doctrine/DBAL/Platforms/MySqlPlatform.php

@@ -81,13 +81,13 @@ class MySqlPlatform extends AbstractPlatform
     /**
      * {@inheritDoc}
      */
-    public function getLocateExpression($str, $substr, $startPos = false)
+    public function getLocateExpression(string $string, string $substring, ?string $start = null) : string
     {
-        if ($startPos === false) {
-            return 'LOCATE(' . $substr . ', ' . $str . ')';
+        if ($start === null) {
+            return sprintf('LOCATE(%s, %s)', $substring, $string);
         }
 
-        return 'LOCATE(' . $substr . ', ' . $str . ', ' . $startPos . ')';
+        return sprintf('LOCATE(%s, %s, %s)', $substring, $string, $start);
     }
 
     /**

lib/Doctrine/DBAL/Platforms/OraclePlatform.php

@@ -47,13 +47,13 @@ class OraclePlatform extends AbstractPlatform
     /**
      * {@inheritDoc}
      */
-    public function getSubstringExpression($value, $position, $length = null)
+    public function getSubstringExpression(string $string, string $start, ?string $length = null) : string
     {
-        if ($length !== null) {
-            return sprintf('SUBSTR(%s, %d, %d)', $value, $position, $length);
+        if ($length === null) {
+            return sprintf('SUBSTR(%s, %s)', $string, $start);
         }
 
-        return sprintf('SUBSTR(%s, %d)', $value, $position);
+        return sprintf('SUBSTR(%s, %s, %s)', $string, $start, $length);
     }
 
     /**
@@ -73,13 +73,13 @@ class OraclePlatform extends AbstractPlatform
     /**
      * {@inheritDoc}
      */
-    public function getLocateExpression($str, $substr, $startPos = false)
+    public function getLocateExpression(string $string, string $substring, ?string $start = null) : string
     {
-        if ($startPos === false) {
-            return 'INSTR(' . $str . ', ' . $substr . ')';
+        if ($start === null) {
+            return sprintf('INSTR(%s, %s)', $string, $substring);
         }
 
-        return 'INSTR(' . $str . ', ' . $substr . ', ' . $startPos . ')';
+        return sprintf('INSTR(%s, %s, %s)', $string, $substring, $start);
     }
 
     /**

lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php

@@ -75,18 +75,6 @@ class PostgreSqlPlatform extends AbstractPlatform
         $this->useBooleanTrueFalseStrings = (bool) $flag;
     }
 
-    /**
-     * {@inheritDoc}
-     */
-    public function getSubstringExpression($value, $from, $length = null)
-    {
-        if ($length === null) {
-            return 'SUBSTRING(' . $value . ' FROM ' . $from . ')';
-        }
-
-        return 'SUBSTRING(' . $value . ' FROM ' . $from . ' FOR ' . $length . ')';
-    }
-
     /**
      * {@inheritDoc}
      */
@@ -106,15 +94,15 @@ class PostgreSqlPlatform extends AbstractPlatform
     /**
      * {@inheritDoc}
      */
-    public function getLocateExpression($str, $substr, $startPos = false)
+    public function getLocateExpression(string $string, string $substring, ?string $start = null) : string
     {
-        if ($startPos !== false) {
-            $str = $this->getSubstringExpression($str, $startPos);
+        if ($start !== null) {
+            $string = $this->getSubstringExpression($string, $start);
 
-            return 'CASE WHEN (POSITION(' . $substr . ' IN ' . $str . ') = 0) THEN 0 ELSE (POSITION(' . $substr . ' IN ' . $str . ') + ' . ($startPos-1) . ') END';
+            return 'CASE WHEN (POSITION(' . $substring . ' IN ' . $string . ') = 0) THEN 0 ELSE (POSITION(' . $substring . ' IN ' . $string . ') + ' . $start . ' - 1) END';
         }
 
-        return 'POSITION(' . $substr . ' IN ' . $str . ')';
+        return sprintf('POSITION(%s IN %s)', $substring, $string);
     }
 
     /**

lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php

@@ -966,13 +966,13 @@ SQL
     /**
      * {@inheritdoc}
      */
-    public function getLocateExpression($str, $substr, $startPos = false)
+    public function getLocateExpression(string $string, string $substring, ?string $start = null) : string
     {
-        if ($startPos === false) {
-            return 'LOCATE(' . $str . ', ' . $substr . ')';
+        if ($start === null) {
+            return sprintf('LOCATE(%s, %s)', $string, $substring);
         }
 
-        return 'LOCATE(' . $str . ', ' . $substr . ', ' . $startPos . ')';
+        return sprintf('LOCATE(%s, %s, %s)', $string, $substring, $start);
     }
 
     /**
@@ -1089,13 +1089,13 @@ SQL
     /**
      * {@inheritdoc}
      */
-    public function getSubstringExpression($value, $from, $length = null)
+    public function getSubstringExpression(string $string, string $start, ?string $length = null) : string
     {
         if ($length === null) {
-            return 'SUBSTRING(' . $value . ', ' . $from . ')';
+            return sprintf('SUBSTRING(%s, %s)', $string, $start);
         }
 
-        return 'SUBSTRING(' . $value . ', ' . $from . ', ' . $length . ')';
+        return sprintf('SUBSTRING(%s, %s, %s)', $string, $start, $length);
     }
 
     /**

lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php

@@ -1016,13 +1016,13 @@ SQL
     /**
      * {@inheritDoc}
      */
-    public function getLocateExpression($str, $substr, $startPos = false)
+    public function getLocateExpression(string $string, string $substring, ?string $start = null) : string
     {
-        if ($startPos === false) {
-            return 'CHARINDEX(' . $substr . ', ' . $str . ')';
+        if ($start === null) {
+            return sprintf('CHARINDEX(%s, %s)', $substring, $string);
         }
 
-        return 'CHARINDEX(' . $substr . ', ' . $str . ', ' . $startPos . ')';
+        return sprintf('CHARINDEX(%s, %s, %s)', $substring, $string, $start);
     }
 
     /**
@@ -1108,13 +1108,13 @@ SQL
     /**
      * {@inheritDoc}
      */
-    public function getSubstringExpression($value, $from, $length = null)
+    public function getSubstringExpression(string $string, string $start, ?string $length = null) : string
     {
-        if ($length !== null) {
-            return 'SUBSTRING(' . $value . ', ' . $from . ', ' . $length . ')';
+        if ($length === null) {
+            return sprintf('SUBSTRING(%s, %s, LEN(%s) - %s + 1)', $string, $start, $string, $start);
         }
 
-        return 'SUBSTRING(' . $value . ', ' . $from . ', LEN(' . $value . ') - ' . $from . ' + 1)';
+        return sprintf('SUBSTRING(%s, %s, %s)', $string, $start, $length);
     }
 
     /**

lib/Doctrine/DBAL/Platforms/SqlitePlatform.php

@@ -97,28 +97,26 @@ class SqlitePlatform extends AbstractPlatform
 
     /**
      * {@inheritDoc}
-     *
-     * SQLite only supports the 2 parameter variant of this function
      */
-    public function getSubstringExpression($value, $position, $length = null)
+    public function getSubstringExpression(string $string, string $start, ?string $length = null) : string
     {
-        if ($length !== null) {
-            return 'SUBSTR(' . $value . ', ' . $position . ', ' . $length . ')';
+        if ($length === null) {
+            return sprintf('SUBSTR(%s, %s)', $string, $start);
         }
 
-        return 'SUBSTR(' . $value . ', ' . $position . ', LENGTH(' . $value . '))';
+        return sprintf('SUBSTR(%s, %s, %s)', $string, $start, $length);
     }
 
     /**
      * {@inheritDoc}
      */
-    public function getLocateExpression($str, $substr, $startPos = false)
+    public function getLocateExpression(string $string, string $substring, ?string $start = null) : string
     {
-        if ($startPos === false) {
-            return 'LOCATE(' . $str . ', ' . $substr . ')';
+        if ($start === null) {
+            return sprintf('LOCATE(%s, %s)', $string, $substring);
         }
 
-        return 'LOCATE(' . $str . ', ' . $substr . ', ' . $startPos . ')';
+        return sprintf('LOCATE(%s, %s, %s)', $string, $substring, $start);
     }
 
     /**

tests/Doctrine/Tests/DBAL/Functional/DataAccessTest.php

@@ -679,6 +679,47 @@ class DataAccessTest extends DbalFunctionalTestCase
         self::assertEquals(0, $row['locate9']);
     }
 
+    /**
+     * @dataProvider substringExpressionProvider
+     */
+    public function testSubstringExpression(string $string, string $start, ?string $length, string $expected) : void
+    {
+        $platform = $this->connection->getDatabasePlatform();
+
+        $query = $platform->getDummySelectSQL(
+            $platform->getSubstringExpression($string, $start, $length)
+        );
+
+        $this->assertEquals($expected, $this->connection->fetchColumn($query));
+    }
+
+    /**
+     * @return mixed[][]
+     */
+    public static function substringExpressionProvider() : iterable
+    {
+        return [
+            'start-no-length' => [
+                "'abcdef'",
+                '3',
+                null,
+                'cdef',
+            ],
+            'start-with-length' => [
+                "'abcdef'",
+                '2',
+                '4',
+                'bcde',
+            ],
+            'expressions' => [
+                "'abcdef'",
+                '1 + 1',
+                '1 + 1',
+                'bc',
+            ],
+        ];
+    }
+
     public function testQuoteSQLInjection() : void
     {
         $sql  = 'SELECT * FROM fetch_table WHERE test_string = ' . $this->connection->quote("bar' OR '1'='1");

tests/Doctrine/Tests/DBAL/Platforms/DB2PlatformTest.php

@@ -363,7 +363,6 @@ class DB2PlatformTest extends AbstractPlatformTestCase
         self::assertEquals("'1987/05/02' - 10 YEAR", $this->platform->getDateSubYearsExpression("'1987/05/02'", 10));
         self::assertEquals(' WITH RR USE AND KEEP UPDATE LOCKS', $this->platform->getForUpdateSQL());
         self::assertEquals('LOCATE(substring_column, string_column)', $this->platform->getLocateExpression('string_column', 'substring_column'));
-        self::assertEquals('LOCATE(substring_column, string_column)', $this->platform->getLocateExpression('string_column', 'substring_column'));
         self::assertEquals('LOCATE(substring_column, string_column, 1)', $this->platform->getLocateExpression('string_column', 'substring_column', 1));
         self::assertEquals('SUBSTR(column, 5)', $this->platform->getSubstringExpression('column', 5));
         self::assertEquals('SUBSTR(column, 5, 2)', $this->platform->getSubstringExpression('column', 5, 2));

tests/Doctrine/Tests/DBAL/Platforms/SqlitePlatformTest.php

@@ -37,7 +37,7 @@ class SqlitePlatformTest extends AbstractPlatformTestCase
     public function testGeneratesSqlSnippets() : void
     {
         self::assertEquals('REGEXP', $this->platform->getRegexpExpression(), 'Regular expression operator is not correct');
-        self::assertEquals('SUBSTR(column, 5, LENGTH(column))', $this->platform->getSubstringExpression('column', 5), 'Substring expression without length is not correct');
+        self::assertEquals('SUBSTR(column, 5)', $this->platform->getSubstringExpression('column', 5), 'Substring expression without length is not correct');
         self::assertEquals('SUBSTR(column, 0, 5)', $this->platform->getSubstringExpression('column', 0, 5), 'Substring expression with length is not correct');
     }