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

Allow to set an optional filter #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow to set an optional filter #7

wants to merge 1 commit into from

Conversation

bakura10
Copy link
Member

Addresses #6

I think this logic could actually be added to the AbstactHydrator, so that any built-in hydrator has the chance to specify an optional filter. As this is done in the constructor, it still makes the filter chain immutable, so we can keep any aggressive optimization.

@Ocramius
Copy link
Member

I don't really understand #6 - what is the possible test case for this?

@bakura10
Copy link
Member Author

Well, this is simple.

With the new architecture, all built-in hydrators are final, so we cannot modify their filters. Instead, our hydrators just implement HydratorInterface and composes built-in hydrators (most of the time).

So imagine an entity like this:

class User
    {
        protected $projects;

        public function getActiveProjects()
        {
            return $projects->matching(Criteria::expr(Criteria::eq('active', true)));
        }
    }

And your hydrator that composes class methods:

class UserHydrator implements HydratorInterface
    {
        protected $hydrator;

        public function __construct()
        {
            $this->hydrator = new ClassMethodsHydrator();
        }

        public function extract($object)
        {
            $data = $this->hydrator->extract($object);
            unset($data['active_projects']);

            return $data;
        }
    }

This is nice, we use unset to remove any unwanted value. We could even have more complex logic (based on roles for instance). The issue here is that the getActiveProjects is always called by the hydrator. But in this very specific case, the getActiveProjects generate a SQL query. In some of my models, I take advantage of the Criteria API to have 2 or 3 utility methods like that. This means that it may generate 2 or 3 SQL queries EACH TIME I extract, for just removing the data all the time.

This is a lot of overhead.

So my idea is to allow each hydrator to accept an optional filter (this can be a composite filter if you need multiple filters) that gets ANDed to the other filters. This can only be set in the constructor, so it is still guaranteed that filters won't change during the life of the hydrator, hence we are still able to do aggressive caching:

```php
class UserHydrator implements HydratorInterface
    {
        protected $hydrator;

        public function __construct()
        {
                        // Add a filter
                        $filter = new ExcludeMethodsFilter(['getActiveMethods']);
            $this->hydrator = new ClassMethodsHydrator($filter);
        }
    }

Internally, the optional filter just is added to the filter chain using an AND condition. The getActiveMethods won't be called anymore, we still are able to use the ClassMethods without rewriting all its internal just for this...

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

Successfully merging this pull request may close these issues.

2 participants