doctrine/doctrine2 — pull request #606 by jean-gui (2013-03-08 16:14:53+00:00)
Title: Don't add empty Expr to another one

Doctrine should not allow to add an empty Expr to another one. Current code allows that, which can lead to wrong DQL.
Example 1:
```php
$andExpr = $this->_expr->andx();
$andExpr->add($this->_expr->andx());
$andExpr->add($this->_expr->eq(1, 1));
echo $andExpr;
```
will output:
```sql
AND 1 = 1
```
instead of:
```sql
1 = 1
```

Example 2:
```php
echo $andExpr;
```
will output:
```sql
AND AND
```
instead of nothing.

IRC log of the discusion on #doctrine:
```irc
[jeudi 7 mars 2013] [14:36:18] hi
[jeudi 7 mars 2013] [14:36:35] I have a question about Doctrine/ORM/Query/Expr/Base.php
[jeudi 7 mars 2013] [14:37:17] looking at the add function (https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Query/Expr/Base.php#L89), the first if seems wrong
[jeudi 7 mars 2013] [14:37:39] [[ if ( $arg !== null || ($arg instanceof self && $arg->count() > 0) ) ]]
[jeudi 7 mars 2013] [14:39:49] if $arg is not null, then it will get added even if $arg->count === 0
[jeudi 7 mars 2013] [14:41:03] yes
[jeudi 7 mars 2013] [14:41:42] that doesn't seem right, does it?
[jeudi 7 mars 2013] [14:43:29] why not?
[jeudi 7 mars 2013] [14:43:34] can you elaborate?
[jeudi 7 mars 2013] [14:45:37] that "if" seems to be meaning that the function shouldn't add $arg if $arg->count() equals 0
[jeudi 7 mars 2013] [14:45:45] ocramius: i think Jean-Gui means that the right side of the OR will be called only if the left side is false, which means $arg is null. And if $arg is null it cannot be an object
[jeudi 7 mars 2013] [14:46:07] right
[jeudi 7 mars 2013] [14:46:30] this condition is indeed bugged
[jeudi 7 mars 2013] [14:47:12] I see Jean-Gui's point too
[jeudi 7 mars 2013] [14:47:18] write a test case then :)
[jeudi 7 mars 2013] [14:47:24] maybe it works right, but it's confusing
[jeudi 7 mars 2013] [14:48:07] this condition will not throw any error because the "instanceof" will prevent the $arg::count function to be called on a null object, but it is still useless
[jeudi 7 mars 2013] [14:48:20] I think [[ if ( $arg !== null && (!($arg instanceof self) || ($arg instanceof self && $arg->count() > 0)) ) ]] would work better
[jeudi 7 mars 2013] [14:48:31] * Jean-Gui will look into how to submit bug reports
[jeudi 7 mars 2013] [14:48:49] isn't this enough? if ($arg instanceof self && $arg->count() > 0)
[jeudi 7 mars 2013] [14:49:18] i think it is, alcuadradoatwork
[jeudi 7 mars 2013] [14:49:28] no, because if $arg is not an instance of self, I think we still want to add it
[jeudi 7 mars 2013] [14:49:45] alcuadradoatwork: again... if it is a buggy condition, write a small test and open a PR :)
[jeudi 7 mars 2013] [14:50:03] ocramius, it depends on your definition of "buggy"
[jeudi 7 mars 2013] [14:50:32] it won't damage the behavior of the software, but it's quality its kind of pour
[jeudi 7 mars 2013] [14:51:19] alcuadradoatwork: yes, still it needs coverage. I didn't say it needs a _FAILING_ test
[jeudi 7 mars 2013] [14:51:38] I see your point
[jeudi 7 mars 2013] [14:51:47] if the logic is : "add any non-null object, but if it's self so add it only if count>0" then it must be rewriten
[jeudi 7 mars 2013] [14:52:46] $args should be added if it's an instance of self or $_allowedClasses
[jeudi 7 mars 2013] [14:53:16] hum
[jeudi 7 mars 2013] [14:53:37] $args should be added if it's an instance of self with count > 0 or any other $_allowedClasse
[jeudi 7 mars 2013] [14:53:55] yes, Ninj
[jeudi 7 mars 2013] [14:55:01] thanks for your help guys, I'll submit a bug report with some test cases
[jeudi 7 mars 2013] [14:55:43] your welcome Jean-Gui
```