Add a AddParameters method in the QueryBuilder

[Taluu]

Hi,

On doctrine 2.2, when we were calling the method setParameters to set a bunch of parameters on the QueryBuilder, each parameters were added or modified if they were already existing.

That is not the case since doctrine 2.3 : each calls to setParameters will wipe out the other already setted parameters. I think it is logic, but still is a huge BC break, which is not really mentionned in the UPGRADE file (only the one regarding the use of an ArrayCollection is mentionned).

With this PR, I added a addParameters which allows to add several parameters in several calls. Here is a use case :

<?php
// ... A repository
class UserRepository extends EntityRepository
{
     public function getUserWithSomething($criteriaA, $criteriaB, DateTime $since = null)
     {
         $qb = $this->createQueryBuilder('u');
         $qb->where($qb->expr()->andX($qb->expr()->eq('u.criteriaA', ':criteriaA'),
                                      $qb->expr()->eq('u.criteriaB', ':criteriaB')));

         // wanna search for objects since a specific date
         if (null !== $since) {
             $qb = $this->addSince($qb, $since);
         }

         $qb->addParameters(new ArrayCollection(['criteriaA' => $criteriaA, 
                                                 'criteriaB' => $criteriaB]));  

         return $qb->getQuery()->execute();
     }

     public function addSince(QueryBuilder $qb, DateTime $since)
     {
         $qb = $qb->andWhere($qb->expr()->gte('u.date', ':since'));

         return $qb->setParameter('since', $since);
     }
}

So, as I was saying, until 2.2, you could use setParameters in the main method (getUserWithSomething) which was calling (or not) the submethod addSince, which could set parameters before calling the setParameters method. Now, you can't do that anymore : either you need to make several calls to setParameter :

<?php
// ....
     $qb = $qb->setParameter('criteriaA', $criteriaA)
              ->setParameter('criteriaB', $criteriaB); 
//...

Either, I guess, you need to first retrieve all the parameters and then add them by hand :

<?php
// ....
     $parameters = $qb->getParameters();
     $parameters->add(new Parameter('criteriaA', $criteriaA);
     $parameters->add(new Parameter('criteriaB', $criteriaB);

     $qb = $qb->setParameters($parameters);
//...

So that's why I propose this new method in the QueryBuilder. if I'm wrong anywhere, or need more information please feel free to comment. :)

Cheers.

edit : If I tried to use either a ArrayCollection or an array in the parameters, it's because I tried to keep in mind the BC breeaks for 2.2 and below branches, and if I am converting the Parameter into a simple array, it is to ease the use for setParameter.

[doctrinebot]

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-2400

We use Jira to track the state of pull requests and the versions they got
included in.

[beberlei]

@guilhermeblanco can you say something here? You refactored the parameter handling.

[guilhermeblanco]

@Taluu Method setParameters should always override the list. It was a bug in 2.2 which was resolved in 2.3.
Before that, there was no way to wipe parameters and the parameter handling refactored allowed this possibility.
You were relying on a broken behavior (a bug) and surely it would be fixed one day. It was done in this release and that's why you're suffering this issue now.
What it seems to miss with 2.3 release was a note in UPGRADE file.
If you don't want to make multiple calls to ->setParameter(), then manually creating the Parameter instance is the way to go.

@beberlei I'm against bloating the Query API even more to add another alternative way to do something it's already possible to do 2 different ways. If we really want to simplify the Query API, that indeed demands a BC break (removing ->setParameter() from AbstractQuery API and only allowing to be done through a possible ParameterCollection (subclass of ArrayCollection) class).

Closing the ticket as Query API should be simplified, not increased. I'm -1 on this.

[Taluu]

@Taluu Method setParameters should always override the list. It was a bug in 2.2 which was resolved in 2.3.
Before that, there was no way to wipe parameters and the parameter handling refactored allowed this possibility.
You were relying on a broken behavior (a bug) and surely it would be fixed one day. It was done in this release and that's why you're suffering this issue now.

Yup, that's what I thought, and also thought it was logic to do so.

But I still think there should be a simpler way to add (or rewrite) parameters, than having to manually having to create the needed Parameter. Specially in the case of changing an already existing parameter.

BTW, and just out of curiosity, are the parameters given in the execute method of the Query API erasing the already setted parameters or are they fused with it ?

edit : nevermind, I had my answer : all the parameters are wiped out if using the argument in the execute method.

[Ocramius]

@Taluu it calls setParameters internally. See https://github.com/doctrine/doctrine2/blob/2.3.3/lib/Doctrine/ORM/AbstractQuery.php#L709-L711

[guilhermeblanco]

@Taluu such method would require subclassing ArrayCollection into a possible ParameterCollection and adding handy method, like ->merge(ParameterCollection $collection) that would do the job for you.
But taking such an alternative would lead to a BC break since I'd strongly suggest removing ->setParameter() out of Query API.

[deeky666]

@guilhermeblanco I'm also +1 for a ParameterCollection class that can handle parameters in any desired way. The Query API should only take a list of paramters and not provide different ways of handling/managing parameters. This seems pretty unnatural in the Query class and should be taken care of anywhere outside.

[Taluu]

Yup, I'm also +1 on the ParameterCollection solution, and why not remove the handling of parameters through the Query API, as @deeky666 suggested.

BTW I was always wondering why the ArrayCollection never had a merge method to begin with ?

[beberlei]

@guilhermeblanco setParameter() will always be part of the API from my perspective. I dont see an alternative except getting the parameter bag then setting a value, which is a clear law of demeter violation.

[deeky666]

setParameters should take a ParameterCollection. Parameter management can then also be performed after the setParameters has been called, directly in the ParameterCollection object. So you would only have to notice the Query about a certain ParameterCollection object, nothing else.