Part of #1971384: [META] Convert page callbacks to controllers
Also follow-up for #1872870: Implement a RoleListController and RoleFormController

Roles now a Config Entity with form and list controllers, so we need to get rid of hook_menu() here
-Also add RoleAccessController because delete has special check for system roles

For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

andypost’s picture

Status: Active » Needs review
Issue tags: +Configurables
FileSize
8.8 KB

Suppose Delete form should be included in the patch

Status: Needs review » Needs work

The last submitted patch, 1992428-roles-2.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
8.67 KB
3.2 KB

reverted back unneeded changes

dawehner’s picture

+++ b/core/modules/user/lib/Drupal/user/Form/UserRoleDelete.phpundefined
@@ -0,0 +1,71 @@
+  protected $user_role;
...
+    $this->user_role = $user_role;

should be $userRole or just $role?

+++ b/core/modules/user/lib/Drupal/user/Form/UserRoleDelete.phpundefined
@@ -0,0 +1,71 @@
+  public function buildForm(array $form, array &$form_state, Role $user_role = NULL) {

It still feels kind of wrong to not document new parameters ...

+++ b/core/modules/user/lib/Drupal/user/RoleAccessController.phpundefined
@@ -0,0 +1,34 @@
+ * Defines the access controller for the user entity type.

... actually for the role entity type :)

andypost’s picture

FileSize
8.91 KB
3.38 KB

Fix #5 and add changes from #1872870-46: Implement a RoleListController and RoleFormController (suppose better to fix this as follow-up, not here)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/UserRoleUpgradePathTest.phpundefined
@@ -39,8 +39,10 @@ public function testRoleUpgrade() {
+    $this->assertRaw('gärtner', 'Role edit page for "gärtner" was found.');

Oh drupal, please don't but in german words :(

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/user/lib/Drupal/user/Form/UserRoleDelete.phpundefined
@@ -0,0 +1,73 @@
+use Drupal\user\Plugin\Core\Entity\Role;
...
+   * @var \Drupal\user\Plugin\Core\Entity\Role
...
+   * @param \Drupal\user\Plugin\Core\Entity\Role $user_role
...
+  public function buildForm(array $form, array &$form_state, Role $user_role = NULL) {

This should typehint with \Drupal\user\RoleInterface

Otherwise it's perfect

andypost’s picture

Status: Needs work » Needs review
FileSize
8.89 KB
1.22 KB

nice catch!

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Perfect!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 3282c15 and pushed to 8.x. Thanks!

andypost’s picture

Status: Fixed » Needs review
FileSize
744 bytes

Here's a follow-up patch to cleanup left functions that obsolete

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This code is not used anymore, checked via storm

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Yay for less code!

Committed ff0f94d and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.