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

CommentFileSizeAuthor
#12 1992428-drop_leftover_code.patch744 bytesandypost
#9 interdiff.txt1.22 KBandypost
#9 1992428-roles-9.patch8.89 KBandypost
#6 interdiff.txt3.38 KBandypost
#6 1992428-roles-6.patch8.91 KBandypost
#4 interdiff.txt3.2 KBandypost
#4 1992428-roles-4.patch8.67 KBandypost
#2 1992428-roles-2.patch8.8 KBandypost

Comments

andypost’s picture

andypost
he/him
Primary language Russian
commented

Role delete form conversion happens in #1946466: Convert all confirm_form() in user.module and user.pages.inc to the new form interface and convert route

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs review
Issue tags: +Configurables
StatusFileSize
new 1992428-roles-2.patch8.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

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new 1992428-roles-4.patch8.67 KB
new interdiff.txt3.2 KB

reverted back unneeded changes

dawehner’s picture

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

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 1992428-roles-6.patch8.91 KB
new interdiff.txt3.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

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

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
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

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new 1992428-roles-9.patch8.89 KB
new interdiff.txt1.22 KB

nice catch!

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Perfect!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

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

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Fixed » Needs review
StatusFileSize
new 1992428-drop_leftover_code.patch744 bytes

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

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

This code is not used anymore, checked via storm

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
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

(not verified) commented
Issue summary: View changes

Updated issue summary.