Fix #1021 order by hidden eager loading#1022
Conversation
soyuka
commented
Mar 30, 2017
soyuka
commented
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #1021 |
| License | MIT |
| Doc PR | na |
355bbf9 to
f05370b
Compare
|
I'm curious about |
| $wherePart = $queryBuilder->getDQLPart('where'); | ||
|
|
||
| //reset parts | ||
| $queryBuilderClone->resetDQLPart('orderBy'); |
There was a problem hiding this comment.
Logically the sequence should be: join - where - orderBy \o/
|
Your right, I'll add those in ~30min ;) |
2be6ab5 to
03b581c
Compare
|
W8 do not merge I'm adding a fix for #1023 |
| INNER JOIN o_2.compositeLabel label_2 | ||
| WHERE item_2.field1 = :foo | ||
| ) | ||
| SQL; |
There was a problem hiding this comment.
WDYT @teohhanhui ? A better fix would be to fix doctrine but this works fine as a temporary fix (will take a long time to get that fixed in doctrine :p). Indeed, it should be WHERE (o.item, o.label) IN (subquery).
There was a problem hiding this comment.
Maybe can you add a comment with this rationale? We'll remove this temporary when (if) the patch will be merged in Doctrine.
There was a problem hiding this comment.
I didn't even know there's such a syntax in SQL. Obviously I don't really use composite keys 😛
Anyway, I think this is fine.
There was a problem hiding this comment.
Oh, but I think the tests are confusing, because there seems to be no eager loading going on (how do we tell?). If so, we can skip query rewriting in such cases, right?
There was a problem hiding this comment.
The fix will get applied only if the conditions are a match (where clause + eager + composite identifier in this case). There will be no rewriting if those conditions don't match.
I force those conditions with the test.
There was a problem hiding this comment.
What I mean is that even though $forceEager is true, the query itself might not need to be rewritten if there is no eager loading going on, no? (Not specific to this test case.)
There was a problem hiding this comment.
Eager loading is active when there are joins, this condition checks that. The rewrite is needed only when there is a where clause.
|
Feel free to merge this one @soyuka. I'll tag a new patch release after that. |
ee14560 to
8ba69e3
Compare
8ba69e3 to
7a59035
Compare
|
|
||
| private function toDQLString(string $dql): string | ||
| { | ||
| return preg_replace('/\\r\\n|\\n/', '', str_replace(' ', '', $dql)); |
There was a problem hiding this comment.
If we improve the regex then there'd be no need for the str_replace 😄
There was a problem hiding this comment.
haha yeah appveyor behavior was driving me crazy, intent was first to use PHP_EOL then I ended up with the regexp. Anyway, no big deal this is just to make tests more readable.
…loading-order-by-hidden Fix api-platform#1021 order by hidden eager loading
3 participants
