Prevent adding empty CompositeExpression objects #2389
Conversation
647e812 to
5afcd39
Compare
| $emptyExpr = new CompositeExpression(CompositeExpression::TYPE_AND, []); | ||
| $expr = new CompositeExpression(CompositeExpression::TYPE_AND, [$emptyExpr]); | ||
|
|
||
| $this->assertEquals(0, count($expr)); |
There was a problem hiding this comment.
I don't quite get it. The object is mutable, so you can add any more parts as you want as long as you do not cast it as string. Then you might want to check for number of parts >= 2 and eventually throw an exception. But your test is not revealing any issue. Also I am not sure if this is not considered part of the internal API /cc @Ocramius
There was a problem hiding this comment.
Sorry, but I don't get your comment at all. I can try to explain the issue a bit more, but maybe there is any way to get in contact with you in any chat?
The problem which I ran into:
- reading the (old) code, it should be possible to pass an object of type CompositeExpression (object2) as a new part in another CompositeExpression object (object1)
- object2 should only be added it it contains parts on its own
- so creating object2 with empty parts and passing it to object1 shouldn't change parts of object1
Currently this is broken and an empty object2 is added to the parts of object1. IMHO this is not intended. I created the patch and a unit test which fails before and is fixed with the patch.
There was a problem hiding this comment.
Hehe sorry if I was not clear enough. What I want to point out is that if passing an empty instance of CompositeExpression it should not be rejected at all IMO because it is passed as an object by reference and you can still add parts to that later on. Example:
$expression1 = new CompositeExpression(CompositeExpression::TYPE_AND, []);
$expression2 = new CompositeExpression(CompositeExpression::TYPE_AND, [$expression1]);
$expression1->add(array('u.group_id = 1')); // now $expression1 is not empty anymoreThis is what I was referring to as "the API is mutable".
There was a problem hiding this comment.
Ir that would be a way how the api can be used, why was there a check
($part instanceof self && $part->count() > 0)
in the code? I think it shouldn't be possible to add constraints after adding the object to the CompositeExpression
There was a problem hiding this comment.
I don't have a strong opinion on this TBH and I don't know what the original intention was /cc @Ocramius @guilhermeblanco
There was a problem hiding this comment.
It would be much easier to prevent an empty operands list to make it into a CompositeExpression, but I suppose that we can't change the API now.
There was a problem hiding this comment.
@Ocramius just to be sure: what you're saying is that the fix here is correct, but as it affects public API, it can't be fixed?
There was a problem hiding this comment.
No, it was just wishful thinking: if we didn't allow mutation of expressions, this problem would not exist in first place.
Anyway, the fix doesn't seem correct to me: I think the correct way of dealing with the problem is to remove unnecessary composite expression building within __toString, which is called at the very end of the entire procedure. This way, even if state changed in-between, we'd still get consistent behavior.
|
Fixed in #3864. |
Fixes #2388