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

Comments

mgifford’s picture

mgifford
he/him
Primary language English
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
commented
Status: Active » Needs review
StatusFileSize
new 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
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
commented
StatusFileSize
new 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
commented

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

legolasbo’s picture

legolasbo
he/him
Primary language Dutch
Location Middelburg
commented
StatusFileSize
new 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
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
commented
Status: Needs review » Reviewed & tested by the community

Looks good I think.

dawehner’s picture

dawehner
Primary language German
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
commented

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
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
commented
Status: Needs work » Needs review
StatusFileSize
new 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
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
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 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

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

dawehner’s picture

dawehner
Primary language German
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
commented
Status: Reviewed & tested by the community » Fixed
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
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
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.