[DDC-2235] Fix for using a LEFT JOIN onto an entity with single table inheritance #656
Conversation
The issue is when using DQL to perform a left join on an entity using single table inheritance, doctrine tries to insert an `IN()` predicate into the `WHERE` clause for all of the discriminator values. That makes sense and is valid, so it would be wrong to remove that behaviour. However when using a *left* join having an `IN()` in the main where clause makes the `LEFT JOIN` pretty much useless, as it implicitly creates a `WHERE NOT NULL` clause. This commit attempts to fix that by including an `OR IS NULL` in the query if the join is a `LEFT JOIN`.
| @@ -1026,6 +1040,10 @@ public function walkJoin($join) | |||
| ? ' LEFT JOIN ' | |||
| : ' INNER JOIN '; | |||
|
|
|||
| if ($joinType == AST\Join::JOIN_TYPE_LEFT) { | |||
There was a problem hiding this comment.
use strict comparison and yoda conditionals
|
By the way, this doesn't seem to solve the problem, rather than patching up the symptoms... Shouldn't all the |
|
I'd much prefer that, but not quite sure where the other parts of the ON clause are generated, and whether its best to just arbitrarily put an AND IN() on the end of that string, or if there is a better way to go about it. If you could point me in the right direction I'll gladly implement it. :) |
|
Got it - will push a new commit with that implemented shortly. |
|
@Ocramius updated |
| @@ -437,7 +437,7 @@ private function _generateDiscriminatorColumnConditionSQL(array $dqlAliases) | |||
| } | |||
|
|
|||
| $sqlParts[] = (($this->useSqlTableAliases) ? $this->getSQLTableAlias($class->getTableName(), $dqlAlias) . '.' : '') | |||
| . $class->discriminatorColumn['name'] . ' IN (' . implode(', ', $values) . ')'; | |||
| . $class->discriminatorColumn['name'] . ' IN (' . implode(', ', $values) . ')'; | |||
There was a problem hiding this comment.
Gah, will fix this whitespace.
|
@Ocramius Any news on this being reviewed again / merged into master? |
|
The patch worked for my use-case (similar situation, left-joining a model with a discriminator column) |
|
@tarnfeld merges of such major changes are not up to me - I generally agree with what you have implemented here though |
| . $condExprConjunction . '(' . $this->walkConditionalExpression($join->conditionalExpression) . ')'; | ||
|
|
||
| $discrSql = $this->_generateDiscriminatorColumnConditionSQL(array($joinAlias)); | ||
| if ( ! empty($discrSql)) { |
There was a problem hiding this comment.
Missing newline before this line
|
@Ocramius Thanks for the heads up, i've pushed a fix for the new line. |
|
I'm working on this issue. |
|
I replaced this PR with a more in depth solution as of #758 |
|
Nice one, thanks. |
Possible fix for the bug DDC-2235. I'd love to hear some opinions on whether this is the right way to go about this issue. I'm not particularly familiar with the internals of doctrine so there may be a better solution.
The issue is when using DQL to perform a left join on an entity using single table inheritance, doctrine tries to insert an
IN()predicate into theWHEREclause for all of the discriminator values. That makes sense and is valid, so it would be wrong to remove that behaviour.However when using a left join having an
IN()in the main where clause makes theLEFT JOINpretty much useless, as it implicitly creates aWHERE NOT NULLclause. This commit attempts to fix that by including anOR IS NULLin the query if the join is aLEFT JOIN.I've added some regression tests to ensure this bug never creeps back in. They fail on master (highlighting the bug) and pass after these commits have been applied. I've also included a couple of other queries as tests to be sure only this one case has been affected.