Viewing the user listing page as a non-admin user results in the PHP error message: "Warning: Illegal offset type in isset or empty in views_query_views_alter()"

Will attach a patch for this issue shortly.

CommentFileSizeAuthor
#5 administerusersbyrole-views-error-2853289-5.patch3.98 KBshawn_smiley
#2 administerusersbyrole-views-error-2853289-1.patch4.04 KBshawn_smiley

Comments

shawn_smiley created an issue. See original summary.

shawn_smiley’s picture

shawn_smiley commented
Status: Active » Needs review
StatusFileSize
new administerusersbyrole-views-error-2853289-1.patch4.04 KB

The attached patch updates administerusersbyrole_query_administerusersbyrole_edit_access_alter() to resolve the PHP Illegal Offset Warning.

adamps’s picture

adamps commented
Status: Needs review » Needs work

Many thanks for the bug report and patch. I think you understand SQL better than me and can help us get better code, so I appreciate that.

There seem to be a lot of changes, and I think not all of them just to fix the warning - please can you help me understand the changes you are making?

1) New function administerusersbyrole_get_role_exclusions looks like a performance fix. I am nervous of making these without clear evidence about what is the bottleneck. It's easy to make the code more complex yet not any faster. I believe you that this gets called multiple times, but how do you know that this is significant overall?

2) Addition of if (!empty($exclude)) seems to fix #2849563: Errors on user overview page when user can edit all roles so thank you.

3) As per the other issue, let's handle @var markers separately.

4) Beyond that you are changing stuff and I don't understand why. The code is the same in D7 and seems fine. The code in D8 seems to work apart from the PHP warning.

5) If we are changing D8 do we also need to change D7? On the whole I'd prefer to keep them roughly equal where possible to simplify ongoing maintenance.

Please excuse me for being cautious, but I think that's the role of a maintainer. I'm not against innovation and progress, but in balance I need to validate the patches and look after overall stability of the module. On the whole, I think I would be more comfortable with a much simpler patch for what on the face of it is a simple problem - but I'm prepared to listen to counter-arguments.

shawn_smiley’s picture

shawn_smiley commented

1) The function administerusersbyrole_get_role_exclusions() is a tiny performance optimization for the query. It just wraps the following code from administerusersbyrole_query_administerusersbyrole_edit_access_alter in a function with static caching since there is no need to repeatedly build the exclusion list within the same HTTP request. In my debugging, I found that this code would be executed 2-5 times per request. Though I did not profile the code to determine the overall performance impact.

    $user = \Drupal::currentUser();
     $roles = user_roles(TRUE);
    $exclude = array();
     foreach ($roles as $rid => $role) {
      if (!$user->hasPermission(_administerusersbyrole_build_perm_string($rid, 'edit'))) {
      $permission = _administerusersbyrole_build_perm_string($rid, 'edit');
      if (!$user->hasPermission($permission)) {
         $exclude[$rid] = $rid;
       }
     }

2) I love it when a fix for one thing fixes other problems at the same time. :-)

3) I'll re-roll the patch to remove the @var comments.

4 & 5) The underlying problem in the code that was causing the error message was this line:

$urAlias = $query->leftjoin('user__roles', 'ur', 'ur.entity_id=users_field_data.uid AND ur.roles_target_id IN (:exclude[])', array(':exclude[]' => $exclude));

In Drupal 8, the query processor wasn't correctly converting the $exclude variable into the query syntax.

The new version basically does the same thing and should work with both Drupal 7 and Drupal 8.

shawn_smiley’s picture

shawn_smiley commented
StatusFileSize
new administerusersbyrole-views-error-2853289-5.patch3.98 KB

Updated patch based on the latest dev commits with the @var comments removed.

shawn_smiley’s picture

shawn_smiley commented
Status: Needs work » Needs review
adamps’s picture

adamps commented

Thanks Shawn for the updated patch. For efficiency's sake, you don't need to send me any more updated patches. I'm happy for us to finish the discussion, then I'll adapt your patch if needed based on our conclusions.

