-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Update DQL arbitrary joins to use the ON keyword instead of WITH #12198
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -1609,8 +1609,7 @@ public function SubselectIdentificationVariableDeclaration(): AST\Identification | |||
|
|
||||
| /** | ||||
| * Join ::= ["LEFT" ["OUTER"] | "INNER"] "JOIN" | ||||
| * (JoinAssociationDeclaration | RangeVariableDeclaration) | ||||
| * ["WITH" ConditionalExpression] | ||||
| * (JoinAssociationDeclaration ["WITH" ConditionalExpression] | RangeVariableDeclaration [("ON" | "WITH") ConditionalExpression]) | ||||
| */ | ||||
| public function Join(): AST\Join | ||||
| { | ||||
|
|
@@ -1644,22 +1643,32 @@ public function Join(): AST\Join | |||
|
|
||||
| $next = $this->lexer->glimpse(); | ||||
| assert($next !== null); | ||||
| $joinDeclaration = $next->type === TokenType::T_DOT ? $this->JoinAssociationDeclaration() : $this->RangeVariableDeclaration(); | ||||
| $adhocConditions = $this->lexer->isNextToken(TokenType::T_WITH); | ||||
| $join = new AST\Join($joinType, $joinDeclaration); | ||||
| $conditionalExpression = null; | ||||
|
|
||||
| // Describe non-root join declaration | ||||
| if ($joinDeclaration instanceof AST\RangeVariableDeclaration) { | ||||
| $joinDeclaration->isRoot = false; | ||||
| } | ||||
| if ($next->type === TokenType::T_DOT) { | ||||
| $joinDeclaration = $this->JoinAssociationDeclaration(); | ||||
|
|
||||
| // Check for ad-hoc Join conditions | ||||
| if ($adhocConditions) { | ||||
| $this->match(TokenType::T_WITH); | ||||
| if ($this->lexer->isNextToken(TokenType::T_WITH)) { | ||||
| $this->match(TokenType::T_WITH); | ||||
| $conditionalExpression = $this->ConditionalExpression(); | ||||
| } | ||||
| } else { | ||||
| $joinDeclaration = $this->RangeVariableDeclaration(); | ||||
| $joinDeclaration->isRoot = false; | ||||
|
|
||||
| $join->conditionalExpression = $this->ConditionalExpression(); | ||||
| if ($this->lexer->isNextToken(TokenType::T_ON)) { | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have a computer near me right now, so I cannot try out... But what happens when you omit the ˋWITHˋ as well as ˋONˋ here? Is it possible to get away with a JOIN without a condition and should we prevent that from happening?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was already supported before. The SqlWalker has code to deal with it (even though the doc says the join condition is required for arbitrary joins). Note that the query generated for a join without condition might have worse performance: Line 1152 in c6207b1
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And see this
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that case, would it make sense to remove the
remark from the docs?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will keep it for now, because I'm not sure this is something we should actually encourage, given the kind of hacks it requires for cross-platform support. |
||||
| $this->match(TokenType::T_ON); | ||||
| $conditionalExpression = $this->ConditionalExpression(); | ||||
| } elseif ($this->lexer->isNextToken(TokenType::T_WITH)) { | ||||
| $this->match(TokenType::T_WITH); | ||||
| $conditionalExpression = $this->ConditionalExpression(); | ||||
| Deprecation::trigger('doctrine/orm', 'https://github.com/doctrine/orm/issues/12192', 'Using WITH for the join condition of arbitrary joins is deprecated. Use ON instead.'); | ||||
| } | ||||
| } | ||||
|
|
||||
| $join = new AST\Join($joinType, $joinDeclaration); | ||||
| $join->conditionalExpression = $conditionalExpression; | ||||
|
|
||||
| return $join; | ||||
| } | ||||
|
|
||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -90,4 +90,5 @@ enum TokenType: int | |
| case T_WHERE = 255; | ||
| case T_WITH = 256; | ||
| case T_NAMED = 257; | ||
| case T_ON = 258; | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.