| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | database system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Berdir |
| Status: | needs work |
Issue Summary
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?
- Extend query:
db_select('users', 'u')->extend('Filter'); - Define a unique name:
$query->setFilterName('user_admin') - Define your filters in
hook_filter_elements_filtername() - 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)
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| filter.png | 28.64 KB | Ignored: Check issue status. | None | None |
| filter_extender.patch | 17.33 KB | Idle | Failed: Failed to apply patch. | View details | Re-test |
Comments
#1
Updated patch with some comments and simplified a few bits..
#2
Concept gets a major +1 Oooo from me. Need to review implementation soon.
#3
The last submitted patch failed testing.
#4
Re-roll because of the drupal_get_form() changes..
#5
The last submitted patch failed testing.
#6
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...
#7
The last submitted patch failed testing.
#8
Re-roll
#9
Re-roll, because of a conflict with the recent coding style fixes...
#10
The last submitted patch failed testing.
#11
Re-roll.
#12
The last submitted patch failed testing.
#13
Re-roll.
#14
subscribing
#15
I assume you crossposted.. ;)
#16
The last submitted patch failed testing.
#17
HEAD broke.
#19
The last submitted patch failed testing.
#20
Re-roll.
Still looking for reviews ;)
#21
(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).
#22
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, ...
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:
<?phparray($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.
Naming suggestions are always welcome, I'm quite bad at that stuff ;) I can't really understand what you mean, though ;)
Well, you have to look it up where you defined the query. You can't just know it.
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:
<?php$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:
<?php$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 already did that for hook_ranking in #394182: DBTNG search.module, so I think I can add that too.
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.
#23
The last submitted patch failed testing.
#24
I don' know why this should fail to install HEAD all of a suddon.. let's try again..
#25
The last submitted patch failed testing.
#26
We'll try again later.
#27
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.