Skip to content
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

Add extension point to match conditions using closure #96

Closed
wants to merge 1 commit into from

Conversation

yvoyer
Copy link

@yvoyer yvoyer commented Nov 20, 2016

This PR will give the opportunity to provide a custom entry point in the matching of objects without relying on the inner get* is* format supported in the ClosureExpressionVisitor::getObjectFieldValue().

Clients will now be able to have their own mechanism (whatever the name format, with or without arguments).

class Person
{
    public function matchAddress(Address $address) {
         return $this->streetNumber == $address->streetNumber();
    }
}

$collection = new ArrayCollection(
    array(
        new Person($streetNumber = 1),
        new Person($streetNumber = 45),
        new Person($streetNumber = 78),
    )
);
$santaAddress = Address::fromString('1 main street, North Pole H0H 0H0');

$criteria = Criteria::create(
    andWhere(
        Criteria::expr()->matchingClosure(
            function (Person $context) use ($santaAddress){
                return $context->matchAddress($santaAddress);
            }
        )
    );

$collection->match($criteria); // returns the persons with addresses that match the street number

This PR could potentially help close #95, #68, #62, #50, #45, #28, #27 (if we do not plan on adding a fix), since the reporters could define a callback for their use case.

I am not sure what it would mean for PersistentCollection in the ORM package, but at least it do not affects the pattern used to access the data for checking the value.

@yvoyer yvoyer changed the title Add extension point to match conditiions using closure Add extension point to match conditions using closure Nov 20, 2016
@sovaalexandr
Copy link

Just noticed: doctrine/collections minimal required PHP version now is 5.5 so tests should pass after rebase to current master.

* Add comparison type implementations
* Centralize the comparison at the right place
* enable a new extension place to provide custom operators.
* Add MatchClosure Expression
@mikeSimonson
Copy link
Contributor

The main reason, I have not merged this PR yet is because it would maybe break in subtle way the lazy loading of the ORM. There is not way to translate a closure into a sql filter.

I am trying to know if it's really blocking or not @Ocramius ?

@Ocramius
Copy link
Member

@mikeSimonson it is blocking - we need to (sadly) merge visitor changes in sync with the repos. That sucks, but it's a design issue we didn't think of initially.

@yvoyer
Copy link
Author

yvoyer commented Dec 12, 2016

@mikeSimonson @Ocramius anything I can do to help merge this PR?

Would you prefer if I made another PR just for the extraction in classes, without the MatchClosure?

@stof
Copy link
Member

stof commented Dec 12, 2016

this is simply impossible to implement for the ORM and ODM, so it does not fit the Doctrine project IMO

@yvoyer
Copy link
Author

yvoyer commented Dec 12, 2016

Since this is a library that user can use as a stand alone, at least we should give them the opportunity to hook in the expression matching without having the switch case. Extracting in classes would help to do that. If a user wishes to define a criteria using a closure and do not have to bother with SQL filter, why do we forbid it?

I use the collections in some projects without the ORM and ODM, and sometimes I encounter the restriction caused by name standards and object equality that would be solved by a custom expression.

If we don't want to introduce the ClosureMatcher in the core, I have no problem with it, but at least we should open a door for the user to use its custom expression.

@stof
Copy link
Member

stof commented Dec 12, 2016

@yvoyer the criteria system is not meant to be extensible currently. As the system is meant to provide a way to apply criteria on persistent Doctrine collection without fully initializing them, making it extensible would imply making these extensions working for database evaluations too, which is a hard problem.
The goal of doctrine/collections is not to provide a full-feature in memory collection, but to meet the needs of the Doctrine project.

so for me, even getFilterCallback receives a -1.

@Ocramius
Copy link
Member

Closing as per @stof's review here. Meanwhile, please note that ORM 3.x will have a completely property lazy-loading mechanism that will indeed support private properties too.

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

Successfully merging this pull request may close these issues.

5 participants