Skip to content

[DDC-1840] Implemented parameters as a collection. #360

Merged
merged 6 commits into from May 29, 2012

7 participants

@guilhermeblanco
Doctrine member

Breaking down the changes per method:

execute

BEFORE:

$query = $entityManager->createQuery(
    'SELECT u  FROM User u WHERE u.id = :id'
);
$result = $query->execute(array('id' => 1));

AFTER:

$query = $entityManager->createQuery(
    'SELECT u  FROM User u WHERE u.id = :id'
);
$result = $query->execute(new ArrayCollection(array(new Parameter('id', 1))));

setParameters

BEFORE:

$query = $entityManager->createQuery(
    'SELECT u  FROM User u WHERE u.id = :id AND u.status = :status'
);
$query->setParameters(array(
    'id'     => 1,
    'status' => 'active'
));
$result = $query->getResult();

AFTER:

$query = $entityManager->createQuery(
    'SELECT u  FROM User u WHERE u.id = :id AND u.status = :status'
);
$query->setParameters(new ArrayCollection(array(
    new Parameter('id', 1),
    new Parameter('status', 'active')
));
$result = $query->getResult();

getParameter

BEFORE:

$query = $entityManager->createQuery(
    'SELECT u  FROM User u WHERE u.id = :id AND u.status = :status'
);
$query->setParameters(array(
    'id'     => 1,
    'status' => 'active'
));

$parameter = $query->getParameter('id');

// $parameter === 1
// $query->getParameterType('id') === PDO::PARAM_INT

AFTER:

$query = $entityManager->createQuery(
    'SELECT u  FROM User u WHERE u.id = :id AND u.status = :status'
);
$query->setParameters(new ArrayCollection(array(
    new Parameter('id', 1),
    new Parameter('status', 'active')
));


$parameter = $query->getParameter('id');

// $parameter->getName() === 'id'
// $parameter->getValue() === 1
// $parameter->getType() === \PDO::PARAM_INT

Related changes:

BEFORE:

$user = $entityManager->getReference('User', 1);

$query->setParameter(1, $user);

// $query->getParameter(1) === 1

AFTER

$user = $entityManager->getReference('User', 1);

$query->setParameter(1, $user);

// $query->getParameter(1) instanceof Doctrine\ORM\Query\Parameter

$parameter = $query->getParameter(1);

// $parameter->getValue() === $user
@travisbot

This pull request fails (merged 1635e0a into 1f2ce21).

@Koc
Koc commented May 28, 2012

For what this needed?

@beberlei
Doctrine member

Its cleanup of the parameter handling, because you cannot reset parametrs and also its hard to work with existing parameers and modify them. Advanced Query Building use-cases.

@guilhermeblanco How about making it BC by allowing arrays and having a method to convert arrays into ArrayCollection?

@travisbot

This pull request fails (merged 79ff1f1 into 1f2ce21).

@travisbot

This pull request fails (merged d8e165d into 1f2ce21).

@guilhermeblanco
Doctrine member

@beberlei I'd avoid adding magical transformations internally.
Specially because the BC break is already made (between key => value to a Parameter instance). So we can live with the complete change since we're already enforcing people to update 90% of it.

@schmittjoh
Doctrine member

Could you add some before/after examples?

@guilhermeblanco
Doctrine member

@schmittjoh updated and added examples of change impact =)

@Koc
Koc commented May 28, 2012

So after this changes methods like cloneQuery will save binded types?

@guilhermeblanco
Doctrine member

@Koc preliminary inference, yes.
All parameters are double inferred if the processed parameter value changed from the original (for example, an array transformation or a proxy to id transformation).

@travisbot

This pull request fails (merged b3e7493 into 1f2ce21).

@guilhermeblanco
Doctrine member

@travisbot don't tell me this PR fails because you're lying! =P
The build is not passing because of a DBAL issue for generating cache keys which was not modified by my code.

@travisbot

This pull request fails (merged 161ae31 into 1f2ce21).

@guilhermeblanco guilhermeblanco merged commit e4935e5 into master May 29, 2012
@travisbot

This pull request fails (merged 15f76c6 into 1f2ce21).

@stof stof commented on the diff May 29, 2012
lib/Doctrine/ORM/AbstractQuery.php
@@ -697,18 +696,18 @@ public function iterate(array $params = array(), $hydrationMode = null)
/**
* Executes the query.
*
- * @param array $params Any additional query parameters.
+ * @param \Doctrine\Common\Collections\ArrayCollection|array $parameters Query parameters.
@stof
Doctrine member
stof added a note May 29, 2012

shouldn't the doc mention the interface instead ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on the diff May 29, 2012
lib/Doctrine/ORM/AbstractQuery.php
- foreach ($params AS $key => $value) {
- $params[$key] = $this->processParameterValue($value);
+ foreach ($this->getParameters()->getIterator() as $parameter) {
@stof
Doctrine member
stof added a note May 29, 2012

->getIterator() is not needed here thanks to \IteratorAggregate on the collection

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on the diff May 29, 2012
lib/Doctrine/ORM/NativeQuery.php
- if ($params && is_int(key($params))) {
- ksort($params);
+ foreach ($this->getParameters()->getIterator() as $parameter) {
@stof
Doctrine member
stof added a note May 29, 2012

same here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on the diff May 29, 2012
lib/Doctrine/ORM/Query.php
@@ -269,17 +272,23 @@ protected function _doExecute()
*/
private function processParameterMappings($paramMappings)
{
- $sqlParams = $types = array();
+ $sqlParams = array();
+ $types = array();
+
+ foreach ($this->parameters->getIterator() as $parameter) {
@stof
Doctrine member
stof added a note May 29, 2012

same here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on the diff May 29, 2012
lib/Doctrine/ORM/QueryBuilder.php
* @return QueryBuilder This QueryBuilder instance.
*/
- public function setParameters(array $params, array $types = array())
+ public function setParameters(ArrayCollection $parameters)
@stof
Doctrine member
stof added a note May 29, 2012

you should typehint the interface

@stof
Doctrine member
stof added a note May 29, 2012

btw, why not allowing an array here whereas the query allows it ? It is inconsistent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on the diff May 29, 2012
lib/Doctrine/ORM/QueryBuilder.php
* @return mixed The value of the bound parameter.
*/
public function getParameter($key)
{
- return isset($this->_params[$key]) ? $this->_params[$key] : null;
+ foreach ($this->parameters->getIterator() as $parameter) {
@stof
Doctrine member
stof added a note May 29, 2012

no need of the explicit getIterator()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof
Doctrine member
stof commented May 29, 2012

btw, I think the pagination might have side effects now: it clones the query and set the parameters again, but the collection will be set by reference in the cloned query now as it is an object whereas it was copied previously when using an array

@umpirsky

This commit produced a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.