Skip to content

Loading…

DDC-2341: [GH-606] Don't add empty Expr to another one #3047

Closed
doctrinebot opened this Issue · 3 comments

2 participants

@doctrinebot

Jira issue originally created by user @beberlei:

This issue is created automatically through a Github pull request on behalf of jean-gui:

Url: #606

Message:

Doctrine should not allow to add an empty Expr to another one. Current code allows that, which can lead to wrong DQL.
Example 1:

$andExpr = $this->_expr->andx();
$andExpr->add($this->_expr->andx());
$andExpr->add($this->_expr->eq(1, 1));
echo $andExpr;

will output:

 AND 1 = 1

instead of:

1 = 1

Example 2:

echo $andExpr;

will output:

 AND  AND 

instead of nothing.

IRC log of the discusion on #doctrine:

[jeudi 7 mars 2013] [14:36:18] <Jean-Gui>       hi
[jeudi 7 mars 2013] [14:36:35] <Jean-Gui>       I have a question about Doctrine/ORM/Query/Expr/Base.php
[jeudi 7 mars 2013] [14:37:17] <Jean-Gui>       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] <Jean-Gui>       [[ if ( $arg !== null ](| ($arg instanceof self && $arg->count() > 0) ) )]
[jeudi 7 mars 2013] [14:39:49] <Jean-Gui>       if $arg is not null, then it will get added even if $arg->count === 0
[jeudi 7 mars 2013] [14:41:03] <ocramius>       yes
[jeudi 7 mars 2013] [14:41:42] <Jean-Gui>       that doesn't seem right, does it?
[jeudi 7 mars 2013] [14:43:29] <ocramius>       why not?
[jeudi 7 mars 2013] [14:43:34] <ocramius>       can you elaborate?
[jeudi 7 mars 2013] [14:45:37] <Jean-Gui>       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] <Ninj>   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] <Jean-Gui>       right
[jeudi 7 mars 2013] [14:46:30] <Ninj>   this condition is indeed bugged
[jeudi 7 mars 2013] [14:47:12] <alcuadradoatwork>       I see Jean-Gui's point too
[jeudi 7 mars 2013] [14:47:18] <ocramius>       write a test case then :)
[jeudi 7 mars 2013] [14:47:24] <alcuadradoatwork>       maybe it works right, but it's confusing
[jeudi 7 mars 2013] [14:48:07] <Ninj>   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] <Jean-Gui>       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] <alcuadradoatwork>       isn't this enough?  if ($arg instanceof self && $arg->count() > 0)
[jeudi 7 mars 2013] [14:49:18] <Ninj>   i think it is, alcuadradoatwork
[jeudi 7 mars 2013] [14:49:28] <Jean-Gui>       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] <ocramius>       alcuadradoatwork: again... if it is a buggy condition, write a small test and open a PR :)
[jeudi 7 mars 2013] [14:50:03] <alcuadradoatwork>       ocramius, it depends on your definition of "buggy"
[jeudi 7 mars 2013] [14:50:32] <alcuadradoatwork>       it won't damage the behavior of the software, but it's quality its kind of pour
[jeudi 7 mars 2013] [14:51:19] <ocramius>       alcuadradoatwork: yes, still it needs coverage. I didn't say it needs a *FAILING* test
[jeudi 7 mars 2013] [14:51:38] <alcuadradoatwork>       I see your point
[jeudi 7 mars 2013] [14:51:47] <Ninj>   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] <Jean-Gui>       $args should be added if it's an instance of self or $_allowedClasses
[jeudi 7 mars 2013] [14:53:16] <Ninj>   hum
[jeudi 7 mars 2013] [14:53:37] <Ninj>   $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] <Jean-Gui>       yes, Ninj
[jeudi 7 mars 2013] [14:55:01] <Jean-Gui>       thanks for your help guys, I'll submit a bug report with some test cases
[jeudi 7 mars 2013] [14:55:43] <Ninj>   your welcome Jean-Gui
@doctrinebot

Issue was closed with resolution "Fixed"

@doctrinebot

Comment created by @beberlei:

A related Github Pull-Request [GH-606] was closed
#606

@doctrinebot

Comment created by @doctrinebot:

A related Github Pull-Request [GH-606] was closed:
doctrine/dbal#606

@beberlei beberlei was assigned by doctrinebot
@doctrinebot doctrinebot added this to the 2.3.3 milestone
@doctrinebot doctrinebot closed this
@doctrinebot doctrinebot added the Bug label
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.