DDC-3016 Support using embeddables in criterias with ArrayCollection

[mnapoli]

Fix for: DDC-3016 - Criterias do not work with embeddables when matching in memory.

---

When using criterias and doing the matching on a collection already loaded in memory, it will not work if filtering on embeddable objects.

Example:

$criteria = new Criteria();
$criteria->andWhere($criteria->expr()->eq('actions.view', true));

$authorizations = $this->authorizations->matching($criteria);

Here the ClosureExpressionVisitor will try to get the property named ->actions.view instead of ->actions->view.

PHPUnit_Framework_Error_Notice : Undefined property: Tests\...\Model\ArticleAuthorization::$actions.view

Criterias in PersistentCollection work perfectly with embeddables, so it makes sense to have ArrayCollection also support it (else there's a mismatch between both implementations).

[stof]

I see an issue here: this means that matching in memory now supports ToOne relations as well, which is not supported when matching in SQL

[mnapoli]

Yes I was expecting someone to point this out :/

Given there is no notion of Embedded in doctrine/common, we can't differentiate between embedded and associations… In the end it's either:

1. we can match embedded in SQL, but not in memory
2. we can match embedded in SQL and in memory, and we can match associations in memory but not in SQL

I would much rather go down the 2nd solution of course, because that's the most feature-full. But either way there will be a mismatch.

We could document explicitly not to use "matching" with associations because it won't work with SQL.

[Ocramius]

@mnapoli here's my idea for matching objects over associations in both ORM and SQL:

$criteria = new Criteria();
$addressCriteria = new Criteria();

$addressCriteria->andWhere($addressCriteria->expr()->eq('street', 'Foo Street');
$criteria->andWhere($criteria->expr()->matching('address', $addressCriteria));

$usersInFooStreet = $users->matching($criteria);

[mnapoli]

Interesting! though a bit verbose and complex but at least it's explicit

[Ocramius]

@mnapoli it would solve the problem of filtering over associations of any type ;-)

[erik-am]

I see another problem with matching Embeddables. Embeddables are almost perfect for implementing Value Objects for doing Domain-Driven Development, but strict comparisons in Doctrine make this difficult.

Example

/** Embeddable **/
class Address {
    private $street;
    private $nr;

    public function __construct($street, $nr) {
        /* optionally validating input and setting variables */
    }

    /* getters, if required */
}

/** Entity **/
class User {
    private $address;

    public function getAddress() {
        return $this->address;
    }

    public function setAddress(Address $address) {
        $this->address = $address;
    }
}

$criteria = Criteria::create()->where(
    Criteria::expr()->eq('address', new Address('High Street', 1))
);
$usersInTheSameHouse = $users->matching($criteria);

This will not work, because the equality operator will do a strict comparison. In many cases strict comparisons are desired, but not for value objects. As was said in an earlier comment, the collections do not know if they're working with an Embeddable or not, and unfortunately we cannot override an equals() function in PHP. As an alternative, is it an idea to add another operator (besides 'eq') that would do a loose comparison?

I could make a separate pull request for this and create some code and tests if you agree with me that Doctrine should support this scenario. (Just not sure how the SQL implementation would work out. It should just match all properties when comparing Embeddables; I'm not sure if that is already the case or not.)

[mnapoli]

@erik-am I agree this is a problem (and I've faced it also doing DDD). However I think it's a separate issue. This one is about matching properties of the embeddables, not the whole object. So I would say it's better to create a new issue for this to avoid getting mixed up.

[erik-am]

@mnapoli Thanks for your reply, I created a separate issue in #28.

[Ocramius]

Newline before this line

[Ocramius]

This change is also incompatible with QueryBuilder#addCriteria(), no?

[mnapoli]

@Ocramius how is it incompatible? I don't know if this feature is supported by QueryBuilder#addCriteria() (just like it is supported by PersistentCollection) but I don't see how it would introduce a regression in the QueryBuilder.

[Ocramius]

@mnapoli I usually add criteria containing dots (in field names) to my query builder...

[estelsmith]

@mnapoli @Ocramius Any news on this? I've been looking for ways to make ArrayCollection either support embeddables or allow the criteria to query sub-collections.

I'm fine with either of these ways, but I would love to see at least one of them implemented in the near future.

Option 1:

$posts = new ArrayCollection(/* list of posts */);

$categoryCriteria = Criteria::create()
    ->where(
        Criteria::expr()->eq('name', 'Test Category')
    )
;

$criteria = Criteria::create()
    ->where(
        Criteria::expr()->matches('categories', $categoryCriteria)
    )
;

$matching = $posts->matching($criteria);

Option 2:

$posts = new ArrayCollection(/* list of posts */);

$criteria = Criteria::create()
    ->where(
        Criteria::expr()->eq('categories.name', 'Test Category')
    )
;

$matching = $posts->matching($criteria);

A third option, which is easy to implement by making ClosureExpressionVisitor::walkComparison() check if the field value is a collection in the Comparison::CONTAINS closure.

I know that "contains" appears to be reserved for string comparisons, but the phrase "contains" applies to collections, as well.

I'm just not sure how this would work out in the ORM:

$category = /* Category object */;
$posts = new ArrayCollection(/* list of posts */);

$criteria = Criteria::create()
    ->where(
        Criteria::expr()->contains('categories', $category)
    )
;

$matching = $posts->matching($criteria);

[estelsmith]

Here's an example of option 3 being implemented in ClosureExpressionVisitor::walkComparison() from my previous comment. It works rather well, but I don't know how it would affect ORM.

case Comparison::CONTAINS:
    return function ($object) use ($field, $value) {
        $fieldValue = static::getObjectFieldValue($object, $field);

        if ($fieldValue instanceof Collection) {
            return $fieldValue->contains($value);
        }

        return false !== strpos($fieldValue, $value);
    };
    break;

[marcospassos]

Any news on this?

[matiux]

Hi at all,
I have the problem:

        $criteria = Criteria::create()
            ->where(Criteria::expr()->eq('to.address', $this->owner->getEmail()))
            ->orderBy(['messageContent.date' => Criteria::DESC])
            ->setMaxResults(1);

        $messages = $this->messages->matching($criteria);

With this code I get Undefined property: MyDomain\Entity\Message::$from.address

Do you have any idea?

[trogwarz]

Any news here?

[garak]

Hi, I stumbled upon this PR after trying to use Criteria with a Value Object.
I'd like to see this PR merged, so I'm just bumping this up.

[El-Sam]

Hi, any updates on this issue?

[someniatko]

@Ocramius sorry for bumping, but few years of silence have already passed. I am also interested in this feature.

[SenseException]

I don't know if @mnapoli is still working on this PR or if someone like to take over. The branch needs a rebase before it can be worked on it again.

[mnapoli]

I am not working on this PR, it's been five years, it is obsolete now.

I am going to close it, if anyone reads this in the future you can pick this up, but I don't think it's worth notifying all the 13 people in this 5 years old thread, I encourage you to create a new PR and start a new discussion there.

[garak]

@mnapoli can you please clarify how this is "obsolete"?

[alcaeus]

Not speculating on what they meant, but it's a PR that's been sitting for 5 years. PRs aren't like good wine: they don't age well. If you're willing to work on this, take the work (you can do so by fetching the PR ref or the patch file), continue it locally (ideally preserving original commit authors) and get it to work before creating a new pull request.

To avoid further discussion on a stale issue, I'm closing this here. People are free to pick this up if they want to work on it, but I can understand if @mnapoli doesn't want to work on this anymore.