EasyCorp / EasyAdminBundle

EasyAdmin is a fast, beautiful and modern admin generator for Symfony applications.
MIT License
3.99k stars 1.01k forks source link

Add a new "dql_filter" configuration option #986

Closed javiereguiluz closed 7 years ago

javiereguiluz commented 8 years ago

NOTE: this documentation explains a feature that doesn't exist yet. As usual, we first write the documentation, then ask the community and finally we implement the feature.

Thoughts? Thanks!

yceruto commented 8 years ago

...the only restriction is that you must use the entity name to refer to the current entity.

Why? We could not do it without this restriction? property = 'value'?

javiereguiluz commented 8 years ago

Why? We could not do it without this restriction?

Because we use a hardcoded entity. in all our current queries. For example: https://github.com/javiereguiluz/EasyAdminBundle/blob/master/Controller/AdminController.php#L494-L503 I don't see it like a problem because entity. name is easy to understand that it refers to the current entity. (SonataAdmin uses this other convention: $query->getRootAlias() which in my opinion is harder to understand)

yceruto commented 8 years ago

What do you think about adding some example by using container parameter? Something like this:

easy_admin:
    entities:
        VipCustomers:
            class: AppBundle\Entity\User
            list:
                dql_filter: 'entity.budget > %budget%'

This makes it possible to use different values depending on the environment or maybe dynamic values.

javiereguiluz commented 8 years ago

@yceruto thanks for the review. I've added an example with parameters.

javiereguiluz commented 8 years ago

I think I've found a "pain point" for this feature.

In this example:

easy_admin:
    entities:
        VipCustomers:
            class: AppBundle\Entity\User
            list:
                dql_filter: 'entity.budget > 100000'
        RegularCustomers:
            class: AppBundle\Entity\User
            list:
                dql_filter: 'entity.budget <= 100000'

The dql_filter config is very easy ... but you must duplicate all the customizations made for the User entity.

I see two possible solutions:

1) Introduce a extends config which makes an entity inherit all the config from another one:

easy_admin:
    entities:
        VipCustomers:
            class: AppBundle\Entity\User
            # customize a lot of things ...
            list:
                # customize everything about the 'list' option...
                dql_filter: 'entity.budget > 100000'
        RegularCustomers:
            extends: VipCustomers
            list:
                dql_filter: 'entity.budget <= 100000'

2) Copy the "Index Scopes" feature from Ruby's ActiveAdmin (see http://activeadmin.info/docs/3-index-pages.html)

The config would look like this:

easy_admin:
    entities:
        Customers:
            class: AppBundle\Entity\User
            # customize a lot of things ...
            list:
                # customize everything about the 'list' option...
                scopes:
                    - { label: 'VIP', dql_filter: 'entity.budget > 100000' }
                    - { label: 'Normal', dql_filter: 'entity.budget <= 100000' }
                    - { label: 'All Customers', default: true }

And the list page would show this as a filter:

scope

yceruto commented 8 years ago

I like the second option and it's really easy to understand. However, when we need a little more complexity e.g: use authenticated user or values coming from a service:

easy_admin:
    entities:
        Reports:
            class: AppBundle\Entity\Report
            list:
                scopes:
                    - { label: 'My Reports', dql_filter: 'entity.user = :user' }
                    - { label: 'All Reports', default: true }

then we must use one of these alternatives to complete the query ($qb->setParameters(...)):

You think you can add an advanced tutorial for this?

javiereguiluz commented 8 years ago

@yceruto very nice comment. I faced the problem about users as well. I need to think about this.


In addition, I think that we can implement both dql_filter and scopes. First option comes in very handy in lots of situations and the second one is useful while we don't have filters.

Pierstoval commented 8 years ago

Actually, when talking about dql_filter I couldn't agree because it would've created 2 backends for one single entity.

But then with the scopes notion from Ruby (Ruby is good, but painful :stuck_out_tongue: ) I find this very much acceptable !

Actually, implementing dql_filter only under scopes sounds like the best option to me.

For the :user and dynamic parameters, I don't see what use there can be: as long as we need a dynamic parameter, it comes to PHP, and then we need to go to the AdminController, and therefore we can add the filter manually in the createListQueryBuilder or createSearchQueryBuilder like this:

