Merge pull request #3248 from morozov/non-nullable-offset

Made the OFFSET in LIMIT queries non-nullable integer defaulting to 0
doctrine · Nov 14, 2018 · 1201311 · 1201311
2 parents e9e1d63 + 9757beb
commit 1201311
Show file tree

Hide file tree

Showing 12 changed files with 25 additions and 42 deletions.
diff --git a/UPGRADE.md b/UPGRADE.md
@@ -1,5 +1,9 @@
 # Upgrade to 3.0
 
+## BC BREAK: The `NULL` value of `$offset` in LIMIT queries is not allowed
+
+The `NULL` value of the `$offset` argument in `AbstractPlatform::(do)?ModifyLimitQuery()` methods is no longer allowed. The absence of the offset should be indicated with a `0` which is now the default value.
+
 ## BC BREAK: Removed dbal:import CLI command
 
 The `dbal:import` CLI command has been removed since it only worked with PDO-based drivers by relying on a non-documented behavior of the extension, and it was impossible to make it work with other drivers.

diff --git a/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php b/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php
@@ -3340,22 +3340,10 @@ public function getTimeFormatString()
     /**
      * Adds an driver-specific LIMIT clause to the query.
      *
-     * @param string   $query
-     * @param int|null $limit
-     * @param int|null $offset
-     *
-     * @return string
-     *
      * @throws DBALException
      */
