Merge pull request #727 from Ocramius/hotfix/#722-disallow-empty-delete-criteria

Disallow empty delete criteria on the connection

lib/Doctrine/DBAL/Connection.php

@@ -20,6 +20,7 @@
 namespace Doctrine\DBAL;
 
 use Doctrine\DBAL\Driver\ServerInfoAwareConnection;
+use Doctrine\DBAL\Exception\InvalidArgumentException;
 use PDO;
 use Closure;
 use Exception;
@@ -570,36 +571,28 @@ class Connection implements DriverConnection
      * @param array  $types      The types of identifiers.
      *
      * @return integer The number of affected rows.
-     */
-    public function delete($tableExpression, array $identifier, array $types = array())
-    {
-        $this->connect();
-
-        return $this->executeUpdate(
-            'DELETE FROM ' . $tableExpression . $this->getWhereSql($identifier),
-            array_values($identifier),
-            is_string(key($types)) ? $this->extractTypeValues($identifier, $types) : $types
-        );
-    }
-
-    /**
-     * @param array $identifier An associative array containing column-value pairs.
      *
-     * @return string
+     * @throws InvalidArgumentException
      */
-    private function getWhereSql(array $identifier)
+    public function delete($tableExpression, array $identifier, array $types = array())
     {
         if (empty($identifier)) {
-            return '';
+            throw InvalidArgumentException::fromEmptyCriteria();
         }
 
+        $this->connect();
+
         $criteria = array();
 
         foreach (array_keys($identifier) as $columnName) {
             $criteria[] = $columnName . ' = ?';
         }
 
-        return ' WHERE ' . implode(' AND ', $criteria);
+        return $this->executeUpdate(
+            'DELETE FROM ' . $tableExpression . ' WHERE ' . implode(' AND ', $criteria),
+            array_values($identifier),
+            is_string(key($types)) ? $this->extractTypeValues($identifier, $types) : $types
+        );
     }
 
     /**

lib/Doctrine/DBAL/Exception/InvalidArgumentException.php

+<?php
+/*
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * This software consists of voluntary contributions made by many individuals
+ * and is licensed under the MIT license. For more information, see
+ * <http://www.doctrine-project.org>.
+ */
+
+namespace Doctrine\DBAL\Exception;
+
+use Doctrine\DBAL\DBALException;
+
+/**
+ * Exception to be thrown when invalid arguments are passed to any DBAL API
+ *
+ * @author Marco Pivetta <ocramius@gmail.com>
+ * @link   www.doctrine-project.org
+ * @since  2.5
+ */
+class InvalidArgumentException extends DBALException
+{
+    /**
+     * @return self
+     */
+    public static function fromEmptyCriteria()
+    {
+        return new self('Empty criteria was used, expected non-empty criteria');
+    }
+}

tests/Doctrine/Tests/DBAL/ConnectionTest.php

@@ -467,19 +467,19 @@ SQLSTATE[HY000]: General error: 1 near \"MUUHAAAAHAAAA\"");
         $this->assertTrue($conn->isConnected(), "Connection is not connected after passing external PDO");
     }
 
-    public function testCallingDeleteWithNoDeletionCriteriaResultsInSqlWithoutWhereClause()
+    public function testCallingDeleteWithNoDeletionCriteriaResultsInInvalidArgumentException()
     {
+        /* @var $driver \Doctrine\DBAL\Driver */
+        $driver  = $this->getMock('Doctrine\DBAL\Driver');
         $pdoMock = $this->getMock('Doctrine\DBAL\Driver\Connection');
 
-        $pdoMock->expects($this->once())
-            ->method('exec')
-            ->with($this->equalTo('DELETE FROM kittens'));
+        // should never execute queries with invalid arguments
+        $pdoMock->expects($this->never())->method('exec');
+        $pdoMock->expects($this->never())->method('prepare');
 
-        $conn = new Connection(
-            array('pdo' => $pdoMock),
-            $this->getMock('Doctrine\DBAL\Driver')
-        );
+        $conn = new Connection(array('pdo' => $pdoMock), $driver);
 
+        $this->setExpectedException('Doctrine\DBAL\Exception\InvalidArgumentException');
         $conn->delete('kittens', array());
     }
 }

tests/Doctrine/Tests/DBAL/Exception/InvalidArgumentExceptionTest.php

+<?php
+/*
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * This software consists of voluntary contributions made by many individuals
+ * and is licensed under the MIT license. For more information, see
+ * <http://www.doctrine-project.org>.
+ */
+
+namespace Doctrine\Tests\DBAL\Exception;
+
+use Doctrine\DBAL\Exception\InvalidArgumentException;
+use PHPUnit_Framework_TestCase;
+
+/**
+ * Tests for {@see \Doctrine\DBAL\Exception\InvalidArgumentException}
+ *
+ * @covers \Doctrine\DBAL\Exception\InvalidArgumentException
+ *
+ * @author Marco Pivetta <ocramius@gmail.com>
+ */
+class InvalidArgumentExceptionTest extends PHPUnit_Framework_TestCase
+{
+    public function testFromEmptyCriteria()
+    {
+        $exception = InvalidArgumentException::fromEmptyCriteria();
+
+        $this->assertInstanceOf('Doctrine\DBAL\Exception\InvalidArgumentException', $exception);
+        $this->assertSame('Empty criteria was used, expected non-empty criteria', $exception->getMessage());
+    }
+}