Files: 
CommentFileSizeAuthor
#46 1872870-fu-fail.patch850 bytesandypost
FAILED: [[SimpleTest]]: [MySQL] 55,565 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#46 1872870-fu-46.patch981 bytesandypost
PASSED: [[SimpleTest]]: [MySQL] 55,509 pass(es).
[ View ]
#45 1872870-fu-fail.patch772 bytesandypost
PASSED: [[SimpleTest]]: [MySQL] 55,567 pass(es).
[ View ]
#45 1872870-fu-45.patch902 bytesandypost
PASSED: [[SimpleTest]]: [MySQL] 55,722 pass(es).
[ View ]
#41 interdiff.txt2.03 KBandypost
#41 1872870-roles-41.patch23.31 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,649 pass(es).
[ View ]
#39 1872870-roles-38.patch23.39 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 56,232 pass(es), 7 fail(s), and 9 exception(s).
[ View ]
#37 interdiff.txt642 bytesandypost
#37 1872870-roles-37.patch23.38 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,452 pass(es).
[ View ]
#35 1872870-roles-35.patch23.29 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#34 33-and-33-no-comments-interdiff.txt1.37 KBalexpott
#33 interdiff.txt5.56 KBandypost
#33 1872870-roles-33.patch23.35 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 54,570 pass(es).
[ View ]
#33 1872870-roles-33-no-comments.patch21.85 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 54,486 pass(es).
[ View ]
#28 1872870-roles-22.patch22.48 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 54,404 pass(es).
[ View ]
#26 1872870-roles-24.patch22.36 KBnaquah
PASSED: [[SimpleTest]]: [MySQL] 54,540 pass(es).
[ View ]
#26 interdiff.do-no-test.patch992 bytesnaquah
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#22 interdiff.txt889 bytesandypost
#22 1872870-roles-22.patch22.48 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 54,479 pass(es).
[ View ]
#20 interdiff.txt1.05 KBandypost
#20 1872870-roles-20.patch22.5 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 54,264 pass(es).
[ View ]
#19 interdiff.txt754 bytesandypost
#19 1872870-roles-19.patch22.2 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 54,089 pass(es).
[ View ]
#16 interdiff.txt2.63 KBandypost
#16 1872870-roles-16.patch22.27 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 54,147 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#14 interdiff.txt4.51 KBandypost
#14 1872870-roles-14.patch22.25 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 53,288 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#8 interdiff.txt2.13 KBandypost
#8 1872870-roles-8.patch22.38 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 53,262 pass(es).
[ View ]
#6 interdiff.txt3.77 KBandypost
#6 1872870-roles-6.patch23.04 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 53,295 pass(es).
[ View ]
#1 role-list.png26.95 KBandypost
#1 role-add.png8.91 KBandypost
#1 role-edit.png13.86 KBandypost
#1 1872870-roles-1.patch22.85 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1872870-roles-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new22.85 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1872870-roles-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new13.86 KB
new8.91 KB
new26.95 KB

While 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()

Role listRole addRole edit

@fubhy what's left here?

I 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?

#1: 1872870-roles-1.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 1872870-roles-1.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new23.04 KB
PASSED: [[SimpleTest]]: [MySQL] 53,295 pass(es).
[ View ]
new3.77 KB

Chasing HEAD:

  • List controller implemented on top of FormInterface
  • Default operation is 'Edit permissions' - I think it more useful

Status:Needs review» Needs work

Some minor nits...

+++ b/core/modules/user/lib/Drupal/user/RoleListController.phpundefined
@@ -0,0 +1,156 @@
+/**
+ * Contains \Drupal\user\RoleListController.
+ */

Missing @file

+++ b/core/modules/user/lib/Drupal/user/RoleListController.phpundefined
@@ -0,0 +1,156 @@
+   *
+   * Form constructor for manipulating role weights.
+   *
+   * @param array $form
+   *   An associative array containing the structure of the form.
+   * @param array $form_state
+   *   A reference to a keyed array containing the current state of the form.
+   *
+   * @return array
+   *   The array containing the complete form.

