Invalid SQL generated by TranslationWalker when using LIMIT

[xelax90]

Environment

Package

show

$ composer show --latest gedmo/doctrine-extensions
name     : gedmo/doctrine-extensions
descrip. : Doctrine behavioral extensions
keywords : Blameable, behaviors, doctrine, extensions, gedmo, loggable, nestedset, odm, orm, sluggable, sortable, timestampable, translatable, tree, uploadable
versions : * v3.18.0
latest   : v3.18.0
type     : library
license  : MIT License (MIT) (OSI approved) https://spdx.org/licenses/MIT.html#licenseText
homepage : http://gediminasm.org/
source   : [git] https://github.com/doctrine-extensions/DoctrineExtensions.git 964db6c4fb5b0fc8aa25cc31b17471b4963460c6
dist     : [zip] https://api.github.com/repos/doctrine-extensions/DoctrineExtensions/zipball/964db6c4fb5b0fc8aa25cc31b17471b4963460c6 964db6c4fb5b0fc8aa25cc31b17471b4963460c6
path     : /git/httpdocs/vendor/gedmo/doctrine-extensions
names    : gedmo/doctrine-extensions

support
email : gediminas.morkevicius@gmail.com
issues : https://github.com/doctrine-extensions/DoctrineExtensions/issues
source : https://github.com/doctrine-extensions/DoctrineExtensions/tree/v3.18.0
wiki : https://github.com/Atlantic18/DoctrineExtensions/tree/main/doc

autoload
psr-4
Gedmo\ => src/

requires
behat/transliterator ^1.2
doctrine/collections ^1.2 || ^2.0
doctrine/deprecations ^1.0
doctrine/event-manager ^1.2 || ^2.0
doctrine/persistence ^2.2 || ^3.0 || ^4.0
php ^7.4 || ^8.0
psr/cache ^1 || ^2 || ^3
psr/clock ^1
symfony/cache ^5.4 || ^6.0 || ^7.0

requires (dev)
doctrine/annotations ^1.13 || ^2.0
doctrine/cache ^1.11 || ^2.0
doctrine/common ^2.13 || ^3.0
doctrine/dbal ^3.7 || ^4.0
doctrine/doctrine-bundle ^2.3
doctrine/mongodb-odm ^2.3
doctrine/orm ^2.14.0 || ^3.0
friendsofphp/php-cs-fixer ^3.14.0
nesbot/carbon ^2.71 || ^3.0
phpstan/phpstan ^2.1.1
phpstan/phpstan-doctrine ^2.0.1
phpstan/phpstan-phpunit ^2.0.3
phpunit/phpunit ^9.6
rector/rector ^2.0.6
symfony/console ^5.4 || ^6.0 || ^7.0
symfony/doctrine-bridge ^5.4 || ^6.0 || ^7.0
symfony/phpunit-bridge ^6.0 || ^7.0
symfony/uid ^5.4 || ^6.0 || ^7.0
symfony/yaml ^5.4 || ^6.0 || ^7.0

suggests
doctrine/mongodb-odm to use the extensions with the MongoDB ODM
doctrine/orm to use the extensions with the ORM

conflicts
doctrine/annotations <1.13 || >=3.0
doctrine/common <2.13 || >=4.0
doctrine/dbal <3.7 || >=5.0
doctrine/mongodb-odm <2.3 || >=3.0
doctrine/orm <2.14.0 || 2.16.0 || 2.16.1 || >=4.0

Doctrine packages

show

$ composer show --latest 'doctrine/*'
doctrine/annotations               2.0.2  2.0.2  Docblock Annotations Parser
doctrine/cache                     2.2.0  2.2.0  PHP Doctrine Cache library is a popular cache implementation that supports many different drivers such as redis, memcache, apc, mongodb and others.
doctrine/collections               2.2.2  2.2.2  PHP Doctrine Collections library that adds additional functionality on top of PHP arrays.
doctrine/common                    3.5.0  3.5.0  PHP Doctrine Common project is a library that provides additional functionality that other Doctrine projects depend on such as better reflection support, p...
doctrine/dbal                      3.9.4  4.2.2  Powerful PHP database abstraction layer (DBAL) with many features for database schema introspection and management.
doctrine/deprecations              1.1.4  1.1.4  A small layer on top of trigger_error(E_USER_DEPRECATED) or PSR-3 logging with options to disable all deprecations or selectively for packages.
doctrine/doctrine-laminas-hydrator 3.6.1  3.6.1  Doctrine hydrators for Laminas applications
doctrine/doctrine-module           6.1.1  6.3.0  Laminas Module that provides Doctrine basic functionality required for ORM and ODM modules
doctrine/doctrine-orm-module       6.1.0  6.3.0  Laminas Module that provides Doctrine ORM functionality
doctrine/event-manager             1.2.0  2.0.1  The Doctrine Event Manager is a simple PHP event system that was built to be used with the various Doctrine projects.
doctrine/inflector                 2.0.10 2.0.10 PHP Doctrine Inflector is a small library that can perform string manipulations with regard to upper/lowercase and singular/plural forms of words.
doctrine/instantiator              2.0.0  2.0.0  A small, lightweight utility to instantiate objects in PHP without invoking their constructors
doctrine/lexer                     2.1.1  3.0.1  PHP Doctrine Lexer parser library that can be used in Top-Down, Recursive Descent Parsers.
doctrine/migrations                3.8.2  3.8.2  PHP Doctrine Migrations project offer additional functionality on top of the database abstraction layer (DBAL) for versioning your database schema and easi...
doctrine/orm                       2.20.1 3.3.1  Object-Relational-Mapper for PHP
doctrine/persistence               3.4.0  4.0.0  The Doctrine Persistence project is a set of shared interfaces and functionality that the different Doctrine object mappers share.

PHP version

$ php -v
PHP 8.1.25 (cli) (built: Nov 17 2023 14:15:02) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.1.25, Copyright (c) Zend Technologies
    with Zend OPcache v8.1.25, Copyright (c), by Zend Technologies
    with Xdebug v3.2.2, Copyright (c) 2002-2023, by Derick Rethan

Subject

In version 3.18.0 an extra LIMIT clause is appended to SQL statements after the TranslationWalker. This does not happen in 3.17.1.

Steps to reproduce

use Doctrine\ORM\EntityManager;
use Doctrine\ORM\Query;
use Gedmo\Translatable\Query\TreeWalker\TranslationWalker;
use Gedmo\Translatable\TranslatableListener;
use MyProject\Entity\SystemOption;

$query = $this->entityManager->createQueryBuilder()
    ->select('s.title')
    ->from(SystemOption::class, 's')
    ->setMaxResults(10)
    ->getQuery();
$query->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, TranslationWalker::class);
$query->setHint(TranslatableListener::HINT_TRANSLATABLE_LOCALE, 'de_DE');
$query->setHint(TranslatableListener::HINT_FALLBACK, 1);
dump($query->getSQL());

Expected results

SELECT COALESCE(t1_.content, s0_.title) AS title_0
FROM SystemOption s0_
LEFT JOIN Translation t1_ ON t1_.locale = 'de_DE'
    AND t1_.field = 'title'
    AND t1_.objectClass = 'MyProject\\Entity\\SystemOption'
    AND t1_.foreignKey = s0_.id
LEFT JOIN Translation t2_ ON t2_.locale = 'de_DE'
    AND t2_.field = 'description'
    AND t2_.objectClass = 'MyProject\\Entity\\SystemOption'
    AND t2_.foreignKey = s0_.id
LIMIT 10

Actual results

SELECT COALESCE(t1_.content, s0_.title) AS title_0
FROM SystemOption s0_
LEFT JOIN Translation t1_ ON t1_.locale = 'de_DE'
    AND t1_.field = 'title'
    AND t1_.objectClass = 'MyProject\\Entity\\SystemOption'
    AND t1_.foreignKey = s0_.id
LEFT JOIN Translation t2_ ON t2_.locale = 'de_DE'
    AND t2_.field = 'description'
    AND t2_.objectClass = 'MyProject\\Entity\\SystemOption'
    AND t2_.foreignKey = s0_.id
LIMIT 10
LIMIT 10

[Pixelshaped]

Hi there,
I was going to post a similar issue. I spent some time troubleshooting the issue this weekend.
My findings is that it's not directly linked to gedmo/doctrine-extensions:3.18.0 but to doctrine/orm:3.3.x. You'll find that if you freeze your requirements to doctrine/orm:3.2.x, the problem doesn't happen.

IMO, it's linked to doctrine/orm#11188. With ORM3.3 the SqlFinalizer gets executed twice, adding LIMIT 10 twice.

I get the error too because I too am using $query->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, TranslationWalker::class);. If I don't use the hint, I don't get the error (but then I'm making numerous N+1 requests to the ext_translations table, which is not an option.

As of now doctrine-extensions is not compatible with doctrine/orm:3.3.

[mbabker]

My findings is that it's not directly linked to gedmo/doctrine-extensions:3.18.0 but to doctrine/orm:3.3.x.

Does it change any with 3.18? The new version includes #2895 which uses the new classes introduced by that ORM PR.

[xelax90]

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.

[Pixelshaped]

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.

[mbabker]

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.

[Pixelshaped]

Minimum reproducer code:

$query = $entityManager->createQuery('SELECT u FROM '.User::class.' u')
    ->setFirstResult(0)
    ->setMaxResults(5)
;

$query->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, TranslationWalker::class);

$paginator = new Paginator($query);

foreach ($paginator as $entity) { dump($entity); }

