Merge pull request #3858 from BenMorel/master-immutable

Make CompositeExpression immutable
doctrine · Jan 30, 2020 · 28fa940 · 28fa940
2 parents 43819af + e8497cf
commit 28fa940
Show file tree

Hide file tree

Showing 5 changed files with 16 additions and 67 deletions.
diff --git a/UPGRADE.md b/UPGRADE.md
@@ -1,5 +1,10 @@
 # Upgrade to 3.0
 
+## BC BREAK: Removed `CompositeExpression` methods
+
+The `add()` and `addMultiple()` methods of the `CompositeExpression` class have been removed. Use `with()` instead, which returns a new instance.
+The `CompositeExpression` class is now immutable.
+
 ## BC BREAK: Changes in the QueryBuilder API.
 
 1. The `select()`, `addSelect()`, `groupBy()` and `addGroupBy()` methods no longer accept an array of arguments. Pass each expression as an individual argument or expand an array of expressions using the `...` operator.

diff --git a/lib/Doctrine/DBAL/Query/Expression/CompositeExpression.php b/lib/Doctrine/DBAL/Query/Expression/CompositeExpression.php
@@ -5,11 +5,15 @@
 namespace Doctrine\DBAL\Query\Expression;
 
 use Countable;
+use function array_filter;
+use function array_values;
 use function count;
 use function implode;
 
 /**
  * Composite expression is responsible to build a group of similar expression.
+ *
+ * This class is immutable.
  */
 class CompositeExpression implements Countable
 {
@@ -43,51 +47,10 @@ class CompositeExpression implements Countable
      */
     public function __construct(string $type, array $parts = [])
     {
-        $this->type = $type;
-
-        $this->addMultiple($parts);
-    }
-
-    /**
-     * Adds multiple parts to composite expression.
-     *
-     * @deprecated This class will be made immutable. Use with() instead.
-     *
-     * @param array<int, self|string> $parts
-     *
-     * @return $this
-     */
-    public function addMultiple(array $parts = []) : self
-    {
-        foreach ($parts as $part) {
-            $this->add($part);
-        }
-
-        return $this;
-    }
-
-    /**
-     * Adds an expression to composite expression.
-     *
-     * @deprecated This class will be made immutable. Use with() instead.
-     *
-     * @param self|string $part
-     *
-     * @return $this
-     */
-    public function add($part) : self
-    {
-        if (empty($part)) {
-            return $this;
-        }
-
-        if ($part instanceof self && count($part) === 0) {
-            return $this;
-        }
-
-        $this->parts[] = $part;
-
-        return $this;
+        $this->type  = $type;
+        $this->parts = array_values(array_filter($parts, static function ($part) {
+            return ! ($part instanceof self && count($part) === 0);
+        }));
     }
 
     /**

diff --git a/lib/Doctrine/DBAL/Query/QueryBuilder.php b/lib/Doctrine/DBAL/Query/QueryBuilder.php
@@ -1039,7 +1039,7 @@ private function createPredicate($predicate, ...$predicates)
     private function appendToPredicate($currentPredicate, string $type, ...$predicates)
     {
         if ($currentPredicate instanceof CompositeExpression && $currentPredicate->getType() === $type) {
-            return $currentPredicate->addMultiple($predicates);
+            return $currentPredicate->with(...$predicates);
         }
 
         if ($currentPredicate !== null) {

diff --git a/tests/Doctrine/Tests/DBAL/Query/Expression/CompositeExpressionTest.php b/tests/Doctrine/Tests/DBAL/Query/Expression/CompositeExpressionTest.php
@@ -23,25 +23,6 @@ public function testCount() : void
         self::assertCount(2, $expr);
     }
 
-    public function testAdd() : void
-    {
-        $expr = new CompositeExpression(CompositeExpression::TYPE_OR, ['u.group_id = 1']);
-
-        self::assertCount(1, $expr);
-
-        $expr->add(new CompositeExpression(CompositeExpression::TYPE_AND, []));
-
-        self::assertCount(1, $expr);
-
-        $expr->add(new CompositeExpression(CompositeExpression::TYPE_OR, ['u.user_id = 1']));
-
-        self::assertCount(2, $expr);
-
-        $expr->add('u.user_id = 1');
-
-        self::assertCount(3, $expr);
-    }
-
     public function testWith() : void
     {
         $expr = new CompositeExpression(CompositeExpression::TYPE_OR, ['u.group_id = 1']);

diff --git a/tests/Doctrine/Tests/DBAL/Query/Expression/ExpressionBuilderTest.php b/tests/Doctrine/Tests/DBAL/Query/Expression/ExpressionBuilderTest.php
@@ -50,7 +50,7 @@ public function testAndX(array $parts, string $expected) : void
         $composite = $this->expr->andX();
 
         foreach ($parts as $part) {
-            $composite->add($part);
+            $composite = $composite->with($part);
         }
 
         self::assertEquals($expected, (string) $composite);
@@ -123,7 +123,7 @@ public function testOrX(array $parts, string $expected) : void
         $composite = $this->expr->orX();
 
         foreach ($parts as $part) {
-            $composite->add($part);
+            $composite = $composite->with($part);
         }
 
         self::assertEquals($expected, (string) $composite);