Unnecessary documentation.

+++ b/core/modules/user/lib/Drupal/user/RoleListController.phpundefined
@@ -0,0 +1,156 @@
+   *
+   * Form submission handler for the roles overview form.
+   *
+   * @param array $form
+   *   An associative array containing the structure of the form.
+   * @param array $form_state
+   *   A reference to a keyed array containing the current state of the form.

Unnecessary documentation.

+++ b/core/modules/user/user.admin.incundefined
@@ -849,208 +849,34 @@ function theme_user_permission_description($variables) {
+  return \Drupal::service('plugin.manager.entity')

Afaik unnecessary leading \

/**
* Page callback: Lists roles and allows to reorder them.
*
* @see user_menu()
*/
function user_admin_roles_list() {
  return \Drupal::service('plugin.manager.entity')
    ->getListController('user_role')->render();
}
/**
* Page callback: Presents the role creation form.
*
* @return array
*   A form array as expected by drupal_render().
*
* @see user_menu()
*/
function user_admin_role_add() {
  drupal_set_title(t('Add role'));
  $role = entity_create('user_role', array());
  return entity_get_form($role);
}

Either we need to document the @return or not... but this seems inconsistent.

Status:Needs work» Needs review
StatusFileSize
new22.38 KB
PASSED: [[SimpleTest]]: [MySQL] 53,262 pass(es).
[ View ]
new2.13 KB

addressed #7
Currently core does not document result of render array

+++ b/core/modules/user/lib/Drupal/user/Tests/UserRoleAdminTest.phpundefined
@@ -37,38 +37,38 @@ function testRoleAdministration() {
-    $this->drupalPost("admin/people/roles/edit/{$role->id()}", $edit, t('Save role'));
-    $this->assertText(t('The role has been renamed.'), 'The role has been renamed.');
+    $edit = array('label' => $role_name);
+    $this->drupalPost("admin/people/roles/manage/{$role->id()}", $edit, t('Save role'));

Why 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?

@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

What @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:

+++ b/core/modules/user/lib/Drupal/user/RoleFormController.php
@@ -0,0 +1,95 @@
+      '#machine_name' => array(
+        'exists' => 'user_role_load',
+        'source' => array('label'),

Didn't we default the 'source' to 'label' already?

+++ b/core/modules/user/lib/Drupal/user/RoleFormController.php
@@ -0,0 +1,95 @@
+    // Adjust actions.
+    $actions['submit']['#value'] = !$entity->isNew() ? t('Save role') : t('Add role');
+    $actions['delete']['#value'] = t('Delete role');

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)

+++ b/core/modules/user/lib/Drupal/user/RoleFormController.php
@@ -0,0 +1,95 @@
+    if ($entity->save() == SAVED_UPDATED) {
+      drupal_set_message(t('Role %label has been renamed.', array('%label' => $entity->label())));
...
+    }
+    else {
+      drupal_set_message(t('Role %label has been added.', array('%label' => $entity->label())));

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".

+++ b/core/modules/user/lib/Drupal/user/RoleListController.php
@@ -0,0 +1,140 @@
+    $operations['permissions'] = array(
+      'title' => t('Edit permissions'),
...
+      'weight' => 5,

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).

+++ b/core/modules/user/lib/Drupal/user/RoleListController.php
@@ -0,0 +1,140 @@
+    // Build-in roles could not be deleted.
+    if (in_array($entity->id(), array(DRUPAL_ANONYMOUS_RID, DRUPAL_AUTHENTICATED_RID))) {
+      unset($operations['delete']);
+    }

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".

+++ b/core/modules/user/user.admin.inc
@@ -849,208 +849,31 @@ function theme_user_permission_description($variables) {
-function user_admin_role($form, $form_state, $role) {
...
+  $role = entity_create('user_role', array());
+  return entity_get_form($role);

API change to be stated in the change notice:

The $form_id changed from user_admin_role to user_role_form.

+++ b/core/modules/user/user.module
@@ -961,23 +957,39 @@ function user_menu() {
+  $items['admin/people/roles/add'] = array(
+    'title' => 'Add role',
+    'page callback' => 'user_admin_role_add',
+    'access arguments' => array('administer permissions'),
+    'type' => MENU_LOCAL_ACTION,
+    'weight' => 1,
+    'file' => 'user.admin.inc',
+  );

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.

+++ b/core/modules/user/user.module
@@ -961,23 +957,39 @@ function user_menu() {
+    'type' => MENU_DEFAULT_LOCAL_TASK,
+    'weight' => -10,

The weight can be omitted for default local tasks now.

+++ b/core/modules/user/user.module
@@ -961,23 +957,39 @@ function user_menu() {
+  $items['admin/people/roles/manage/%user_role/delete'] = array(
     'title' => 'Delete role',
...
+    'type' => MENU_LOCAL_TASK,

This task appears as tab but should not. It needs a context of MENU_CONTEXT_INLINE.

Tagging to loop in the usability team.

To clarify why this technical patch needs UX review/sign-off:

  1. This patch attempts to remove the embedded form for creating a new user role from the page that is listing user roles.
  2. Instead, it adds an "Add role" local action link to that listing page, which points to the user role add/edit form page.
  3. In other words, the embedded role addition form is nixed altogether and replaced with the de-facto standard UI pattern we're using everywhere else.
  4. You can manually test this patch very easily via simplytest.me.

Although 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 )

StatusFileSize
new22.25 KB
FAILED: [[SimpleTest]]: [MySQL] 53,288 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
new4.51 KB

New patch addressed #11 changes

Status:Needs review» Needs work

The last submitted patch, 1872870-roles-14.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new22.27 KB
FAILED: [[SimpleTest]]: [MySQL] 54,147 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new2.63 KB

Fix tests

#16: 1872870-roles-16.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs usability review, +Usability

The last submitted patch, 1872870-roles-16.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new22.2 KB
PASSED: [[SimpleTest]]: [MySQL] 54,089 pass(es).
[ View ]
new754 bytes

Removed Delete tab

StatusFileSize
new22.5 KB
PASSED: [[SimpleTest]]: [MySQL] 54,264 pass(es).
[ View ]
new1.05 KB

+++ b/core/modules/user/lib/Drupal/user/RoleFormController.phpundefined
@@ -0,0 +1,91 @@
+  public function form(array $form, array &$form_state, EntityInterface $entity) {
+    $form = parent::form($form, $form_state, $entity);

I 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.

StatusFileSize
new22.48 KB
PASSED: [[SimpleTest]]: [MySQL] 54,479 pass(es).
[ View ]
new889 bytes

Sure, to allow language and other play with form items!

Status:Needs review» Reviewed & tested by the community

+1! Thanks.

Status:Reviewed & tested by the community» Needs work
Issue tags:-Needs usability review, -Usability

The last submitted patch, 1872870-roles-22.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Needs usability review, +Usability

#22: 1872870-roles-22.patch queued for re-testing.

StatusFileSize
new992 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new22.36 KB
PASSED: [[SimpleTest]]: [MySQL] 54,540 pass(es).
[ View ]

In #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!

Status:Needs review» Needs work

The last submitted patch, interdiff.do-no-test.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new22.48 KB
PASSED: [[SimpleTest]]: [MySQL] 54,404 pass(es).
[ View ]

@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.

@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.

Status:Needs review» Reviewed & tested by the community

So RTBC #26 patch by @naquah

@Bojhan, andypost: thanks!

Status:Reviewed & tested by the community» Needs work

Reviewing 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

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/Role.phpundefined
@@ -57,4 +61,17 @@ class Role extends ConfigEntityBase {
+   * Overrides \Drupal\Core\Entity\Entity::uri().
+++ b/core/modules/user/lib/Drupal/user/RoleFormController.phpundefined
@@ -0,0 +1,89 @@
+   * Overrides EntityFormController::form().
...
+   * Overrides EntityFormController::actions().
...
+   * Overrides EntityFormController::save().
...
+   * Overrides EntityFormController::delete().
+++ b/core/modules/user/lib/Drupal/user/RoleListController.phpundefined
@@ -0,0 +1,139 @@
+   * Implements \Drupal\Core\Form\FormInterface::getFormID().
...
+   * Overrides \Drupal\Core\Entity\EntityListController::buildHeader().
...
+   * Overrides \Drupal\Core\Entity\EntityListController::getOperations().
...
+   * Overrides \Drupal\Core\Entity\EntityListController::buildRow().
...
+   * Overrides \Drupal\Core\Entity\EntityListController::render().
...
+   * Implements \Drupal\Core\Form\FormInterface::buildForm().
...
+   * Implements \Drupal\Core\Form\FormInterface::validateForm().
...
+   * Implements \Drupal\Core\Form\FormInterface::submitForm().

{@inheritdoc}

Status:Needs work» Needs review
StatusFileSize
new21.85 KB
PASSED: [[SimpleTest]]: [MySQL] 54,486 pass(es).
[ View ]
new23.35 KB
PASSED: [[SimpleTest]]: [MySQL] 54,570 pass(es).
[ View ]
new5.56 KB

re-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

StatusFileSize
new1.37 KB

Adding 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.

StatusFileSize
new23.29 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Re-roll after #1807042: Reorganizie entity storage/list/form/render controller annotation keys
doc-block comments for storage in patch

Status:Needs review» Needs work

The last submitted patch, 1872870-roles-35.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new23.38 KB
PASSED: [[SimpleTest]]: [MySQL] 55,452 pass(es).
[ View ]
new642 bytes

Missed the comma

tagged

StatusFileSize
new23.39 KB
FAILED: [[SimpleTest]]: [MySQL] 56,232 pass(es), 7 fail(s), and 9 exception(s).
[ View ]

re-roll

Status:Needs review» Needs work

The last submitted patch, 1872870-roles-38.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new23.31 KB
PASSED: [[SimpleTest]]: [MySQL] 55,649 pass(es).
[ View ]
new2.03 KB

Re-roll with new EntityFormController

Status:Needs review» Reviewed & tested by the community

#41 looks good for me

Title:Implement a RoleListController and RoleFormControllerChange notice: Implement a RoleListController and RoleFormController
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active
Issue tags:-Configurables

Committed 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.

Filed 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 role
user_admin_role() => uses EntityFormController

theme_user_admin_roles() removed

Path normalized to follow core standard admin/people/roles/manage

Status:Active» Needs review
Issue tags:-Needs usability review+Needs change record, +Configurables
StatusFileSize
new902 bytes
PASSED: [[SimpleTest]]: [MySQL] 55,722 pass(es).
[ View ]
new772 bytes
PASSED: [[SimpleTest]]: [MySQL] 55,567 pass(es).
[ View ]

Updated 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

StatusFileSize
new981 bytes
PASSED: [[SimpleTest]]: [MySQL] 55,509 pass(es).
[ View ]
new850 bytes
FAILED: [[SimpleTest]]: [MySQL] 55,565 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

needs stronger condition

This code added to follow-up issue #1992428-6: Convert Roles management to a Controller

Title:Change notice: Implement a RoleListController and RoleFormController[Followup] Implement a RoleListController and RoleFormController
Priority:Critical» Normal

The change notice looks good to me, demoting to normal for the followup.

Issue summary:View changes

Added related

Title:[Followup] Implement a RoleListController and RoleFormControllerImplement a RoleListController and RoleFormController
Status:Needs review» Fixed

Follow-up to improve test merged in upcoming #1992428-9: Convert Roles management to a Controller

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

Issue summary:View changes

follow-up