From e61102ef97e3c017367566935d38d0b7e4cb8fcc Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Sun, 17 Mar 2019 21:46:56 -0700 Subject: [PATCH] Modified AbstractPlatform::getLocateExpression() and ::getSubstringExpression() signatures --- UPGRADE.md | 8 ++++ .../DBAL/Platforms/AbstractPlatform.php | 32 +++++++-------- lib/Doctrine/DBAL/Platforms/DB2Platform.php | 14 +++---- lib/Doctrine/DBAL/Platforms/MySqlPlatform.php | 8 ++-- .../DBAL/Platforms/OraclePlatform.php | 16 ++++---- .../DBAL/Platforms/PostgreSqlPlatform.php | 22 +++------- .../DBAL/Platforms/SQLAnywherePlatform.php | 14 +++---- .../DBAL/Platforms/SQLServerPlatform.php | 16 ++++---- .../DBAL/Platforms/SqlitePlatform.php | 18 ++++---- .../Tests/DBAL/Functional/DataAccessTest.php | 41 +++++++++++++++++++ .../Tests/DBAL/Platforms/DB2PlatformTest.php | 1 - .../DBAL/Platforms/SqlitePlatformTest.php | 2 +- 12 files changed, 111 insertions(+), 81 deletions(-) diff --git a/UPGRADE.md b/UPGRADE.md index ad0b57c91e1..097e5abcfac 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -1,5 +1,13 @@ # 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. diff --git a/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php b/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php index 0383f561424..fb079b7f2a2 100644 --- a/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php +++ b/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php @@ -850,17 +850,16 @@ public function getLowerExpression($str) } /** - * 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 @@ public function getNowExpression() } /** - * 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); } /** diff --git a/lib/Doctrine/DBAL/Platforms/DB2Platform.php b/lib/Doctrine/DBAL/Platforms/DB2Platform.php index 0555b8754d3..36317b589ed 100644 --- a/lib/Doctrine/DBAL/Platforms/DB2Platform.php +++ b/lib/Doctrine/DBAL/Platforms/DB2Platform.php @@ -814,25 +814,25 @@ protected function doModifyLimitQuery(string $query, ?int $limit, int $offset) : /** * {@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); } /** diff --git a/lib/Doctrine/DBAL/Platforms/MySqlPlatform.php b/lib/Doctrine/DBAL/Platforms/MySqlPlatform.php index 6c5fb2e92dd..c9d814b6cce 100644 --- a/lib/Doctrine/DBAL/Platforms/MySqlPlatform.php +++ b/lib/Doctrine/DBAL/Platforms/MySqlPlatform.php @@ -81,13 +81,13 @@ public function getRegexpExpression() /** * {@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); } /** diff --git a/lib/Doctrine/DBAL/Platforms/OraclePlatform.php b/lib/Doctrine/DBAL/Platforms/OraclePlatform.php index f621f454f6c..28db38583e5 100644 --- a/lib/Doctrine/DBAL/Platforms/OraclePlatform.php +++ b/lib/Doctrine/DBAL/Platforms/OraclePlatform.php @@ -47,13 +47,13 @@ public static function assertValidIdentifier($identifier) /** * {@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 @@ public function getNowExpression($type = 'timestamp') /** * {@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); } /** diff --git a/lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php b/lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php index f5603c5928d..47f80579ff2 100644 --- a/lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php +++ b/lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php @@ -75,18 +75,6 @@ public function setUseBooleanTrueFalseStrings($flag) $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 @@ public function getRegexpExpression() /** * {@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); } /** diff --git a/lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php b/lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php index 794fdf33a67..91f2334fe8a 100644 --- a/lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php +++ b/lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php @@ -966,13 +966,13 @@ public function getListViewsSQL($database) /** * {@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 @@ public function getStopDatabaseSQL($database) /** * {@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); } /** diff --git a/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php b/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php index 8e7f87b94c7..3fedc7f68ee 100644 --- a/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php +++ b/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php @@ -1011,13 +1011,13 @@ public function getDropViewSQL($name) /** * {@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); } /** @@ -1103,13 +1103,13 @@ public function getListNamespacesSQL() /** * {@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); } /** diff --git a/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php b/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php index 848f3a7f3fc..397cff35692 100644 --- a/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php +++ b/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php @@ -96,28 +96,26 @@ public function getTrimExpression(string $str, int $mode = TrimMode::UNSPECIFIED /** * {@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); } /** diff --git a/tests/Doctrine/Tests/DBAL/Functional/DataAccessTest.php b/tests/Doctrine/Tests/DBAL/Functional/DataAccessTest.php index 507fad81ce3..a6d3a425621 100644 --- a/tests/Doctrine/Tests/DBAL/Functional/DataAccessTest.php +++ b/tests/Doctrine/Tests/DBAL/Functional/DataAccessTest.php @@ -679,6 +679,47 @@ public function testLocateExpression() : void 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"); diff --git a/tests/Doctrine/Tests/DBAL/Platforms/DB2PlatformTest.php b/tests/Doctrine/Tests/DBAL/Platforms/DB2PlatformTest.php index 36ef0d6b2da..d0ba92df563 100644 --- a/tests/Doctrine/Tests/DBAL/Platforms/DB2PlatformTest.php +++ b/tests/Doctrine/Tests/DBAL/Platforms/DB2PlatformTest.php @@ -363,7 +363,6 @@ public function testGeneratesSQLSnippets() : void 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)); diff --git a/tests/Doctrine/Tests/DBAL/Platforms/SqlitePlatformTest.php b/tests/Doctrine/Tests/DBAL/Platforms/SqlitePlatformTest.php index d645b218cb8..2cf891ce97d 100644 --- a/tests/Doctrine/Tests/DBAL/Platforms/SqlitePlatformTest.php +++ b/tests/Doctrine/Tests/DBAL/Platforms/SqlitePlatformTest.php @@ -37,7 +37,7 @@ public function getGenerateTableWithMultiColumnUniqueIndexSql() : array 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'); }