easy_admin:
    entities:
        Reports:
            class: AppBundle\Entity\Report
            list:
                scopes:
                    # No notion of "default". Here, the default is always the first.
                    # BUT there is a notion of "scope ID" that is important to refer to it anywhere.
                    all_reports: { label: 'All Reports' }
                    my_reports: { label: 'My Reports' }
public function createListQueryBuilder($entityClass, $searchQuery, array $searchableFields, $sortField = null, $sortDirection = null) {
    $qb = parent::createSearchQueryBuilder($entityClass, $searchQuery, $searchableFields, $sortField, $sortDirection);
    if ($this->scope === 'my_reports') {
        $qb->andWhere('entity.user = :user')->setParameter('user', $this->getUser());
    }
}

And voilà, easy dynamic query builder customization.

Obviously, the same can be done in search action (which I still want to be merged with list action because they're basically the same)

javiereguiluz commented 8 years ago

@Pierstoval I disagree with you about dql_filter. I think this small feature would provide lots of value to our users. Filtering the list of elements is a very common and basic need.

Pierstoval commented 8 years ago

Exactly, that's why the scopes option would be here for

tiraeth commented 8 years ago

Guys, if you could find time, please take a look at my fork — commits e19551a (scopes functionality) and af4a21b (tests). I took a shot at the scopes functionality. If this is the approach you wish EasyAdminBundle should go with, I can work on documentation updates (and include missing translation keys for list.scopes.all default label), rebase everything to a single commit, and prepare a PR for easy merge.

I tried to keep things fully backward compatible, so existing extensions around QueryBuilder do not break.

:smile:

Pierstoval commented 8 years ago

@tiraeth Sounds good to me, could not review "deeply", but I think you can prepare a PR. If needed, @javiereguiluz will rebase it manually to add modifications :)

javiereguiluz commented 8 years ago

@Pierstoval actually ... I'm not ready yet to merge the "scopes" feature. I'd like to do that in the future as part of the filter feature. What I'm willing to add right away is the dql_filter feature.

tiraeth commented 8 years ago

@javiereguiluz any reason to add functionality that will be removed in future (replaced by scopes) :-) introducing BCB, need for depreciations etc.

javiereguiluz commented 8 years ago

@tiraeth sorry I didn't explain myself better: I'd like to have both features. The dql_filter is a quick way to do things that you commonly need. The scopes (and filters) feature takes more time to configure but they are more powerful and useful for complex backends. I think both are needed (and therefore, your work will be needed when implementing the "scopes"). Thanks!

Pierstoval commented 8 years ago

@javiereguiluz I don't understand why you want to make things "slower" for this two features. If @tiraeth makes a PR and if it's compliant with EasyAdmin needs and practices, I don't see why you need time to integrate the feature "later" while a fully-developed & tested PR is available for both features.

tiraeth commented 8 years ago

I can add global filter under list configuration node too that will be used with query builder if not false/null :-) also I would keep query prefix instead of dql or none at all. If easyadminbundle is going to be used with other orms/odms in future :-)

Pierstoval commented 8 years ago

If easyadminbundle is going to be used with other orms/odms in future :-)

I bet it will not, but at least, consistency with query and dql is important, I also agree with the idea because actually we're writing a "piece of query" , not a "dql query" even if it "looks" like dql :)

javiereguiluz commented 8 years ago

I prefer to keep the name dql_filter because it's self-explanatory: it only accepts Doctrine DQL code. In the future, if we support ODM, etc. (very unlikely), we can rename it to query_filter. By the way, I plan to introduce for the search view another option called dql_query (so you can define the entire DQL query to look for in joined entities, etc.)

tiraeth commented 8 years ago

@javiereguiluz @Pierstoval please check my new commits a4635c5 and 1443572 — renamed filter to dql_filter, added global dql_filter, modified tests and fixed a small bug in paginator.html.twig HTML structure.

javiereguiluz commented 8 years ago

@tiraeth I'm sorry I didn't reply to you. The reason is that I need to find enough time to review your work. Thanks for your patience.

anthony-launay commented 8 years ago

+1

kesar commented 7 years ago

fix conflicts and 👍

javiereguiluz commented 7 years ago

This feature is now completed and ready to be reviewed. Thanks!