New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DQL Query: process ArrayCollection values to ease development #590

Merged
merged 1 commit into from Dec 17, 2013

Conversation

Projects
None yet
9 participants
@michaelperrin
Contributor

michaelperrin commented Feb 25, 2013

I added some code to ease "where in" parameter binding.

As you know, when a where condition is added, the object itself can be passed as a parameter and it's id is automatically retrieved:

$queryBuilder = $this
    ->where('model.category = :category')
    ->setParameter('category', $category)
;

Where $category is an object.

But it doesn't work for collections:

$queryBuilder = $this
    ->where('model.category IN (:categories)')
    ->setParameter('categories', $categories)
;

Where categories is an ArrayCollection object (retrieved from a many to one relation for instance).

This doesn't work in the current version of Doctrine, and my PR solves that.

Note that I didn't add any unit test for this feature. Can you explain me where I should add the test?

This enhancement is now unit-tested in this same PR.

So far, the only solution was to do the following, which is pretty borring:

$categoryIds = array();

foreach ($categories as $category) {
    $categoryIds[] = $category->getId();
}

$queryBuilder = $this
    ->where('model.category IN (:category_ids)')
    ->setParameter('category_ids', $categoryIds)
;
@doctrinebot

This comment has been minimized.

doctrinebot commented Feb 25, 2013

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-2319

@stof

View changes

