Update DQL arbitrary joins to use the ON keyword instead of WITH #12198
Conversation
stof
force-pushed
the
arbitrary_join_on
branch
from
4ec12e9 to
32ed181
Compare
stof
force-pushed
the
arbitrary_join_on
branch
from
32ed181 to
44a69c4
Compare
|
Can you have a look at the failing tests? |
stof
force-pushed
the
arbitrary_join_on
branch
from
44a69c4 to
c16c9d2
Compare
stof
force-pushed
the
arbitrary_join_on
branch
from
c16c9d2 to
9f6e841
Compare
stof
force-pushed
the
arbitrary_join_on
branch
from
9f6e841 to
30de04f
Compare
| $joinDeclaration->isRoot = false; | ||
|
|
||
| $join->conditionalExpression = $this->ConditionalExpression(); | ||
| if ($this->lexer->isNextToken(TokenType::T_ON)) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
And see this if ($adhocConditions) { in the old code that was making the WITH keyword optional.
There was a problem hiding this comment.
In that case, would it make sense to remove the
ON is required, even if it is 1 = 1
remark from the docs?
There was a problem hiding this comment.
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.
In my opinion, it might make sense to deprecate the case of an arbitrary join without a join condition. I guess the main reason it was initially supported is related to the fact that this was initially parsed on top of association joins (which is why it was implemented with the WITH keyword instead of the intended ON), where the filter condition is optional (because an association join already has a built-in condition for the association)
|
No objections. Unsure if the parser would now accept JOINS without conditions that it would reject previously. |
|
@stof Maybe the "fail on deprecation" test passes if you rebase on 3.6.x? |
stof
force-pushed
the
arbitrary_join_on
branch
from
30de04f to
a308ec7
Compare
|
😿 |
|
@mpdude there is still 1 deprecation coming from DBAL, but it is not related to this PR. |
|
Yes. 🚀 from my POV |
|
Oh, wait, does this deserve an entry in the UPGRADING.md? |
DQL arbitrary joins are semantically equivalent to SQL joins, so using the same keyword reduces confusion. It also means that in next major version, the WITH keyword will only be about applying adhoc filtering on relations instead of having 2 responsibilities.
stof
force-pushed
the
arbitrary_join_on
branch
from
a308ec7 to
587caf8
Compare
|
@mpdude good catch. I added an upgrade note |
4 participants
DQL arbitrary joins are semantically equivalent to SQL joins, so using the same keyword reduces confusion. It also means that in next major version, the WITH keyword will only be about applying adhoc filtering on relations instead of having 2 responsibilities.
Closes #12192
Closes #7891 (the confusion is solved)
Closes #3544 (that constant now produces valid code)