Merge pull request #6653 from cakephp/issue-6568

Fix empty query expressions for generating invalid SQL
cakephp · May 27, 2015 · 74f3fd6 · 74f3fd6
2 parents 3915efe + 0584e60
commit 74f3fd6
Show file tree

Hide file tree

Showing 3 changed files with 78 additions and 2 deletions.
diff --git a/src/Database/Expression/QueryExpression.php b/src/Database/Expression/QueryExpression.php
@@ -403,16 +403,22 @@ public function count()
      */
     public function sql(ValueBinder $generator)
     {
+        $len = $this->count();
+        if ($len === 0) {
+            return '';
+        }
         $conjunction = $this->_conjunction;
-        $template = ($this->count() === 1) ? '%s' : '(%s)';
+        $template = ($len === 1) ? '%s' : '(%s)';
         $parts = [];
         foreach ($this->_conditions as $part) {
             if ($part instanceof Query) {
                 $part = '(' . $part->sql($generator) . ')';
             } elseif ($part instanceof ExpressionInterface) {
                 $part = $part->sql($generator);
             }
-            $parts[] = $part;
+            if (strlen($part)) {
+                $parts[] = $part;
+            }
         }
         return sprintf($template, implode(" $conjunction ", $parts));
     }

diff --git a/tests/TestCase/Database/Expression/QueryExpressionTest.php b/tests/TestCase/Database/Expression/QueryExpressionTest.php
@@ -37,4 +37,56 @@ public function testAndOrCalls()
         $this->assertInstanceOf($expected, $expr->and([]));
         $this->assertInstanceOf($expected, $expr->or([]));
     }
+
+    /**
+     * Test SQL generation with one element
+     *
+     * @return void
+     */
+    public function testSqlGenerationOneClause()
+    {
+        $expr = new QueryExpression();
+        $binder = new ValueBinder();
+        $expr->add(['Users.username' => 'sally'], ['Users.username' => 'string']);
+
+        $result = $expr->sql($binder);
+        $this->assertEquals('Users.username = :c0', $result);
+    }
+
+    /**
+     * Test SQL generation with many elements
+     *
+     * @return void
+     */
+    public function testSqlGenerationMultipleClauses()
+    {
+        $expr = new QueryExpression();
+        $binder = new ValueBinder();
+        $expr->add(
+            [
+                'Users.username' => 'sally',
+                'Users.active' => 1,
+            ],
+            [
+                'Users.username' => 'string',
+                'Users.active' => 'boolean'
+            ]
+        );
+
+        $result = $expr->sql($binder);
+        $this->assertEquals('(Users.username = :c0 AND Users.active = :c1)', $result);
+    }
+
+    /**
+     * Test that empty expressions don't emit invalid SQL.
+     *
+     * @return void
+     */
+    public function testSqlWhenEmpty()
+    {
+        $expr = new QueryExpression();
+        $binder = new ValueBinder();
+        $result = $expr->sql($binder);
+        $this->assertEquals('', $result);
+    }
 }
diff --git a/tests/TestCase/ORM/QueryTest.php b/tests/TestCase/ORM/QueryTest.php
@@ -1861,6 +1861,24 @@ public function testContainWithQueryBuilderJoinableAssociation()
         $this->assertEquals(1, $result[0]->author->id);
     }
 
+    /**
+     * Test containing associations that have empty conditions.
+     *
+     * @return void
+     */
+    public function testContainAssociationWithEmptyConditions()
+    {
+        $articles = TableRegistry::get('Articles');
+        $articles->belongsTo('Authors', [
+            'conditions' => function ($exp, $query) {
+                return $exp;
+            }
+        ]);
+        $query = $articles->find('all')->contain(['Authors']);
+        $result = $query->toArray();
+        $this->assertCount(3, $result);
+    }
+
     /**
      * Tests the formatResults method
      *