After the views conversion of the admin/people listing, there's some cruft left behind that could be cleaned up.

- Extending $options but $options doesn't exist anymore
- Not using user_load_multiple()
- Unecessary loading of user roles with a query per user
- Loading unecessary data in the query.

See #1932068: People Admin Table Missing Labels and #1751274: Do not query user_roles directly, you can basically re-use all changes to that function from the last issue and additionally remove the $options line from the first. Possibly even more that I haven't noticed yet.

CommentFileSizeAuthor
#12 core-clean-up-user-admin-account-2037675-12.patch1.52 KBlegolasbo
#6 core-clean-up-user-admin-account-2037675-6.patch1.52 KBlegolasbo
#4 core-clean-up-user-admin-account-2037675-4.patch1.52 KBlegolasbo
#2 core-clean-up-user-admin-account-2037675-3.patch1.55 KBlegolasbo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented

By disabling Views, you also should be able to get something on the People page that looks better than.

People without Views

legolasbo’s picture

legolasbo
he/him
Primary language Dutch
Location Middelburg
CreditAttribution: legolasbo commented
Status: Active » Needs review
FileSize
core-clean-up-user-admin-account-2037675-3.patch1.55 KB

Attached patch should fix all of the issues mentioned above.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Needs work 
+++ b/core/modules/user/user.admin.incundefined
@@ -28,22 +28,20 @@ function user_admin_account() {
+  $result = $query->execute()
+    ->fetchAllAssoc('uid');

You can use $uids = $query->execute()->fetchCol(), then you don't need the array_keys() below.

You also have some trailing spaces in the patch.

legolasbo’s picture

legolasbo
he/him
Primary language Dutch
Location Middelburg
CreditAttribution: legolasbo commented
FileSize
core-clean-up-user-admin-account-2037675-4.patch1.52 KB

Thanks for your feedback Berdir.
Apparently I forgot to enable automatic removal of trailing whitespace when I got my new laptop. Attached patch uses fetchCol() and has been cleared of whitespace.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Patch looks good except that it's unfortunately inversed :)

legolasbo’s picture

legolasbo
he/him
Primary language Dutch
Location Middelburg
CreditAttribution: legolasbo commented
FileSize
core-clean-up-user-admin-account-2037675-6.patch1.52 KB

Haha, this should do it then.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review

Thanks, remember to set issues to needs review after uploading a patch.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Reviewed & tested by the community

Looks good I think.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

There should be probably a follow up to use a entity list controller for this piece of code?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

#1938884: Replace the fallback user listing with a list controller exists, is blocked.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/user/user.admin.incundefined
@@ -28,22 +28,20 @@ function user_admin_account() {
   $roles = array_map('check_plain', user_role_names(TRUE));

This can be removed as well as $roles is now unused.

legolasbo’s picture

legolasbo
he/him
Primary language Dutch
Location Middelburg
CreditAttribution: legolasbo commented
Status: Needs work » Needs review
FileSize
core-clean-up-user-admin-account-2037675-12.patch1.52 KB

Attached patch also removes the line spotted by alexpott.

Status: Needs review » Needs work
Issue tags: -Novice

The last submitted patch, core-clean-up-user-admin-account-2037675-12.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
Issue tags: +Novice

#12: core-clean-up-user-admin-account-2037675-12.patch queued for re-testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Reviewed & tested by the community

Applied the patch and confirmed that there are no unused variables or something else to clean-up left.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 5600df4 and pushed to 8.x. Thanks!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Fixed » Reviewed & tested by the community

Great. Let's do all the injection stuff (use the entity storage controller) in the followup.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Reviewed & tested by the community » Fixed
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Can someone explain the following code, I seem to be missing something:

+++ b/core/modules/user/user.admin.inc
@@ -28,22 +28,19 @@ function user_admin_account() {
+    unset($users_roles[0]);
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

As discussed, opened #2057999: Add argument to AccountInterface::getRoles() that allows to exclude the anonymous/authenticated roles

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