admin/user filter

Coding style looks good, although I prefer to use long-array format (one element per line) for hook_menu() the way you do for some other entries. It makes it MUCH more readable, especially if you also vertically align the =>'s.

I'm curious, though. What would you think of instead of a separate tab, having the roles be local tasks underneath the normal list tab? The first item (weight -10 or something) would be "all", followed by the rest of them as now.

Alternatively, since some sites (including one I'm planning) can end up with a large number of roles, have a select box instead of tabs, a la the log page. First/default item is "All Roles", with each role a select option below it. That would scale better to sites with more than 4-5 roles.

So +1 on functionality and code style, but UI could be a bit cleaner. :-) I think the select box option would be more consistent with other similar pages (logs).

Yep, we need a select box rather than a tab/subtabs.

Okay, I've completely rewritten this so that things work like node/admin with filters for role, permission, and status (active or banned). Table is still sortable.

Patch affects user.module and drupal.css (just a little). It also provides a function called drupal_array_flatten() which takes an array of arrays and makes it a single level array with keys and values in tact.

Code needs testing and review.

-Jeff

I wanted to test this patch on my laptop (1.6GHz) and the admin/user page never got displayed due to mysql overload. The same machine can handle drupal.org's database fine under other circumstances. I suggest improving the query and testing it with several users. Maybe approx. 30000. :p

Actually, this was just a small problem with a missing ON condition. Ive changed the patch to accomodat this.
One problem with the patch is that you cannot search for users that have roles A and B or perms foo and bar.

One problem with the patch is that you cannot search for users that have roles A and B or perms foo and bar.

Yes. There is the same problem with node.module.

The main issue is: what would the interface look like? And then there are the logic problems. For example how would Drupal display:

permission is edit own blog
or where permission is create pages
and where status is blocked

Because from a logical reading standpoint, the OR operator would take precedence, however in SQL, the AND operator takes precedence. So this statement is actually more like:

permission is edit own blog or
(where permission is create pages and where status is blocked)

And for that matter, there should probably be a selector for "is not", so that one could select users where:

role is authenticated user
and role is not administrator

But now we're building an entire (light duty) SQL query engine, which in itself could really be a handy tool for Drupal (both node.module and user.module could use the same engine - also handy for ecommerce and lots of other db searching), but is quite a lot of feature bloat considering that this patch started as a simple list of users by role.

I'd be willing to take a shot at this, but
a) Do we have enough functionality now for a preliminary commit (assuming code approval)?
b) I'd love to hear some other opinions about this new direction.
c) What are some other uses for this type of filter? What is the best way to implement?

Let's hope that this function doesn't go careening into obscurity due to loss of focus and direction.

-Jeff

In the interest of speed and get-this-in-before-the-freeze, I'd say go simple. Just allow three options for now: filter by a single permission, single role, and single status. Don't bother saving anything or doing complex ANDs and ORs. That already gives a great deal more power than we have now, and should be simple enough to get solid and into core for 4.7.

Once that's in, we can, later, come back and look at doing fancier searches. Even with just that one filter it's a huge step forward from what we have now.

Personally I think the buttons should go below the select boxes, not next to them, but that's me. :-)

I got the following errors:

user error: Unknown table 'ur' in on clause
query: SELECT COUNT(*) FROM users u  INNER JOIN permission p ON ur.rid = p.rid WHERE u.uid != 0 AND  (p.perm LIKE '%administer url aliases%' OR u.uid = 1)   in includes/database.mysql.inc on line 108.

user error: Unknown table 'ur' in on clause
query: SELECT DISTINCT(u.uid), u.name, u.status, u.created, u.access FROM users u  INNER JOIN permission p ON ur.rid = p.rid WHERE u.uid != 0 AND  (p.perm LIKE '%administer url aliases%' OR u.uid = 1)  ORDER BY  u.created DESC LIMIT 0, 50 in includes/database.mysql.inc on line 108.

Can any of this code be simplified? Is there duplication between the node.module and the user.module? It's a fairly large patch.

Please keep this patch simple. No crazy stuff, please. As Crell pointed out, this patch already gives a great deal more power than we have now.

Killes,

You changed this line:

$sql = 'SELECT DISTINCT u.uid, u.name, u.status, u.created, u.access FROM {users} u INNER JOIN {users_roles} ur '. $filter['join'].' WHERE u.uid != 0 '.$filter['where'];

to:

$sql = 'SELECT DISTINCT(u.uid), u.name, u.status, u.created, u.access FROM {users} u '. $filter['join'] .' WHERE u.uid != 0 '. $filter['where'];

and this is where Dries' error is coming from.

Is this where your SQL overflow was coming from?

The problem is that permissions are linked to roles which in turn are linked to users. So there need to be 3 tables joined in order to list users by permission. Thus that more likeliness of a SQL overflow - not to mention the memory problems with having to use "LIKE (%%%s%%)" to find permissions.

Is there duplication between the node.module and the user.module? It's a fairly large patch.

Yes, while most is not direct duplication of node.module functions, there are definitely functions that are performing the same or similar functions. This is where I was saying we may be headed in the direction of creating a central form-to-SQL-query system.

Can any of this code be simplified?

I'm all for that. If we are agreed that the SQL queries get too complicated too fast, I will remove the compound options and $_SESSION stuff and allow for only one filter at a time.

-Jeff

i hope this patch comes back to life. would be useful.

This patch works fine for me on a site with over 250k users, and is direly needed. I am not sure if it could be made simpler, but at first glance I did not notice anything glaring.

+1 from me, I would love to see this go in, as otherwise I'll have to keep a patched user.module until something like this does. Literally every client I have asks for this...

This patch still applies to head.

Here's a version of the patch that only applies one filter at a time. It should protect against the SQL memory overflows that Killes was experiencing.

On the other hand, I haven't experienced these memory overflows and if it's working with Aaron's 250,000 users, maybe we should stick with the "full blown" version.

I've also moved the drupal_flatten_array() function into common.inc as that seems like a more appropriate place for it.

-Jeff

In no special order:

- I really really like this functionality. :-)

- I found one bug. By default, none of the radio options is selected. If you submit the form without selecting one, then it craps out with a "not using an array in a foreach()" error in the drupal_array_flatten() function. I'd suggest just defaulting the first option to be on, for consistency with the content page filter.

- Speaking of the content page, I know I said before that I'd be fine with just the single filterable option, and I still am because given the values we're searching on here there's not a great deal of overlap. However, the content page appears to have a functioning saveable filter. How difficult would it be to steal that code, for UI consistency?

In the long run, I think the best solution is to develop a filter add-on to the table pager system so that it's easy to add a filter dialog like this to any pager table. Then the user page and content page could use it, as well as the log page, and various other pages. That would be sweet. :-) I don't see it happening in the 4.7 timeframe, though. In the near term, fixing the bug above should make this patch commit-ready. The coding style looks good overall, although I'm no expert on the new forms API. Line 2040 in user.module is commented out functionality that you'll probably want to trim, same on 2017.

So:

- 60 second bug fix and line deletion should make this commit-ready.
- MAYBE look into into stealing code from admin/content if you've time. If not, that's OK.
- Long term, this should be generalized into a general query-table system. That's for 4.8, though, not 4.7.

Hm. I was going to just fix this up myself, but while this patch still applies, it now appears non-functional. Something else must have broken it in the past two weeks. I've no idea what, though, and I'm afraid I don't have the time to track down what.

Does anyone else have the time to spare figuring out what went wrong here? This is a very needed feature for large sites.

nice demo :).

- seems like we can't stack multiple filters like admin/node?
- also, if the UI isn't hideous I would switch Status to a checkbox.
- if you didn't already do so, i might also hyperlink the permission names to this page with the permission filtered. your call.

looks nice,

is it possible to add some js-foo to select the right radio item of the select-box I'm changing currently?
would be a nice simplificaton because it would save us one click...

or to remove the radio buttons completely...

@hunmonk - sounds good. i was referring to the permission names on the admin/access page. sorry.

If we can't stack filters like the content module can, we should remove the status messages (eg. "role is supermodel") and we should make sure that the selected options 'stick'. Right now, it looks like the form is reset. Did that explanation make sense?

I like this.

Forgive me if this is off-topic, but I often receive email from users and need to look up their account, knowing only their email address. Any chance we could add a sortable email column to the user table? Filtering by email (i.e. *@yahoo.com) would be even better.

As I noted back up in comment #15, this and admin/node have essentially the same needs, filter-wise. The best solution, IMHO, is to generalize the query-building control and then use it on both pages. Less code, more flexibility, get the saving/stacking in both places, and the ability to use it on other pages at no extra charge.

Also as I noted back up in comment #15, I'd be happy to just get it in for now and revise later. :-)

We should try to make it the same and generalize where/if it makes sense. (It's obvious that the comment.module might be next up so generalization might be helpful -- if it makes sense.)

I'm not sure that is a good idea. It looks like it might take a while before we're able to flesh out the permission patch. It's still in the early design phase. Is the lack of that patch a showstopper?

It would be great if StevenW could carry this patch home. He wrote the node filter patch so he knows the ins and outs of this problem, and it is the right person to compare both implementations.

the criteria builder is not showing in the demo .... the destination param on the admin link is empty. would be good to fix this - is not working in HEAD either.

i added permission criteria as second restrictor as i got an odd duplicate that reads:

permission is administer blocks
and where status is blocked
and where permission is administer blocks

looking close ...

@hunmonk - that sounds perfect. lets get this one in.

Looks good to me. I'll let Steven commit this. It seems that you've been working with him on this in private and it's not clear whether Steven has showstopping concerns. Make sure to update CHANGELOG.txt when this gets committed.

• If no roles other than the default are present, the role dropdown is empty. It should disappear instead.
• The following bit seems incorrect... "anonymous user" permissions do not apply to "authenticated users" and should not be taken into account (and those abbreviations are a bit icky).
  

      // This checks to see if this permission filter is an enabled permission for either the anon or auth roles.
      // If so, then all users would be listed, and we can skip adding it to the filter query.
  

• User 1 never gets the anonymous or authenticated role, which means the INNER JOIN for the permissions check fails if he has no other roles, so no rows are returned and the "OR u.uid = 1" clause is never reached. It should probably be a LEFT JOIN with (p.perm IS NOT NULL AND p.perm LIKE '%%%s%%') OR u.uid = 1.
• The code should use the defined constants for the default roles rather than ints.

Seems to work fine. Committed to HEAD.