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

Don't add empty Expr to another one #606

Closed
wants to merge 1 commit into from
Closed

Don't add empty Expr to another one #606

wants to merge 1 commit into from

Conversation

jean-gui
Copy link
Contributor

@jean-gui jean-gui commented Mar 8, 2013

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:

$andExpr = $this->_expr->andx();
$andExpr->add($this->_expr->andx());
$andExpr->add($this->_expr->andx());
$andExpr->add($this->_expr->andx());
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
Copy link

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DDC-2341

@beberlei
Copy link
Member

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants