Minor code cleanup for efficiency, and tests to check the order by cl…

…ause scrubbing is working correctly and not entering an infinite loop
doctrine · Jan 17, 2017 · 6314d55 · 6314d55
1 parent 9eb014a
commit 6314d55
Show file tree

Hide file tree

Showing 2 changed files with 25 additions and 3 deletions.
diff --git a/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php b/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php
@@ -1224,11 +1224,11 @@ protected function doModifyLimitQuery($query, $limit, $offset = null)
      */
     private function scrubInnerOrderBy($query)
     {
-        $count = substr_count(strtoupper($query), "ORDER BY");
+        $count = substr_count(strtoupper($query), 'ORDER BY');
         $offset = 0;
 
         while ($count-- > 0) {
-            $orderByPos = stripos($query, " ORDER BY", $offset);
+            $orderByPos = stripos($query, ' ORDER BY', $offset);
             if ($orderByPos === false) {
                 break;
             }
@@ -1285,7 +1285,7 @@ private function isOrderByInTopNSubquery($query, $currentPosition)
             }
 
             // Only yank query text on the same nesting level as the ORDER BY clause.
-            $subQueryBuffer = ($parenCount === 0 ? $query[$currentPosition] : " ") . $subQueryBuffer;
+            $subQueryBuffer = ($parenCount === 0 ? $query[$currentPosition] : ' ') . $subQueryBuffer;
 
             $currentPosition--;
         }

diff --git a/tests/Doctrine/Tests/DBAL/Platforms/SQLServerPlatformTest.php b/tests/Doctrine/Tests/DBAL/Platforms/SQLServerPlatformTest.php
@@ -7,6 +7,7 @@
 
 class SQLServerPlatformTest extends AbstractSQLServerPlatformTestCase
 {
+
     public function createPlatform()
     {
         return new SQLServerPlatform;
@@ -24,6 +25,15 @@ public function testAppendsLockHint($lockMode, $lockHint)
         $this->assertSame($expectedResult, $this->_platform->appendLockHint($fromClause, $lockMode));
     }
 
+    /**
+     * @group DBAL-2408
+     * @dataProvider getModifyLimitQueries
+     */
+    public function testScrubInnerOrderBy($query, $limit, $offset, $expectedResult)
+    {
+        $this->assertSame($expectedResult, $this->_platform->modifyLimitQuery($query, $limit, $offset));
+    }
+
     public function getLockHints()
     {
         return array(
@@ -36,4 +46,16 @@ public function getLockHints()
             array(LockMode::PESSIMISTIC_WRITE, ' WITH (UPDLOCK, ROWLOCK)'),
         );
     }
+
+    public function getModifyLimitQueries()
+    {
+        return array(
+            // Test re-ordered query with correctly-scrubbed ORDER BY clause
+            array('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, '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 BETWEEN 1 AND 30 ORDER BY doctrine_rownum ASC'),
+
+            // Test re-ordered query with no scrubbed ORDER BY clause
+            array('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, '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 BETWEEN 1 AND 30 ORDER BY doctrine_rownum ASC'),
+        );
+    }
+
 }