Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make CompositeExpression immutable #3858

Merged
merged 1 commit into from
Jan 30, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions UPGRADE.md
Original file line number Diff line number Diff line change
@@ -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.
Expand Down
53 changes: 8 additions & 45 deletions lib/Doctrine/DBAL/Query/Expression/CompositeExpression.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -43,51 +47,10 @@ class CompositeExpression implements Countable
*/
public function __construct(string $type, array $parts = [])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a future PR, I'd like the constructor to be made private in favor of two named constructors — and() and or() — which would accept ($part, ...$parts) and therefore not allow construction of an empty composite expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I've mentioned that this is temporary in #3850 (comment). I'd like to complete immutability first, then move on to stop accepting empty parts. And you're right, factory methods will be a good idea!

{
$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);
}));
}

/**
Expand Down
2 changes: 1 addition & 1 deletion lib/Doctrine/DBAL/Query/QueryBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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']);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down