lib/Doctrine/ORM/AbstractQuery.php Outdated
$values = array();
foreach ($value as $valueEntry) {
$singleValue = $this->_em->getUnitOfWork()->getSingleIdentifierValue($valueEntry);

This comment has been minimized.

@stof

stof Feb 25, 2013

Member

What if it does not have a single identifier ?

This comment has been minimized.

@michaelperrin

michaelperrin Feb 25, 2013

Contributor

@stof It's tested on the line below and an Exception is thrown, this is the same behavior for the object case (see line 283 of the same file)

This comment has been minimized.

@stof

stof Feb 25, 2013

Member

What I mean is that entities using a composite identifier would throw an exception here because getSingleIdentifierValue is not supported for them (it cannot be a single one)

@stof

View changes

lib/Doctrine/ORM/AbstractQuery.php Outdated
@@ -284,6 +284,22 @@ public function processParameterValue($value)
}
}
if ($value instanceof ArrayCollection) {

This comment has been minimized.

@stof

stof Feb 25, 2013

Member

This would not solve your use case as a relation is likely to give you a PersistentCollection, not an ArrayCollection

@michaelperrin

This comment has been minimized.

Contributor

michaelperrin commented Feb 25, 2013

@stof I updated the code according to the comments you made. If you think the feature is worth being merged, I'll write tests associated to it.

@michaelperrin

This comment has been minimized.

Contributor

michaelperrin commented Feb 26, 2013

@stof Sorry for the misunderstanding. You're right, an exception (ORMInvalidArgumentException) is thrown if one of the collection entities has a composite identifier. That's actually the same when a single entity is bound, see line 281 of the AbstractQuery class:

$value = $this->_em->getUnitOfWork()->getSingleIdentifierValue($value);

When binding object parameters, developers will know (and already know with the current code) that only object having a single identifier can be used thanks to this raised exception.

I added by the way more control on the collection entities and added tests as well for this enhancement.

@FabioBatSilva

View changes

lib/Doctrine/ORM/AbstractQuery.php Outdated
@@ -284,6 +285,26 @@ public function processParameterValue($value)
}
}
if ($value instanceof Collection) {

This comment has been minimized.

@FabioBatSilva

FabioBatSilva Feb 26, 2013

Member

Something like this might be easier to read :

if ($value instanceof Collection) {
    $values = array();

    foreach ($value as $valueEntry) {

        if ( ! is_object($valueEntry) || ! $this->_em->getMetadataFactory()->hasMetadataFor(ClassUtils::getClass($valueEntry))) {
            return $value;
        }

        $singleValue = $this->_em->getUnitOfWork()->getSingleIdentifierValue($valueEntry);

        if ($singleValue === null) {
            throw ORMInvalidArgumentException::invalidIdentifierBindingEntity();
        }

        $values[] = $singleValue;
    }

    return $values;
}

This comment has been minimized.

@beberlei

beberlei Mar 12, 2013

Member

Could you please move the block inside if() into a method processCollectionParameterValue()? This would avoid the method to become that long.

After that its good to merge from my POV.

This comment has been minimized.

@michaelperrin

michaelperrin Mar 13, 2013

Contributor

@beberlei This is now done!

@michaelperrin

This comment has been minimized.

Contributor

michaelperrin commented Feb 26, 2013

@FabioBatSilva Done (small condition refactoring)

@michaelperrin

This comment has been minimized.

Contributor

michaelperrin commented Apr 25, 2013

Rebased commits (no more conflicts with current master branch)

foreach ($value as $valueEntry) {
if ( ! is_object($valueEntry) || ! $this->_em->getMetadataFactory()->hasMetadataFor(ClassUtils::getClass($valueEntry))) {
return $value;

This comment has been minimized.

@Baachi

Baachi Apr 26, 2013

Sure? I think it must be:

        foreach ($value as $valueEntry) {
            if ( ! is_object($valueEntry) || ! $this->_em->getMetadataFactory()->hasMetadataFor(ClassUtils::getClass($valueEntry))) {
                $values[] = $value;
                continue;
@michaelperrin

This comment has been minimized.

Contributor

michaelperrin commented Apr 26, 2013

@Baachi Hum, you're right, that's better adding the value itself in case the value in the collection parameter is not an object. That's fixed now.

foreach ($value as $valueEntry) {
if ( ! is_object($valueEntry) || ! $this->_em->getMetadataFactory()->hasMetadataFor(ClassUtils::getClass($valueEntry))) {
$values[] = $value;

This comment has been minimized.

@FabioBatSilva

FabioBatSilva Apr 26, 2013

Member

This is wrong,
You should add a continue; here..
Otherwise you are adding the value and then trying to extract the identifier which will probably fail ..

Please add a test for this case as well..

@goetas

This comment has been minimized.

Contributor

goetas commented Apr 26, 2013

Is compatible with #522 ?

@@ -288,10 +289,45 @@ public function processParameterValue($value)
return $value->name;
}
if ($value instanceof Collection) {

This comment has been minimized.

@beberlei

beberlei Apr 27, 2013

Member

We are not always passing around objects in collections, and collections musn't have only objects. I am not sure if this is a helpful addition for users and as you show in the test, the API actually requires quite some code to instantiate and add elements to the collection, whereas i could just do:

$query->setParameter('foo', array_map(function ($object) {
    return $object->getId();
}, array($a, $b));

This is much simpler and generic.

This comment has been minimized.

@michaelperrin

michaelperrin Apr 27, 2013

Contributor

@beberlei The case where objects and values can be mixed in the same collection is now handled and unit-tested. And while it requires some code if the collection is built manually, it's not the case when the collection has been generated by Doctrine (like when retrieving a one to many value on an object) and can later directly be used as a query parameter with the solution I propose. What do you think?

@michaelperrin

This comment has been minimized.

Contributor

michaelperrin commented Apr 27, 2013

@FabioBatSilva Aw, my mistake, sorry. This is now fixed, and unit tested.

@beberlei

This comment has been minimized.

Member

beberlei commented May 4, 2013

Just checked the code again, please remove the processcollectionParamterValue() and just add a check for instanceof Collection in the line if (is_array($value)) right at the top. This does exactly the same and only needs one line change.

@michaelperrin

This comment has been minimized.

Contributor

michaelperrin commented May 6, 2013

@beberlei This is indeed a more clever way to implement it indeed and this is now done with the last commit.

I first convert the collection to an array as the processParameterValue method would return a Collection instead, which is not handled correctly.
An other way would be to do the instanceof Collection test and the conversion to an array just before returning the value in the if (is_array($value)) condition. There might be other solutions, but this is my first PR on Doctrine, so I prefer to keep things simple.

Anyway, this solution works fine!

Thanks for your help!

@michaelperrin

This comment has been minimized.

Contributor

michaelperrin commented Sep 19, 2013

Hello,

Do you have any news on the merge of this PR?
I can make further changes if needed.

@michaelperrin

This comment has been minimized.

Contributor

michaelperrin commented Dec 17, 2013

@beberlei I rebased my commits and resolved conflicts so that it can be merged to master.

@asm89

This comment has been minimized.

Member

asm89 commented Dec 17, 2013

@michaelperrin Can you squash your PR into 1 commit?

@michaelperrin

This comment has been minimized.

Contributor

michaelperrin commented Dec 17, 2013

@asm89 Done!

@guilhermeblanco

This comment has been minimized.

Member

guilhermeblanco commented Dec 17, 2013

Waiting for Travis to respond. It failed temporarily and manually triggered a new build.

guilhermeblanco added a commit that referenced this pull request Dec 17, 2013

Merge pull request #590 from michaelperrin/feature/process-collection…
…-value

DQL Query: process ArrayCollection values to ease development

@guilhermeblanco guilhermeblanco merged commit 423ea00 into doctrine:master Dec 17, 2013

1 check passed

default The Travis CI build passed
Details
@michaelperrin

This comment has been minimized.

Contributor

michaelperrin commented Dec 18, 2013

Thanks @guilhermeblanco !

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