doctrine/archived-orm
1
0
Fork 0
mirror of https://github.com/doctrine/orm.git synced 2026-03-23 22:42:18 +01:00
Code Issues 5.3k Packages Projects Releases 100 Wiki Activity

[PR #606] [CLOSED] Don't add empty Expr to another one #8436

New Issue
Closed
opened 2026-01-22 15:59:55 +01:00 by admin · 0 comments
admin commented 2026-01-22 15:59:55 +01:00
Owner
Copy Link

📋 Pull Request Information

Original PR: https://github.com/doctrine/orm/pull/606
Author: @jean-gui
Created: 3/8/2013
Status: ❌ Closed

Base: master ← Head: empty-expr-add

📝 Commits (1)

  • 2a621ab Don't add empty expression to another one

📊 Changes

2 files changed (+15 additions, -1 deletions)

View changed files

📝 lib/Doctrine/ORM/Query/Expr/Base.php (+1 -1)
📝 tests/Doctrine/Tests/ORM/Query/ExprTest.php (+14 -0)

📄 Description

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

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/doctrine/orm/pull/606 **Author:** [@jean-gui](https://github.com/jean-gui) **Created:** 3/8/2013 **Status:** ❌ Closed **Base:** `master` ← **Head:** `empty-expr-add` --- ### 📝 Commits (1) - [`2a621ab`](https://github.com/doctrine/orm/commit/2a621aba65f8ee8667c69e8dadea96b5d8d377ca) Don't add empty expression to another one ### 📊 Changes **2 files changed** (+15 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `lib/Doctrine/ORM/Query/Expr/Base.php` (+1 -1) 📝 `tests/Doctrine/Tests/ORM/Query/ExprTest.php` (+14 -0) </details> ### 📄 Description 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 $andExpr = $this->_expr->andx(); $andExpr->add($this->_expr->andx()); $andExpr->add($this->_expr->andx()); $andExpr->add($this->_expr->andx()); 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] <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 ``` --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
admin added the pull-request label 2026-01-22 15:59:55 +01:00
admin closed this issue 2026-01-22 15:59:56 +01:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
BC Break
Bug
Can't Fix
Deprecation
Documentation
DQL
Duplicate
Failing Test
Hacktoberfest
Improvement
Improvement
Incomplete
Invalid
Missing Tests
New Feature
PR Created
pull-request
Mirrored from GitHub Pull Request
 Question
Regression
Status: Waiting feedback
Waiting feedback
WIP
Won't Fix
No Label pull-request
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
admin
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: doctrine/archived-orm#8436
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?