Fix CompositeExpression#add() optimization, which was leading to broken CompositeExpression instances
#2785
8000
Conversation
|
|
||
| $expr->add(new CompositeExpression(CompositeExpression::TYPE_OR, [])); | ||
|
|
||
| $this->assertEquals(2, count($expr)); |
There was a problem hiding this comment.
This scenario doesn't hit the code path you changed
There was a problem hiding this comment.
@Ocramius Unfortunately, you are wrong. It was failing test.
There was a problem hiding this comment.
Could you move it to a separate test method, and verify both paths then? Specifically:
- When an empty compositeexpression is added
- When a non-empty compositeexpression is added
| @@ -91,7 +91,7 @@ public function addMultiple(array $parts = []) | |||
| */ | |||
| public function add($part) | |||
| { | |||
| if ( ! empty($part) || ($part instanceof self && $part->count() > 0)) { | |||
| if ( ! empty($part) && ! ($part instanceof self && $part->count() === 0)) { | |||
There was a problem hiding this comment.
Can you refactor this conditional to remove some negations? It is extremely hard to read.
| { | ||
| $expr = new CompositeExpression(CompositeExpression::TYPE_OR, array('u.group_id = 1')); | ||
|
|
||
| $this->assertEquals(1, count($expr)); |
There was a problem hiding this comment.
could you use assertCount here?
| @@ -91,7 +91,7 @@ public function addMultiple(array $parts = []) | |||
| */ | |||
| public function add($part) | |||
| { | |||
| if ( ! empty($part) || ($part instanceof self && $part->count() > 0)) { | |||
| if (!empty($part) && count($part)) { | |||
There was a problem hiding this comment.
What if $part is not empty but not countable? E.g. $expr->add('id = 1'). It will fail on PHP 7.2.
There was a problem hiding this comment.
I believe we still should check for instanceof self and only check count in this case. Otherwise, we don't know if empty collection will cast to an empty string (can be any custom class). Something like:
if (empty($part)) {
return;
}
if ($part instanceof self && !count($part)) {
return;
}
// proceedCompositeExpression#add() optimization, which was leading to bbroken CompositeExpression instances
|
Merged, thanks @Grzesie2k! Backported to |
CompositeExpression#add() optimization, which was leading to bbroken CompositeExpression instancesCompositeExpression#add() optimization, which was leading to broken CompositeExpression instances
Current implementation of CompositeExpression->add() has invalid rule for empty instances of self class (never executed).