Added addParameters() to Query and QueryBuilder #512

Closed
wants to merge 3 commits into
from

Projects

None yet

5 participants

@jaspernbrouwer
Contributor

This method behaves like setParameters() before version 2.3:
It will add new parameters to the collection, and override any existing positions/names.

It can take a Doctrine\Common\Collections\ArrayCollection with Doctrine\ORM\Query\Parameter objects, as well as a plain array with key/value pairs, as argument.

This will greatly ease the upgrade to Doctrine 2.3, because you only need to perform a project-wide replace of setParameters with addParameters, in stead of going into your code and determine if calls to setParameters are ok or need refactoring.

I've also added unit-tests to maintain integrity.

@jaspernbrouwer jaspernbrouwer Added addParameters() to Query and QueryBuilder
This method behaves like setParameters() before version 2.3
fda9697
@doctrinebot

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DDC-2140

@henrikbjorn henrikbjorn and 1 other commented on an outdated diff Nov 13, 2012
lib/Doctrine/ORM/AbstractQuery.php
@@ -241,6 +241,47 @@ function ($parameter) use ($key)
}
/**
+ * Adds a collection of query parameters, overriding any existing positions/names.
+ * Behaves like setParameters() before 2.3.
+ *
+ * @param \Doctrine\Common\Collections\ArrayCollection|array $parameters
+ *
+ * @return \Doctrine\ORM\AbstractQuery This query instance.
+ */
+ public function addParameters($parameters)
+ {
+ $currentNames = $this->parameters->map(
+ function ($parameter)
+ {
+ return $parameter->getName();
+ }
+ )->toArray();
@henrikbjorn
henrikbjorn Nov 13, 2012

Coding style is wrong here.

@jaspernbrouwer
jaspernbrouwer Nov 14, 2012 Contributor

I fixed this.

@stof stof commented on an outdated diff Nov 15, 2012
lib/Doctrine/ORM/AbstractQuery.php
+ function ($parameter)
+ {
+ return $parameter->getName();
+ }
+ );
+ $currentNames = $currentNames->toArray();
+
+ foreach ($parameters as $key => $value) {
+ if ($value instanceof Query\Parameter) {
+ // $value is a Query\Parameter
+ $parameter = $value;
+
+ if (false !== ($index = array_search($parameter->getName(), $currentNames))) {
+ $this->parameters[$index]->setValue( $parameter->getValue(), $parameter->getType() );
+ } else {
+ $this->parameters->add( $parameter );
@stof
stof Nov 15, 2012 Member

please remove the spaces inside braces for function calls

@stof stof commented on an outdated diff Nov 15, 2012
tests/Doctrine/Tests/ORM/Query/QueryTest.php
@@ -7,6 +7,8 @@
use Doctrine\ORM\Query\Parameter;
+require_once __DIR__ . '/../../TestInit.php';
@stof
stof Nov 15, 2012 Member

This should be removed

@jaspernbrouwer jaspernbrouwer Fixed code style
Also removed require_once from test.
0ef76f8
@guilhermeblanco
Member

I don't feel strong need to have such an API.
If you want to keep track and merge collection of parameters, all you have to do is create an array (or an ArrayCollection), manipulate the instance and then setParameters at the end.
Unless you give me a stronger argument, this code won't be in. Closing for now.

@jaspernbrouwer
Contributor

Hi Guilherme,

I agree that such a method makes less sense in Query, because when you write a DQL string all parameters are known at once. But when using the QueryBuilder you might need different parameters in different cases, so addParameters() becomes useful there.

I guess it's just a convenience method, like IMHO setParameter() is. (You could just do $qb->getParameters()->add())

The main reason for adding the method was, like I said, upgrading to Doctrine 2.3. I've already upgraded all my projects to Doctrine 2.3, so the method is less useful for me now. But it took me a full day to refactor my repositories, because there is no safe way to automate the process. A simple search-and-replace setParameters() to addParameters() would have taken me 5 minutes ;)

I'm content with your decision.
If others find addParameters() useful, I hope they let us know.

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