Follow-up issue for #1479454: Convert user roles to configurables.
Blocked by:
#1479454: Convert user roles to configurables
#80855: Add element #type table and merge tableselect/tabledrag into it
#1855402: Add generic weighting (tabledrag) support for config entities (DraggableListController)
We will have to fix some of the existing hacks that we use in the admin forms for user roles currently a long the way. (@see user_admin_roles() and how it is currently used.)
More to be added soon.
Related issues
#1946466: Convert all confirm_form() in user.module and user.pages.inc to the new form interface and convert route
Follow-up
#1992428: Convert Roles management to a Controller
Comment | File | Size | Author |
---|---|---|---|
#46 | 1872870-fu-fail.patch | 850 bytes | andypost |
#46 | 1872870-fu-46.patch | 981 bytes | andypost |
#45 | 1872870-fu-fail.patch | 772 bytes | andypost |
#45 | 1872870-fu-45.patch | 902 bytes | andypost |
#41 | interdiff.txt | 2.03 KB | andypost |
Comments
Comment #1
andypostWhile there's no consensus on #1855402: Add generic weighting (tabledrag) support for config entities (DraggableListController)
Patch add form and list controllers.
- UI normalized to all admin listings (there's no Add role within table)
- Introduced Add role local action
- Edit/Delete now local tabs
- Removed theme_user_admin_roles()
Comment #2
andypost@fubhy what's left here?
Comment #3
fubhy CreditAttribution: fubhy commentedI guess we can get this committed and refactor it once #1855402: Add generic weighting (tabledrag) support for config entities (DraggableListController) lands. I don't consider that to be a blocker for this issue anymore.
RTBC anyone?
Comment #4
alexpott#1: 1872870-roles-1.patch queued for re-testing.
Comment #6
andypostChasing HEAD:
Comment #7
alexpottSome minor nits...
Missing @file
Unnecessary documentation.
Unnecessary documentation.
Afaik unnecessary leading \
Either we need to document the @return or not... but this seems inconsistent.
Comment #8
andypostaddressed #7
Currently core does not document result of render array
Comment #9
alexpottWhy have we changed the url from admin/people/roles/edit/{$role->id()} to admin/people/roles/manage/{$role->id()}?
I guess that's because you can also delete the role from this page? But is that necessary now that you can delete from the table using the operations column?
Comment #10
andypost@alexpott This is a common pattern for core, so this issue is a proper place to make this conversion. You can see the same in other issues when ConfigListController involved
Comment #11
sunWhat @andypost said. Wherever we can and technically possible, we should use the de facto standard URL pattern.
This looks pretty good already. However, I found a range of problems in reviewing and testing this, which need to be addressed:
Didn't we default the 'source' to 'label' already?
Do we still need these two?
Happy to defer it to a separate issue, but I don't really understand why we don't have the regular "Save" and "Delete" buttons here... (I perfectly understand it from a UX perspective, but these one-off adjustments are disturbing)
The conditional control structure logic for the save status is not exactly right here. It seems it wasn't right before already.
When just hitting the "Save role" button without any changes, the submit handler prints "has been renamed".
I believe we should at least change this to "has been updated".
The "Edit" operation should be the default operation.
People should not look at the isolated permissions of a single role by default. Security always needs to be evaluated for all roles in combination, because users have multiple roles.
The "Edit permissions" operation is to roles like the "List terms" operation is to taxonomy vocabularies — i.e., the operation is not about the direct entity at hand.
I still believe we badly need to split the dropbuttons in these cases into two dropbuttons; one for the direct entity at hand (role/vocabulary), and another one for the related/inherited entities (permissions/terms).
In testing this, I get a "Disable" operation for each role.
1) Unless I missed a recent change, then user roles cannot be disabled.
2) In any case, the built-in roles cannot be disabled.
Also, typo in code comment:
English irregular past tense: After building something in, it is built and "built-in".
API change to be stated in the change notice:
The $form_id changed from user_admin_role to user_role_form.
Hm. I fear the addition of a local action and removal of the embedded add form belongs into a separate issue, and requires sign-off and testing from the usability team.
If you manage to pull in @Bojhan or @yoroy into this issue and get a sign-off for this change, then we can do it right here.
Otherwise, let's do the straight conversion to entity controllers here only.
The weight can be omitted for default local tasks now.
This task appears as tab but should not. It needs a context of MENU_CONTEXT_INLINE.
Comment #12
sunTagging to loop in the usability team.
To clarify why this technical patch needs UX review/sign-off:
Comment #13
yoroy CreditAttribution: yoroy commentedAlthough the current way is a nice and quick way to add a new role, I agree we should make this more predictable and thus apply the standard pattern. Go ahead.
(Gee, quite a lot of help text on this page! #1952064: Shorten help text on User Roles page )
Comment #14
andypostNew patch addressed #11 changes
Comment #16
andypostFix tests
Comment #17
andypost#16: 1872870-roles-16.patch queued for re-testing.
Comment #19
andypostRemoved Delete tab
Comment #20
andypostComment #21
tim.plunkettI think this now has to go at the bottom of the method. I've seen that mentioned in other issues.
Otherwise, this is perfect! Awesome work @andypost.
Comment #22
andypostSure, to allow language and other play with form items!
Comment #23
tim.plunkett+1! Thanks.
Comment #25
tim.plunkett#22: 1872870-roles-22.patch queued for re-testing.
Comment #26
decafdennis CreditAttribution: decafdennis commentedIn #22, the role labels link to admin/people/roles/manage/ROLE. This path happens to render the role editing form, so it's not necessarily broken. However, the role label being a link is inconsistent with other pages, such as taxonomy and content type administration pages.
I propose the role labels are not links. Otherwise patch looks great!
Comment #28
andypost@naquah Your patch makes no sense! Bot sometimes gives wrong testing results and as you see #22 passes tests.
Link should be a render array and point to role edit form.
Comment #29
Bojhan CreditAttribution: Bojhan commented@tim,plunkett Is right in that this shouldn't be linked. We tend to avoid linking to things that aren't "pages" like content items, or listings. Since this isn't and all it does is go to edit, just using a dropbutton for that would suffice.
Comment #30
andypostSo RTBC #26 patch by @naquah
Comment #31
decafdennis CreditAttribution: decafdennis commented@Bojhan, andypost: thanks!
Comment #32
alexpottReviewing patch in #26... needs work for a new docs standard. NOTE the patch to work with is: https://drupal.org/files/1872870-roles-24.patch
{@inheritdoc}
Comment #33
andypostre-roll to fix docs
While on tris fixed a controller doc-blocks
if no reason or out of scope the commiter will decide (the patch with "no-comments" skiping the controller changes
Comment #34
alexpottAdding interdiff of 1872870-roles-33.patch and 1872870-roles-33-no-comments.patch to show what @andypost is going on about in #33...
Basically he has also fixed
\Drupal\user\RoleStorageController
for the latest coding standards. Which I think is fine.Comment #35
andypostRe-roll after #1807042: Reorganizie entity storage/list/form/render controller annotation keys
doc-block comments for storage in patch
Comment #37
andypostMissed the comma
Comment #38
andyposttagged
Comment #39
andypostre-roll
Comment #41
andypostRe-roll with new EntityFormController
Comment #42
podarok#41 looks good for me
Comment #43
alexpottCommitted 0bdc4cc and pushed to 8.x. Thanks!
Now we just need to convert all this to WSCII :)
And I guess we need a change notice to explain the parts of this change that affect site builders.
Comment #44
andypostFiled follow-up #1992428: Convert Roles management to a Controller and delete form conversion issue #1946466: Convert all confirm_form() in user.module and user.pages.inc to the new form interface and convert route
The change notice should point about:
Form builders:
user_admin_roles()
=>user_admin_roles_form()
and introduced local action to add roleuser_admin_role()
=> usesEntityFormController
theme_user_admin_roles()
removedPath normalized to follow core standard
admin/people/roles/manage
Comment #45
andypostUpdated http://drupal.org/node/1818734 and http://drupal.org/node/1734556
Probably it needs a separate change notice for Form name changes and path
Also here follow-up patch
Comment #46
andypostneeds stronger condition
Comment #47
andypostThis code added to follow-up issue #1992428-6: Convert Roles management to a Controller
Comment #48
andypostFiled change notice
Comment #49
amateescu CreditAttribution: amateescu commentedThe change notice looks good to me, demoting to normal for the followup.
Comment #49.0
amateescu CreditAttribution: amateescu commentedAdded related
Comment #50
andypostFollow-up to improve test merged in upcoming #1992428-9: Convert Roles management to a Controller
Comment #51.0
(not verified) CreditAttribution: commentedfollow-up