Task

Convert the following theme functions to use the new table #type:

Module Theme function name Where in Code What is it really?
user theme_user_admin_permissions function form table
user theme_user_admin_roles function form table draggable

Related issues

CommentFileSizeAuthor
#17 1938938-15-user-tables.patch5.8 KBRisse
#15 1938938-15-user-tables.patch5.97 KBRisse
#6 1938938-6-user-tables.patch5.76 KBduellj
#1 1938938-1-user-tables.patch4.39 KBduellj
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

duellj’s picture

duellj CreditAttribution: duellj commented
Status: Active » Needs review
FileSize
1938938-1-user-tables.patch4.39 KB

First pass at converting theme_user_admin_roles to table #type. Currently there doesn't seem to be a way to add attributes to table #type columns, so theme_user_admin_permissions can't be converted until that's fixed.

duellj’s picture

duellj CreditAttribution: duellj commented
Status: Needs review » Needs work

Waiting on this issue to allow table cell attributes: #1948374: #type 'table' allow attributes on table cells

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

This issue could be marked as duplicate of #1872870: Implement a RoleListController and RoleFormController
Roles will get this controllers anyway

duellj’s picture

duellj CreditAttribution: duellj commented

Thanks anypost, removed theme_user_admin_roles() from the list of conversions. theme_user_admin_permissions() will still need to be converted.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Related issue #229193: Incremental filter for permissions page

duellj’s picture

duellj CreditAttribution: duellj commented
Title: Convert user theme tables to table #type » Convert theme_user_admin_permissions() to table #type
Status: Needs work » Needs review
FileSize
1938938-6-user-tables.patch5.76 KB

Attached patch converts theme_user_admin_permissions to table #type. There's no interdiff, since it's unrelated to the patch from #1 (which converts the roles table). Also updating title to reflect new scope.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue tags: +Needs manual testing

Patch looks ready, needs manual testing for attributes

+++ b/core/modules/user/user.admin.incundefined
@@ -320,12 +320,33 @@ function user_admin_permissions($form, $form_state, $rid = NULL) {
+  $form['permissions'] = array(
+    '#type' => 'table',
+    '#header' => array(t('Permission')),
+    '#id' => 'permissions',
...
+      $form['permissions'][$module] = array(array(
+        '#wrapper_attributes' => array(
+          'colspan' => count($role_names) + 1,
+          'class' => array('module'),
+          'id' => 'module-' . $module,
+        ),
         '#markup' => $module_info[$module]['name'],
-        '#id' => $module,

@@ -333,33 +354,33 @@ function user_admin_permissions($form, $form_state, $rid = NULL) {
+        $form['permissions'][$perm]['description'] = array(
+          '#wrapper_attributes' => array(
+            'class' => array('permission'),
...
+          $form['permissions'][$perm][$rid] = array(
...
+            '#wrapper_attributes' => array(
+              'class' => array('checkbox'),

Is there a way to meka it consistent?

'#attributes' => array('id'=>...)

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Issue tags: +Novice

Tagging.

neochief’s picture

neochief CreditAttribution: neochief commented
Status: Needs review » Needs work

Isn't the #markup stuff should be filtered with at least filter_xss_admin() ? I've tried to create a module with

tag in name and the script was launched on the page with permissions. I know that it's an edge case (if there's an edge for edge cases, that should probably be it), but that's a XSS anyway.
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Hey, this one should be postponed for #1872876: Turn role permission assignments into configuration.
Also should be build on top of FormInterface and provided as route #1971384: [META] Convert page callbacks to controllers

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented 
+++ b/core/modules/user/user.admin.incundefined
@@ -320,12 +320,33 @@ function user_admin_permissions($form, $form_state, $rid = NULL) {
+      $form['permissions'][$module] = array(array(
+        '#wrapper_attributes' => array(
+          'colspan' => count($role_names) + 1,
+          'class' => array('module'),
+          'id' => 'module-' . $module,
+        ),
         '#markup' => $module_info[$module]['name'],
-        '#id' => $module,

$module should be safe

+++ b/core/modules/user/user.admin.incundefined
@@ -333,33 +354,33 @@ function user_admin_permissions($form, $form_state, $rid = NULL) {
+        $form['permissions'][$perm]['description'] = array(
+          '#wrapper_attributes' => array(
+            'class' => array('permission'),
+          ),
           '#type' => 'item',
           '#markup' => $perm_item['title'],
           '#description' => theme('user_permission_description', array('permission_item' => $perm_item, 'hide' => $hide_descriptions)),

Better make it a render array.
#theme => #user_permission_description

+++ b/core/modules/user/user.admin.incundefined
@@ -333,33 +354,33 @@ function user_admin_permissions($form, $form_state, $rid = NULL) {
         foreach ($role_names as $rid => $name) {
...
+            '#title' => $name . ': ' . $perm_item['title'],

Role name could be entered within UI so check_plain() needed here because seems checkbox render does not filters it's title

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented 
+++ b/core/modules/user/user.admin.incundefined
@@ -385,49 +406,6 @@ function user_admin_permissions_submit($form, &$form_state) {
-        $form['checkboxes'][$rid][$key]['#title'] = $roles[$rid] . ': ' . $form['permission'][$key]['#markup'];

seems role name was not filtered before

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs work » Needs review
Issue tags: -Novice, -Needs manual testing

#6: 1938938-6-user-tables.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice, +Needs manual testing

The last submitted patch, 1938938-6-user-tables.patch, failed testing.

Risse’s picture

Risse CreditAttribution: Risse commented
Status: Needs work » Needs review
FileSize
1938938-15-user-tables.patch5.97 KB

Updated duelljs' patch on message #6, should apply correctly.

Status: Needs review » Needs work

The last submitted patch, 1938938-15-user-tables.patch, failed testing.

Risse’s picture

Risse CreditAttribution: Risse commented
Status: Needs work » Needs review
FileSize
1938938-15-user-tables.patch5.8 KB

The previous patch had some bad line endings, this one applies ok.

Brandonian’s picture

Brandonian CreditAttribution: Brandonian commented
Status: Needs review » Reviewed & tested by the community

Patch looks good to me. User permissions page renders properly, still functional. Marking as reviewed and tested.

alexpott’s picture

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

Committed c874593 and pushed to 8.x. Thanks!

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Removing theme_user_admin_roles