-    final public function modifyLimitQuery($query, $limit, $offset = null)
+    final public function modifyLimitQuery(string $query, ?int $limit, int $offset = 0) : string
     {
-        if ($limit !== null) {
-            $limit = (int) $limit;
-        }
-
-        $offset = (int) $offset;
-
         if ($offset < 0) {
             throw new DBALException(sprintf(
                 'Offset must be a positive integer or zero, %d given',
@@ -3375,21 +3363,15 @@ final public function modifyLimitQuery($query, $limit, $offset = null)
 
     /**
      * Adds an platform-specific LIMIT clause to the query.
-     *
-     * @param string   $query
-     * @param int|null $limit
-     * @param int|null $offset
-     *
-     * @return string
      */
-    protected function doModifyLimitQuery($query, $limit, $offset)
+    protected function doModifyLimitQuery(string $query, ?int $limit, int $offset) : string
     {
         if ($limit !== null) {
-            $query .= ' LIMIT ' . $limit;
+            $query .= sprintf(' LIMIT %d', $limit);
         }
 
         if ($offset > 0) {
-            $query .= ' OFFSET ' . $offset;
+            $query .= sprintf(' OFFSET %d', $offset);
         }
 
         return $query;

diff --git a/lib/Doctrine/DBAL/Platforms/DB2Platform.php b/lib/Doctrine/DBAL/Platforms/DB2Platform.php
@@ -782,7 +782,7 @@ public function getTemporaryTableName($tableName)
     /**
      * {@inheritDoc}
      */
-    protected function doModifyLimitQuery($query, $limit, $offset = null)
+    protected function doModifyLimitQuery(string $query, ?int $limit, int $offset) : string
     {
         $where = [];
 

diff --git a/lib/Doctrine/DBAL/Platforms/MySqlPlatform.php b/lib/Doctrine/DBAL/Platforms/MySqlPlatform.php
@@ -47,7 +47,7 @@ class MySqlPlatform extends AbstractPlatform
     /**
      * {@inheritDoc}
      */
-    protected function doModifyLimitQuery($query, $limit, $offset)
+    protected function doModifyLimitQuery(string $query, ?int $limit, int $offset) : string
     {
         if ($limit !== null) {
             $query .= ' LIMIT ' . $limit;

diff --git a/lib/Doctrine/DBAL/Platforms/OraclePlatform.php b/lib/Doctrine/DBAL/Platforms/OraclePlatform.php
@@ -971,7 +971,7 @@ public function getName()
     /**
      * {@inheritDoc}
      */
-    protected function doModifyLimitQuery($query, $limit, $offset = null)
+    protected function doModifyLimitQuery(string $query, ?int $limit, int $offset) : string
     {
         if ($limit === null && $offset <= 0) {
             return $query;

diff --git a/lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php b/lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php
@@ -1352,16 +1352,16 @@ protected function _getTransactionIsolationLevelSQL($level)
     /**
      * {@inheritdoc}
      */
-    protected function doModifyLimitQuery($query, $limit, $offset)
+    protected function doModifyLimitQuery(string $query, ?int $limit, int $offset) : string
     {
         $limitOffsetClause = '';
 
-        if ($limit > 0) {
+        if ($limit !== null) {
             $limitOffsetClause = 'TOP ' . $limit . ' ';
         }
 
         if ($offset > 0) {
-            if ($limit === 0) {
+            if ($limit === null) {
                 $limitOffsetClause = 'TOP ALL ';
             }
 

diff --git a/lib/Doctrine/DBAL/Platforms/SQLServer2012Platform.php b/lib/Doctrine/DBAL/Platforms/SQLServer2012Platform.php
@@ -6,6 +6,7 @@
 use const PREG_OFFSET_CAPTURE;
 use function preg_match;
 use function preg_match_all;
+use function sprintf;
 use function substr_count;
 
 /**
@@ -92,7 +93,7 @@ protected function getReservedKeywordsClass()
     /**
      * {@inheritdoc}
      */
-    protected function doModifyLimitQuery($query, $limit, $offset = null)
+    protected function doModifyLimitQuery(string $query, ?int $limit, int $offset) : string
     {
         if ($limit === null && $offset <= 0) {
             return $query;
@@ -125,17 +126,13 @@ protected function doModifyLimitQuery($query, $limit, $offset = null)
             }
         }
 
-        if ($offset === null) {
-            $offset = 0;
-        }
-
         // This looks somewhat like MYSQL, but limit/offset are in inverse positions
         // Supposedly SQL:2008 core standard.
         // Per TSQL spec, FETCH NEXT n ROWS ONLY is not valid without OFFSET n ROWS.
-        $query .= ' OFFSET ' . (int) $offset . ' ROWS';
+        $query .= sprintf(' OFFSET %d ROWS', $offset);
 
         if ($limit !== null) {
-            $query .= ' FETCH NEXT ' . (int) $limit . ' ROWS ONLY';
+            $query .= sprintf(' FETCH NEXT %d ROWS ONLY', $limit);
         }
 
         return $query;

diff --git a/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php b/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php
@@ -1237,7 +1237,7 @@ public function getBooleanTypeDeclarationSQL(array $field)
     /**
      * {@inheritDoc}
      */
-    protected function doModifyLimitQuery($query, $limit, $offset = null)
+    protected function doModifyLimitQuery(string $query, ?int $limit, int $offset) : string
     {
         $where = [];
 

diff --git a/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php b/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php
@@ -689,10 +689,10 @@ protected function getPostAlterTableIndexForeignKeySQL(TableDiff $diff)
     /**
      * {@inheritDoc}
      */
-    protected function doModifyLimitQuery($query, $limit, $offset)
+    protected function doModifyLimitQuery(string $query, ?int $limit, int $offset) : string
     {
         if ($limit === null && $offset > 0) {
-            return $query . ' LIMIT -1 OFFSET ' . $offset;
+            $limit = -1;
         }
 
         return parent::doModifyLimitQuery($query, $limit, $offset);

diff --git a/tests/Doctrine/Tests/DBAL/Platforms/DB2PlatformTest.php b/tests/Doctrine/Tests/DBAL/Platforms/DB2PlatformTest.php
@@ -342,7 +342,7 @@ public function testModifiesLimitQuery()
     {
         self::assertEquals(
             'SELECT * FROM user',
-            $this->platform->modifyLimitQuery('SELECT * FROM user', null, null)
+            $this->platform->modifyLimitQuery('SELECT * FROM user', null, 0)
         );
 
         self::assertEquals(

diff --git a/tests/Doctrine/Tests/DBAL/Platforms/SQLAnywherePlatformTest.php b/tests/Doctrine/Tests/DBAL/Platforms/SQLAnywherePlatformTest.php
@@ -760,7 +760,7 @@ public function testModifiesLimitQueryWithOffset()
         );
         self::assertEquals(
             'SELECT TOP ALL START AT 6 * FROM user',
-            $this->platform->modifyLimitQuery('SELECT * FROM user', 0, 5)
+            $this->platform->modifyLimitQuery('SELECT * FROM user', null, 5)
         );
     }
 

diff --git a/tests/Doctrine/Tests/DBAL/Platforms/SQLServerPlatformTest.php b/tests/Doctrine/Tests/DBAL/Platforms/SQLServerPlatformTest.php
@@ -53,15 +53,15 @@ public function getModifyLimitQueries()
             [
                 'SELECT id_0, MIN(sclr_2) AS dctrn_minrownum FROM (SELECT c0_.id AS id_0, c0_.title AS title_1, ROW_NUMBER() OVER(ORDER BY c0_.title ASC) AS sclr_2 FROM TestTable c0_ ORDER BY c0_.title ASC) dctrn_result GROUP BY id_0 ORDER BY dctrn_minrownum ASC',
                 30,
-                null,
+                0,
                 'WITH dctrn_cte AS (SELECT TOP 30 id_0, MIN(sclr_2) AS dctrn_minrownum FROM (SELECT c0_.id AS id_0, c0_.title AS title_1, ROW_NUMBER() OVER(ORDER BY c0_.title ASC) AS sclr_2 FROM TestTable c0_) dctrn_result GROUP BY id_0 ORDER BY dctrn_minrownum ASC) SELECT * FROM (SELECT *, ROW_NUMBER() OVER (ORDER BY (SELECT 0)) AS doctrine_rownum FROM dctrn_cte) AS doctrine_tbl WHERE doctrine_rownum <= 30 ORDER BY doctrine_rownum ASC',
             ],
 
             // Test re-ordered query with no scrubbed ORDER BY clause
             [
                 'SELECT id_0, MIN(sclr_2) AS dctrn_minrownum FROM (SELECT c0_.id AS id_0, c0_.title AS title_1, ROW_NUMBER() OVER(ORDER BY c0_.title ASC) AS sclr_2 FROM TestTable c0_) dctrn_result GROUP BY id_0 ORDER BY dctrn_minrownum ASC',
                 30,
-                null,
+                0,
                 'WITH dctrn_cte AS (SELECT TOP 30 id_0, MIN(sclr_2) AS dctrn_minrownum FROM (SELECT c0_.id AS id_0, c0_.title AS title_1, ROW_NUMBER() OVER(ORDER BY c0_.title ASC) AS sclr_2 FROM TestTable c0_) dctrn_result GROUP BY id_0 ORDER BY dctrn_minrownum ASC) SELECT * FROM (SELECT *, ROW_NUMBER() OVER (ORDER BY (SELECT 0)) AS doctrine_rownum FROM dctrn_cte) AS doctrine_tbl WHERE doctrine_rownum <= 30 ORDER BY doctrine_rownum ASC',
             ],
         ];