-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Invalid SQL generated by TranslationWalker when using LIMIT #2917
Comments
Hi there, IMO, it's linked to doctrine/orm#11188. With ORM3.3 the SqlFinalizer gets executed twice, adding I get the error too because I too am using As of now doctrine-extensions is not compatible with doctrine/orm:3.3. |
Does it change any with 3.18? The new version includes #2895 which uses the new classes introduced by that ORM PR. |
In my example I am using doctrine/orm 2.20.1 It works as expected with doctrine-extensions 3.17.1 and the error only occurs with doctrine-extensions 3.18.0. No other versions changed. |
If you can certify your doctrine/orm version did not evolve when you updated to doctrine-extensions 3.18.0 then I might be wrong. I might have wrongly associated the bug with ORM 3.3 because in effect, using MariaDB, I couldn't update to ORM 3 until doctrine-extensions 3.18.0. I'll have to continue investigating. |
FWIW doctrine/orm#11188 landed in both ORM 2.20 and 3.3 so it's possible the bug exists with the current 2.x and 3.x ORM releases. |
Minimum reproducer code:
(I wanted to make sure it had nothing to do with KnpPaginator that I'm also using). But with Doctrine's Paginator, the issue arises all the same. Edit: currently testing versions of doctrine/orm and gedmo/doctrine-extensions to pinpoint the issue. |
I can confirm that the doctrine/orm version did not change in my project. |
ORM 3.3.1 / Extensions 3.17.1 => OK ORM 2.20.1 / Extensions 3.17.1 => OK Seems like it. Not sure what's the best course of action. Is it a Doctrine bug or an Extensions bug? |
I don't use the translatable extension so I honestly don't know where to trace this down to, so it's possible the compat classes I added for the ORM changes need more tuning, or there really is an ORM bug here. Digging through the code I'm not seeing anything that explicitly deals with limit or offset stuff, nor is this tested right now. But it's definitely broken. I need to get to paid work, but I added this test case to cover offset handling: diff --git a/tests/Gedmo/Translatable/TranslationQueryWalkerTest.php b/tests/Gedmo/Translatable/TranslationQueryWalkerTest.php
index 1eeb77a5..ba4e13be 100644
--- a/tests/Gedmo/Translatable/TranslationQueryWalkerTest.php
+++ b/tests/Gedmo/Translatable/TranslationQueryWalkerTest.php
@@ -125,6 +125,19 @@ final class TranslationQueryWalkerTest extends BaseTestCaseORM
static::assertSame('good', $comments[0]['subject']);
}
+ public function testPaginatedQuery(): void
+ {
+ $this->populateMore();
+
+ $dql = 'SELECT a FROM '.Article::class.' a';
+ $q = $this->em->createQuery($dql);
+ $q->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, TranslationWalker::class);
+ $q->setFirstResult(0);
+ $q->setMaxResults(1);
+
+ var_dump($q->getResult());
+ }
+
public function testShouldSelectWithTranslationFallbackOnSimpleObjectHydration(): void
{
$this->em->getConfiguration()->addCustomHydrationMode( When the ORM calls into SELECT a0_.id AS id_0, CAST(t1_.content AS VARCHAR(128)) AS title_1, CAST(t2_.content AS CLOB) AS content_2, CAST(t3_.content AS INTEGER) AS views_3, COALESCE(CAST(t4_.content AS VARCHAR), a0_.author) AS author_4
FROM Article a0_
LEFT JOIN ext_translations t1_ ON t1_.locale = 'lt_lt' AND t1_.field = 'title' AND t1_.object_class = 'Gedmo\Tests\Translatable\Fixture\Article' AND t1_.foreign_key = a0_.id
LEFT JOIN ext_translations t2_ ON t2_.locale = 'lt_lt' AND t2_.field = 'content' AND t2_.object_class = 'Gedmo\Tests\Translatable\Fixture\Article' AND t2_.foreign_key = a0_.id
LEFT JOIN ext_translations t3_ ON t3_.locale = 'lt_lt' AND t3_.field = 'views' AND t3_.object_class = 'Gedmo\Tests\Translatable\Fixture\Article' AND t3_.foreign_key = a0_.id
LEFT JOIN ext_translations t4_ ON t4_.locale = 'lt_lt' AND t4_.field = 'author' AND t4_.object_class = 'Gedmo\Tests\Translatable\Fixture\Article' AND t4_.foreign_key = a0_.id
LIMIT 1 After the finalizer calls (On a side note maybe it'd also be a little easier to just bump the ORM dependency to |
With
With
In the end the problematic test is
If you replace it by Edit: Then again it could also be doctrine/orm#11188 that did not take into account all possible outcomes. It looks like it introduced at least one other bug (doctrine/orm#11741). Should we open an issue there? |
If you need a hot patch, changing If there's a simple case that can be covered in the ORM test suite, it'd be good to open an issue upstream. But like I said before, I'm also not 100% confident the compat layer is done right so I could've accidentally screwed something up on untested code 😅 Edit: The test case I patched in does pass when not using the newer |
#2920 should fix this. |
Environment
Package
show
Doctrine packages
show
PHP version
Subject
In version 3.18.0 an extra
LIMIT
clause is appended to SQL statements after theTranslationWalker
. This does not happen in 3.17.1.Steps to reproduce
Expected results
Actual results
The text was updated successfully, but these errors were encountered: