Handle removed parameters by tree walker in Paginator

[plfort]

There is a bug introduced in #6820 (my bad)
Given this code :

$query = $em->createQuery('SELECT a.id, (SELECT b.id FROM EntityB b WHERE b.id = :paramB) AS foo FROM EntityA a ORDER BY a.id');

$query->setParameter('paramB', 123);
$paginator = new Paginator($query);
$paginator->setUseOutputWalkers(false);
$paginator->getIterator();

The resulting subquery in the Paginator will look like this :

SELECT DISTINCT a.id FROM EntityA a ORDER BY a.id

The query is OK but the parameter 'paramB' is still binded to the query, causing a "tooManyParameters" exception.

Edit: I don't really know how to test this behaviour ...

[Ocramius]

I didn't even know that we allowed subqueries in the SELECT clause - that seems already like a problem to me.

[plfort]

Why do you think it is a problem ?

[Ocramius]

Mostly because the hydration process doesn't consider it, nor we have tests for it AFAIK

[ostrolucky]

Subselects are supported by design, we even have Subselect node and Parser method. We also do have tests for it https://github.com/doctrine/doctrine2/blob/ce1e3250b4bea05dc0c7a4ed09a6fb05e21705d6/tests/Doctrine/Tests/ORM/Functional/QueryTest.php#L701 and it's pretty widely used.

[Ocramius]

Hmm, interesting - fairly sure that subselects were never allowed in the SELECT clause before, so I probably completely missed when this change happened :|

[plfort]

It is also documented on the DQL page : https://www.doctrine-project.org/projects/doctrine-orm/en/2.6/reference/dql-doctrine-query-language.html#clauses

[ostrolucky]

Test case should be added

[ostrolucky]

Method should be named in way this comment is not needed. How about unbindUnusedQueryParams

[plfort]

You're right but I don't know how to test it ... I'd like to mock the Paginator and test that the method unbindUnusedQueryParams is called and test the method itself but how can I do it due to its private accessibility ?

[ostrolucky]

Yeah all pagination tests are currently functional ones. I will try to help you create unit test for this. But if my effort stalls, just create another functional one, similar like this one https://github.com/doctrine/doctrine2/blob/ce1e3250b4bea05dc0c7a4ed09a6fb05e21705d6/tests/Doctrine/Tests/ORM/Functional/PaginationTest.php#L567-L575

Btw test shouldn't just assert working of unbindUnusedQueryParams, but rationalize it. This means it should assert there is either no exception thrown when query is being executed, or count of paramMappings matches count of parameters, basically same as is done here https://github.com/doctrine/doctrine2/blob/ce1e3250b4bea05dc0c7a4ed09a6fb05e21705d6/lib/Doctrine/ORM/Query.php#L309-L315

[plfort]

@ostrolucky thank you for your help

[ostrolucky]

I've replaced your test with unit one, hopefully you don't mind. @Ocramius can you have a look?

addressed

[Ocramius]

Overall, patch seems fine. There is one left problem that needs to be verified, which is a query like following:

SELECT f FROM Foo f WHERE f.foo = :foo

with bound parameters:

[
    'foo' => 'foo',
    'bar' => 'bar',
];

This kind of parameter binding is invalid: would the paginator make it valid because of the automatically removed parameters?

[Ocramius]

Let's please never ever ever ever use compact :-)

[ostrolucky]

So @Majkl578 can you add it to forbidden functions in CS standard? I was careful in not introducing any new violations.

[Ocramius]

I think it is there: https://github.com/doctrine/coding-standard/blob/6432a4e17ef37199794559e86eddb05b519832c0/lib/Doctrine/ruleset.xml#L59

[ostrolucky]

Ah it's because 2.6 is locked to version 1

https://github.com/doctrine/doctrine2/blob/f1143f591f1bfcda269141759a20d117e54217b3/composer.json#L30

Any particular reason for that?

[Ocramius]

Since this is repeated only 2 times, repeating the expects() is OK

[ostrolucky]

So what's the general rule when to duplicate code and when to put it to loop? 3 time repeat?

[Ocramius]

It's a loop done for 2 iterations - seems easier to read with explicit at(0) at(1), at(2)

[Ocramius]

Note that exactly, any, at and such are static methods

[ostrolucky]

Asserts as well

[ostrolucky]

This kind of parameter binding is invalid: would the paginator make it valid because of the automatically removed parameters?

Probably yes. Doesn't seem worth it to me to add extra logic to deal with such edge case though.

[Ocramius]

Well, I'd still expect a test that either expects an exception or just runs regardless of bound parameters :-)

This is just to prevent regressions btw.

[ostrolucky]

@plfort I have changed your patch to only strip extra params if there is LimitSubqueryWalker or CountWalker used, as only these strip out select statements. Is that ok?

I have also added tests which deal with edge case @Ocramius suggested. Dealing with that more correctly might be by moving such logic directly into Parser, instead of in Query#_doExecute()

addressed

[plfort]

Absolutely 👍

[ostrolucky]

bump @Ocramius

[Ocramius]

@ostrolucky can you merge this to master and to 2.6 please?