Invalid paginator query cloning #392

Merged
merged 3 commits into from Jul 8, 2012

4 participants

@egeloen

Hey!

I'm currently using Symfony 2.1 + Doctrine master branch + Pager fanta master branch.

I'm facing a vicious issue.

The doctrine paginator is able to clone a query with his cloneQuery method. This method will only clone the query without this parameters & hints. The issue is the parameters is setted with the setParameters method which will only affect the ArrayCollection reference to the new query and so, shared the reference between the two queries.

This PR will clone the ArrayCollection instead of simply affect it.

Let me know if you need more information. :)

@stof stof commented on an outdated diff Jul 6, 2012
lib/Doctrine/ORM/Tools/Pagination/Paginator.php
@@ -208,7 +208,11 @@ private function cloneQuery(Query $query)
{
/* @var $cloneQuery Query */
$cloneQuery = clone $query;
- $cloneQuery->setParameters($query->getParameters());
+
+ foreach ($query->getParameters() as $parameter) {
+ $cloneQuery->setParameter($parameter->getName(), $parameter->getValue(), $parameter->getType());
+ }
+
@stof
Doctrine member
stof added a note Jul 6, 2012

why not simply $cloneQuery->setParameters(clone $query->getParameters()); instead of looping ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@travisbot

This pull request fails (merged 996e47b into 0a95e42).

@stof
Doctrine member

Please also add a test to avoid regressions

@egeloen

@stof I have updated my code

I have take a look to the pagination tests but I don't know where I can add mine.

@travisbot

This pull request fails (merged cc8d8c7d into 0a95e42).

@travisbot

This pull request fails (merged 8c24e52 into 0a95e42).

@beberlei
Doctrine member

Put a test in Doctrine\ORM\Tests\Functional\PaginationTest

@travisbot

This pull request fails (merged 3bdf869 into 0a95e42).

@egeloen

@stof @beberlei I have added a test

@beberlei
Doctrine member

I don't think the test is enough, isEmpty() is true anyways. You have to check that a query with parameters now doesnt produce the error you had before.

@stof
Doctrine member

@beberlei without the clone, it would not be empty in the original query as the paginated query adds more arguments.

@egeloen

@beberlei Like @stof explains, my test is valid. The paginator will clone the query and set new parameters on it. If the original query parameters is not cloned, they are affected by the internal paginator affectation.

@beberlei beberlei merged commit fe292cc into doctrine:master Jul 8, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment