Note: This needs better documentation, tests, ... It is just a first "working" patch, because I try to be a good contributor and I'd like to have some feedback about the idea/workflow ;) (http://webchick.net/embrace-the-chaos)

The idea:
Provide a flexible, generic, Filter DB Extender which allows to filter listings, for example admin/user/user and admin/content/node.

Why?
Core currently has several of these slightly different filter forms, each with its own form definition, css, ugly query builder functions and so on. They are pretty hardcoded, other modules can't extend them. (for example, node currently needs to define taxonomy stuff: http://api.drupal.org/api/function/node_build_filter_query/7).

What are the advantages?
- Extendable: Other modules can provide additional filter, which can be as complex as they want it to be
- Shared code, same UI/Look & Feel
- Easy to implement for additional listings (I hope.. )
- Actually working multi-value support (for example, admin/user/user seems to be pretty broken). Filters can either only allow a single value (a new filter would override the existing) or multiple values (a new filter extends the existing), just by defining multiple => TRUE.

How does it work?

  1. Extend query: db_select('users', 'u')->extend('Filter');
  2. Define a unique name: $query->setFilterName('user_admin')
  3. Define your filters in hook_filter_elements_filtername()
  4. Either extend an existing form with $form['filter'] = $query->getForm() or use the helper function drupal_get_form('filter_extender_form', $query)

How does the filtering work?
Simple conditions: just define a key 'field' in your hook and Filter will add conditions for it, like $query->condition($field, $value).
Advanced filter: Filter automatically adds a tag "filter_filtername" and adds the currently active filters and filter elements as metadata, so a module can implement hook_query_filtername_alter() and do the advanced stuff there.

What this patch currenty does:
- Implement a Filter Db Extender class
- Replace admin/user/user with this class
- Uses the user_filters theme as base and changes it a bit (Note: This really needs UX/Accessibility help and review)

Files: 
CommentFileSizeAuthor
#20 filter_extender8.patch21.36 KBBerdir
Failed: Failed to apply patch. View
#13 filter_extender7.patch21.37 KBBerdir
Failed: Failed to apply patch. View
#11 filter_extender6.patch21.29 KBBerdir
Failed: Failed to apply patch. View
#9 filter_extender5.patch21.42 KBBerdir
Failed: Failed to apply patch. View
#8 filter_extender4.patch21.42 KBBerdir
Failed: Failed to apply patch. View
#6 filter_extender4.patch21.35 KBBerdir
Failed: Failed to apply patch. View
#4 filter_extender3.patch20.1 KBBerdir
Failed: Failed to apply patch. View
#1 filter_extender2.patch20.04 KBBerdir
Failed: Failed to apply patch. View
filter_extender.patch17.33 KBBerdir
Failed: Failed to apply patch. View
filter.png28.64 KBBerdir

Comments

Berdir’s picture

FileSize
20.04 KB
Failed: Failed to apply patch. View

Updated patch with some comments and simplified a few bits..

Crell’s picture

Concept gets a major +1 Oooo from me. Need to review implementation soon.

Status: Needs review » Needs work

The last submitted patch failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
20.1 KB
Failed: Failed to apply patch. View

Re-roll because of the drupal_get_form() changes..

Status: Needs review » Needs work

The last submitted patch failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
21.35 KB
Failed: Failed to apply patch. View

A few updates:

- Refactored and simplified the code a bit..
- Added support for textfields, optionally with autocomplete
- Added a autocomplete username filter to admin/user/user, basically to test if the above feature is working... it's not using LOWER() currently.

----
TODO:

"- Uses the user_filters theme as base and changes it a bit (Note: This really needs UX/Accessibility help and review)"
This is still true, I'd love to get some ideas/feedback from Usability people about the filter form.. imho, it's ugly but I'm not sure how it could be improved (semantically and visually):

- I'm thinking if and how more complex conditions could be supported by default, without the need of using hook_query_alter(). For example conditions on joined tables...

Status: Needs review » Needs work

The last submitted patch failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
21.42 KB
Failed: Failed to apply patch. View

Re-roll

Berdir’s picture

FileSize
21.42 KB
Failed: Failed to apply patch. View

Re-roll, because of a conflict with the recent coding style fixes...

Status: Needs review » Needs work

The last submitted patch failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
21.29 KB
Failed: Failed to apply patch. View

Re-roll.

Status: Needs review » Needs work

The last submitted patch failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
21.37 KB
Failed: Failed to apply patch. View

Re-roll.

mikey_p’s picture

Status: Needs review » Needs work

subscribing

Berdir’s picture

Status: Needs work » Needs review

I assume you crossposted.. ;)

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

Status: Needs work » Needs review

HEAD broke.

Status: Needs review » Needs work

The last submitted patch failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
21.36 KB
Failed: Failed to apply patch. View

Re-roll.

Still looking for reviews ;)

mikey_p’s picture

(disclaimer: I just spent 20 minutes reading the patch, and didn't test it out)

A couple of things:

 includes/filter.inc         |  310 ++++++++++++++++++++++++++++++++++++++++++++
 modules/user/user.admin.inc |  164 ++---------------------
 modules/user/user.css       |    6 
 modules/user/user.module    |   58 ++++----
 4 files changed, 360 insertions(+), 178 deletions(-)

1. A big +1 for making this simpler inside of use.admin.inc

2. In Filter::getForm, the '#query' => $this just feels awkward. I'm not real sure what to do with that. Pager and TableSort extenders don't attempt anything like this (they don't need to), but what about the possibility of moving the form and form submission stuff out of Filter and having it modify the URL (add $_GET params) like the pager or table sort stuff?

3. The naming of the elements hook seems somewhat confusing. Currently it's hook_filter_elements_FILTERNAME. At least in the case of this patch, should we then define hook_filter_elements_user_admin as it's own hook and make it clear that user_filter_elements_user_admin is an implementation of that hook?

4. How is one to know what tables (and aliases) and fields are available in hook_filter_elements_filtername?

5. I don't like how processing and applying the filters to the query is split up between Filter::execute() and hook_query_QUERY_TAG_alter. I can see why it is simpler for now, but that kind of defeats the overall purpose of simplifying the amount of code to implement each time filter functionality is needed. Moving more of this into Filter::execute() would be nice, perhaps taking a look at how views does things would be good as this seems pretty similar to some of the things it does.

I.e Could this allow a type of filter to be described in hook_filter_elements_filtername, such as 'condition', or 'join' and then an additional array below that to describe all the necessary fields to join against?

Also, is there a way that filters could be aware of finding the correct aliases for the tables, and not requiring the aliases to be used in hook_filter_elements_filtername? It would be nice if there was also a way to add conditions against field not in the original query, although I suspect that would not be too hard (it might even work now, I didn't try it).

Berdir’s picture

1. A big +1 for making this simpler inside of use.admin.inc

Well yes, that's the idea of the patch :). And once it is in, we can do the same for node.admin.inc, dblog.admin.inc, ...

2. In Filter::getForm, the '#query' => $this just feels awkward. I'm not real sure what to do with that. Pager and TableSort extenders don't attempt anything like this (they don't need to), but what about the possibility of moving the form and form submission stuff out of Filter and having it modify the URL (add $_GET params) like the pager or table sort stuff?

It is awkward ;)
The proper way would imho be to add support for php callbacks to drupal_function_exists, so that we can directly call the submit function with something like:
array($this, 'submitForm')
Even if we move the form stuff outside of the class into functions, we still need to access it to load/save the filters and so on. There is imho no way around that, because we need to know the filter name and so on. This is also not a problem of session vs GET, because the filter class should abstract that, the user doesn't need to know where it is saved.

3. The naming of the elements hook seems somewhat confusing. Currently it's hook_filter_elements_FILTERNAME. At least in the case of this patch, should we then define hook_filter_elements_user_admin as it's own hook and make it clear that user_filter_elements_user_admin is an implementation of that hook?

Naming suggestions are always welcome, I'm quite bad at that stuff ;) I can't really understand what you mean, though ;)

4. How is one to know what tables (and aliases) and fields are available in hook_filter_elements_filtername?

Well, you have to look it up where you defined the query. You can't just know it.

5. I don't like how processing and applying the filters to the query is split up between Filter::execute() and hook_query_QUERY_TAG_alter. I can see why it is simpler for now, but that kind of defeats the overall purpose of simplifying the amount of code to implement each time filter functionality is needed. Moving more of this into Filter::execute() would be nice, perhaps taking a look at how views does things would be good as this seems pretty similar to some of the things it does.

The thing is, we can handle *more* things by default, like simple joins. But we can never handle everything (someone might need a join over two tables, or with more join conditions, ...) and that's the reason hook_query_QUERY_TAG_alter exists. Example:

       $account = new stdClass();
       $account->uid = 'user_filter';
       $account->roles = array(DRUPAL_AUTHENTICATED_RID => 1);
       if (user_access($value, $account)) {
         continue;
       }
      $query->leftjoin('role_permission', "p$i", "ur.rid = p$i.rid");
      $query->condition(db_or()
        ->condition("u.uid", 1)
        ->condition("p$i.permission", $value)
      );
      $i++;

The condition should a) not be applied if all users have the permission and b) uid 1 is a special case and does have all permissions implicitly. There is simply no way to "define" this in hook_filter_elements(). However, we should be able to handle the following:

    $i = 0;
    foreach ($filter['role'] as $value) {
      $table = $query->leftJoin('users_roles', "ur$i", "ur$i.uid = u.uid");
      $query->condition("ur$i.rid", $value);
      $i++;
     }
I.e Could this allow a type of filter to be described in hook_filter_elements_filtername, such as 'condition', or 'join' and then an additional array below that to describe all the necessary fields to join against?

I already did that for hook_ranking in #394182: DBTNG search.module, so I think I can add that too.

Also, is there a way that filters could be aware of finding the correct aliases for the tables, and not requiring the aliases to be used in hook_filter_elements_filtername? It would be nice if there was also a way to add conditions against field not in the original query, although I suspect that would not be too hard (it might even work now, I didn't try it).

The whole alias stuff in the hook is so complex because the dynamic alias handling of JOINS is currently broken by design, as you need the alias for the join condition. Adding conditions to other fields should just work, if I understand you correctly.

Status: Needs review » Needs work

The last submitted patch failed testing.

Berdir’s picture

Status: Needs work » Needs review

I don' know why this should fail to install HEAD all of a suddon.. let's try again..

Status: Needs review » Needs work

The last submitted patch failed testing.

Crell’s picture

Version: 7.x-dev » 8.x-dev

We'll try again later.

kika’s picture

FYI, I posted #1475204: [META] Provide a generic search/filter UI interface pattern for listing pages what deals with UI side of things for a generic filter interface.

ivanjaros’s picture

Has anybody been working on this yet?

mikey_p’s picture

Most of the places in D8 where this would be used have been replaced with views, so I'm not sure what is appropriate here.

ivanjaros’s picture

I had a custom solution in D7 and I will need this in D8 so I was just asking if I should use my solution again or wait for this to get into core.
And yes, there sure is/will be need for this in contrib.

Crell’s picture

Status: Needs work » Fixed

With Views in core now I don't believe this is necessary. Marking as fixed since, technically, the use case is now covered.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.