I think we are making progress on two fronts:

  1. I think I have found the underlying problem of a bug in Drupal core #2744069: views_query_views_alter() does not handle IN queries. I have just applied the patch in that issue and the warnings have gone away. The issue is currently set as "need tests".
  2. Shawn has figured out the alternative query - thank you for working this option out. It certainly looks like it does the same. I am a bit concerned how it would compare for efficiency as it seems that the subquery has to collect all users that are not accessible, possibly a substantial list. In comparison the original anti-join approach was working with a list of roles, typically much shorter. Any thoughts on the perf aspect?

At the moment I feel inclined to postpone this issue for a few months. We can add the core patch to our list (making 3 patches needed). With modern tools like composer, auto-apply-patching is pretty easy. We can revisit as the number of installed sites using D8 grows, and in response to the level of grumbling about patching. Hopefully by then at least one of the 3 core bugs has been fixed.

However feel free to start grumbling now if this is a major pain:-)

For the record, even if I change the query, I don't propose to back-apply to D7.

Finally, the patch still has a lot of other changes beyond the principle bug fix. Let's cover these briefly:

  • The tentative optimisation with new procedure. At a guess it's a small percentage change on a low-traffic page, so without clear evidence it's needed, sorry I'm going to veto that. It's increases code complexity and makes it more difficult for me to maintain and merge back and forth with D7.
  • Some comment improvements which we should take, whichever version of the query we use. Not important to back-merge to D7.
  • Of course I should take the fix for the other issue that you accidentally did for free.
  • Additions of query->distinct(TRUE); which haven't yet been discussed. Shawn perhaps you can clarify the reason? This page admittedly for D7 states that "DISTINCT can introduce a performance penalty" - what would you say about that? Would you suggest that we take these even if we stick with the original query? What about merge to D7?
shawn_smiley’s picture

shawn_smiley commented

Thanks for the feedback and prompt responses.

I agree that the potential performance improvement by the static caching is probably very miniscule, especially in comparison to DB query time. So I don't mind that code being moved back into the alter function without the caching.

As for query performance, I did a really quick unscientific test of both query approaches. I used Devel Generate to create roughly 1000 user accounts on a D8 site with various roles and about 25% of the users with multiple rows. Then executed the generated queries directly against the MySQL DB using SQLPro. I stopped and restarted MySQL between each query run to ensure I was working with a cold DB cache.

When the total number of users was less than 50, the original query was slightly faster at about 1.4ms vs. 2ms for my updated version. At 1000 users, the original query is slightly slower at 7.2ms vs. 6.8ms for my updated version.

Neither of these approaches is probably good for sites with 100,000+ users though.

adamps’s picture

adamps commented

Thanks, that's all very clear.

I think that covers everything except you didn't mention "query distinct". After a few searches I get the impression that adding "distinct" may slow the query down. Is there a particular need for it?

shawn_smiley’s picture

shawn_smiley commented

I agree that distinct is less than ideal and on sites with a small number of users it probably adds more overhead than it is worth. I would love to come up with a way to structure this query such that it doesn't use distinct, IS NULL, or NOT IN. All of those are horrible for performance as they require the database to dump all of the data to a temporary table and do a row by row comparison to generate the results.

Unfortunately I haven't been able to come up with anything that doesn't result in duplicate user accounts being listed when users have multiple roles. :-(

From the unscientific spot checks I did this morning, it does appear that doing the left join with an is null check is slightly faster than the NOT IN/DISTINCT approach. Though in my tests, the difference was only about 0.5-1.0 ms with 1000 user accounts.

  • AdamPS committed 1328861 on 8.x-2.x 
    Issue #2853289 by shawn_smiley, AdamPS: Warning: Illegal offset type in...
adamps’s picture

adamps commented
Assigned: shawn_smiley » Unassigned
Status: Needs review » Fixed

The above commit changes just the SQL query. For the other parts:

Thanks your help fixing.

Status: Fixed » Closed (fixed)

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

adamps’s picture

adamps commented

The core bug #2744069: views_query_views_alter() does not handle IN queries has just been fixed so we could put this code back to match D7 any time if we want. However it's not clear if there is any particular reason to do so as the new query seems to be fine.