(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.

[xelax90]

I can confirm that the doctrine/orm version did not change in my project.

[Pixelshaped]

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.

ORM 3.3.1 / Extensions 3.17.1 => OK
ORM 3.3.1 / Extensions 3.18.0 => NOK!!!
ORM 3.3.0 / Extensions 3.18.0 => NOK!!!
ORM 3.2.3 / Extensions 3.18.0 => OK

ORM 2.20.1 / Extensions 3.17.1 => OK
ORM 2.20.1 / Extensions 3.18.0 => NOK!!!
ORM 2.20.0 / Extensions 3.18.0 => NOK!!!
ORM 2.19.8 / Extensions 3.18.0 => OK

Seems like it. Not sure what's the best course of action. Is it a Doctrine bug or an Extensions bug?

[mbabker]

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 Doctrine\ORM\Query\Exec\SingleSelectSqlFinalizer::finalizeSql(), the query it's being given is this (with formatting by me for readability):

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 $platform->modifyLimitQuery() it has the invalid LIMIT 1 LIMIT 1 line. I wouldn't put it past the code here having a bug given this functionality's been completely untested the whole time.

(On a side note maybe it'd also be a little easier to just bump the ORM dependency to ^2.20 || ^3.3 so some of the compat layer complexity can go away because the walkers are now trying to conditionally do stuff differently for ORM 2.14-2.19 and 3.0-3.2 versus 2.20 and 3.3 to implement the newer API and maybe that's just too much complexity)

[Pixelshaped]

With gedmo/doctrine-extensions 3.17.1, doModifyLimitQuery() is called twice:

AbstractMySQLPlatform.php:51, Doctrine\DBAL\Platforms\AbstractMySQLPlatform->doModifyLimitQuery()
AbstractPlatform.php:2092, Doctrine\DBAL\Platforms\AbstractPlatform->modifyLimitQuery()
SingleSelectSqlFinalizer.php:36, Doctrine\ORM\Query\Exec\SingleSelectSqlFinalizer->finalizeSql()
SqlWalker.php:502, Doctrine\ORM\Query\SqlWalker->walkSelectStatement()
TranslationWalker.php:146, Gedmo\Translatable\Query\TreeWalker\TranslationWalker->doWalkSelectStatementWithCompat()
SqlWalkerCompatForOrm3.php:50, Gedmo\Translatable\Query\TreeWalker\TranslationWalker->walkSelectStatement()
SingleSelectExecutor.php:21, Doctrine\ORM\Query\Exec\SingleSelectExecutor->__construct()
TranslationWalker.php:141, Gedmo\Translatable\Query\TreeWalker\TranslationWalker->doGetExecutorWithCompat()
SqlWalkerCompatForOrm3.php:42, Gedmo\Translatable\Query\TreeWalker\TranslationWalker->getExecutor()
Parser.php:375, Doctrine\ORM\Query\Parser->parse()
Paginator.php:232, Doctrine\ORM\Tools\Pagination\Paginator->unbindUnusedQueryParams()
Paginator.php:122, Doctrine\ORM\Tools\Pagination\Paginator->getIterator()

AbstractMySQLPlatform.php:51, Doctrine\DBAL\Platforms\AbstractMySQLPlatform->doModifyLimitQuery()
AbstractPlatform.php:2092, Doctrine\DBAL\Platforms\AbstractPlatform->modifyLimitQuery()
SingleSelectSqlFinalizer.php:36, Doctrine\ORM\Query\Exec\SingleSelectSqlFinalizer->finalizeSql()
SqlWalker.php:502, Doctrine\ORM\Query\SqlWalker->walkSelectStatement()
TranslationWalker.php:146, Gedmo\Translatable\Query\TreeWalker\TranslationWalker->doWalkSelectStatementWithCompat()
SqlWalkerCompatForOrm3.php:50, Gedmo\Translatable\Query\TreeWalker\TranslationWalker->walkSelectStatement()
SingleSelectExecutor.php:21, Doctrine\ORM\Query\Exec\SingleSelectExecutor->__construct()
TranslationWalker.php:141, Gedmo\Translatable\Query\TreeWalker\TranslationWalker->doGetExecutorWithCompat()
SqlWalkerCompatForOrm3.php:42, Gedmo\Translatable\Query\TreeWalker\TranslationWalker->getExecutor()
Parser.php:375, Doctrine\ORM\Query\Parser->parse()
Query.php:248, Doctrine\ORM\Query->parse()
Query.php:717, Doctrine\ORM\Query->getSqlExecutor()
Query.php:257, Doctrine\ORM\Query->_doExecute()
AbstractQuery.php:935, Doctrine\ORM\AbstractQuery->executeIgnoreQueryCache()
AbstractQuery.php:891, Doctrine\ORM\AbstractQuery->execute()
AbstractQuery.php:725, Doctrine\ORM\AbstractQuery->getScalarResult()
Paginator.php:127, Doctrine\ORM\Tools\Pagination\Paginator->getIterator()

With gedmo/doctrine-extensions 3.18.0, doModifyLimitQuery() is called three times:

AbstractMySQLPlatform.php:51, Doctrine\DBAL\Platforms\AbstractMySQLPlatform->doModifyLimitQuery()
AbstractPlatform.php:2092, Doctrine\DBAL\Platforms\AbstractPlatform->modifyLimitQuery()
SingleSelectSqlFinalizer.php:36, Doctrine\ORM\Query\Exec\SingleSelectSqlFinalizer->finalizeSql()
SqlWalker.php:502, Doctrine\ORM\Query\SqlWalker->walkSelectStatement()
TranslationWalker.php:163, Gedmo\Translatable\Query\TreeWalker\TranslationWalker->doWalkSelectStatementWithCompat()
SqlWalkerCompatForOrm3.php:50, Gedmo\Translatable\Query\TreeWalker\TranslationWalker->walkSelectStatement()
TranslationWalker.php:158, Gedmo\Translatable\Query\TreeWalker\TranslationWalker->doGetFinalizerWithCompat()
CompatSqlOutputWalkerForOrm3.php:29, Gedmo\Tool\ORM\Walker\CompatSqlOutputWalker->getFinalizer()
Parser.php:363, Doctrine\ORM\Query\Parser->parse()
Paginator.php:232, Doctrine\ORM\Tools\Pagination\Paginator->unbindUnusedQueryParams()
Paginator.php:122, Doctrine\ORM\Tools\Pagination\Paginator->getIterator()

AbstractMySQLPlatform.php:51, Doctrine\DBAL\Platforms\AbstractMySQLPlatform->doModifyLimitQuery()
AbstractPlatform.php:2092, Doctrine\DBAL\Platforms\AbstractPlatform->modifyLimitQuery()
SingleSelectSqlFinalizer.php:36, Doctrine\ORM\Query\Exec\SingleSelectSqlFinalizer->finalizeSql()
SqlWalker.php:502, Doctrine\ORM\Query\SqlWalker->walkSelectStatement()
TranslationWalker.php:163, Gedmo\Translatable\Query\TreeWalker\TranslationWalker->doWalkSelectStatementWithCompat()
SqlWalkerCompatForOrm3.php:50, Gedmo\Translatable\Query\TreeWalker\TranslationWalker->walkSelectStatement()
TranslationWalker.php:158, Gedmo\Translatable\Query\TreeWalker\TranslationWalker->doGetFinalizerWithCompat()
CompatSqlOutputWalkerForOrm3.php:29, Gedmo\Tool\ORM\Walker\CompatSqlOutputWalker->getFinalizer()
Parser.php:363, Doctrine\ORM\Query\Parser->parse()
Query.php:248, Doctrine\ORM\Query->parse()
Query.php:717, Doctrine\ORM\Query->getSqlExecutor()
Query.php:257, Doctrine\ORM\Query->_doExecute()
AbstractQuery.php:935, Doctrine\ORM\AbstractQuery->executeIgnoreQueryCache()
AbstractQuery.php:891, Doctrine\ORM\AbstractQuery->execute()
AbstractQuery.php:725, Doctrine\ORM\AbstractQuery->getScalarResult()
Paginator.php:127, Doctrine\ORM\Tools\Pagination\Paginator->getIterator()

AbstractMySQLPlatform.php:51, Doctrine\DBAL\Platforms\AbstractMySQLPlatform->doModifyLimitQuery()
AbstractPlatform.php:2092, Doctrine\DBAL\Platforms\AbstractPlatform->modifyLimitQuery()
SingleSelectSqlFinalizer.php:36, Doctrine\ORM\Query\Exec\SingleSelectSqlFinalizer->finalizeSql()
SingleSelectSqlFinalizer.php:56, Doctrine\ORM\Query\Exec\SingleSelectSqlFinalizer->createExecutor()
ParserResult.php:106, Doctrine\ORM\Query\ParserResult->prepareSqlExecutor()
Query.php:717, Doctrine\ORM\Query->getSqlExecutor()
Query.php:257, Doctrine\ORM\Query->_doExecute()
AbstractQuery.php:935, Doctrine\ORM\AbstractQuery->executeIgnoreQueryCache()
AbstractQuery.php:891, Doctrine\ORM\AbstractQuery->execute()
AbstractQuery.php:725, Doctrine\ORM\AbstractQuery->getScalarResult()
Paginator.php:127, Doctrine\ORM\Tools\Pagination\Paginator->getIterator()

In the end the problematic test is Doctrine\ORM\Query\Parser:362.

if ($outputWalker instanceof OutputWalker) {

If you replace it by if (false) { => legacy behavior is restored. So one way to temporarily restore compatibility would be to revert #2895 until this behavior is correctly tested and the problem fixed?

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?

[mbabker]

If you need a hot patch, changing CompatSqlOutputWalker so it never extends the newer SqlOutputWalker class should be enough for an application for the time being since that should effectively do the same as a revert.

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 SqlOutputWalker class from the ORM so it's definitely either an edge case upstream or an incomplete patch on my end

[mbabker]

#2920 should fix this.