Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
user.module
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
2 Mar 2013 at 21:38 UTC
Updated:
29 Jul 2014 at 21:58 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
floydm commentedJust checking, the approach used in the Content table is correct? Invisible labels is the same div as the checkbox?
Comment #2
mgiffordYup. That looks good. The
for=needs to be associated with anid=with the same name.http://wave.webaim.org/toolbar/ is a great tool for this type of simple evaluation.
Comment #3
floydm commentedThe attached patch adds one line to user.admin.inc to add the invisible people admin labels.
The change is just adding
I believe the check_plain is appropriate there but I'm not certain.
Comment #4
mgiffordAwaiting the bot &
check_plain()confirmation but it looks good to me. Assuming those come back good I'd say this is RTBC.Comment #5
mgifford#3: drupal-people_admin_add_missing_labels-1932068-3.patch queued for re-testing.
Comment #6
mgiffordJust tested this again on SimplyTest.me and it's still working fine so marking it RTBC. It got lost in the issue queue.
Comment #8
mgifford#3: drupal-people_admin_add_missing_labels-1932068-3.patch queued for re-testing.
Comment #9
mgiffordOk, that last failure was due to the bot (and me not waiting to see if it would apply correctly).
Setting it back to RTBC.
Comment #10
xjm#3: drupal-people_admin_add_missing_labels-1932068-3.patch queued for re-testing.
Comment #12
mgifford#3: drupal-people_admin_add_missing_labels-1932068-3.patch queued for re-testing.
Comment #13
andymartha commentedOn a fresh install of Drupal 8, I can confirm that the patch drupal-people_admin_add_missing_labels-1932068-3.patch by Floydm on #3 produces the label output seen in the screenshots below (before and after)
Comment #14
alexpottCommitted de0cbdd and pushed to 8.x. Thanks!
Comment #15
berdirHm, shouldn't this use $account->label(), which in turn invokes user_format_name()/hook_user_format_name_alter() just like theme('username'), so that in case of e.g. realname.module, the real name is displayed, just like in the name column which uses the theme function?
Comment #16
xjmThis entire table is being replaced in #1851086: Replace admin/people with a View, so it would be great if folks in this issue could help make sure these improvements are re-added over there.
Comment #17
alexpottRe #15... interesting in light of #1929420: $account->getDisplayName() should be used when outputing username in RDF module ... should this actually be using
user_format_name()Comment #18
mgiffordNow in Core with #1851086: Replace admin/people with a View
Comment #19
berdirThis code is still in place in the non-views fallback implementation.
Which has a number of other weird issues, like not using load multiple and executing completely unneccesary queries to load user roles, see my changes to that function in #1751274: Do not query user_roles directly. I'm fine with creating a follow-up issue to clean up that function separately and fix this too. Could be a nice novice issue, do you have time to open one?
Comment #20
mgifford#3: drupal-people_admin_add_missing_labels-1932068-3.patch queued for re-testing.
Comment #21
mgiffordThanks @Berdir - so hopefully the patch from #3 still applies. user_admin_account() is still there, so should be fine.
Comment #23
mgiffordThe patch in #3 is already in Core, so that should address the non-views fallback, right?
Comment #24
berdirPatch #3 is in core but is wrong, that's why I re-opened this issue. It should be check_plain($account->label())
Comment #25
mgiffordWell, here's a re-roll with that change.
Comment #26
mgiffordNow how do you test this? Trying to disable Views gives me:
Symfony\Component\Routing\Exception\RouteNotFoundException: Route "view.content.page_1"
Building up from a minimal install doesn't seem to give me the right interface either.
Ahh, but you can just disable the View for People in the Views interface, right... That gets me:
But really no closer to finding this label.
Comment #27
berdirOh, that's because there are no checkboxes on that table anymore of course, which makes this dead code. Let's really close this then and continue in #2037675: Clean-up user_admin_account()