Update the markup in UserMultipleCancelConfirm::buildForm() [#2049921]

Problem/Motivation

See the original report

Proposed resolution

Update the markup.

Remaining tasks

Review.
RTBC.
Commit.

User interface changes

None

API changes

None

Data model changes

None

Original report by @jibran

While reviewing #1946466-58: Convert all confirm_form() in user.module and user.pages.inc to the new form interface and convert route I found.

+++ b/core/modules/user/lib/Drupal/user/Form/UserMultipleCancelConfirm.phpundefined
@@ -0,0 +1,220 @@
+    $form['accounts'] = array('#prefix' => '<ul>', '#suffix' => '</ul>', '#tree' => TRUE);
+    foreach ($accounts as $uid => $account) {
+      // Prevent user 1 from being canceled.
+      if ($uid <= 1) {
+        continue;
+      }
+      $form['accounts'][$uid] = array(
+        '#type' => 'hidden',
+        '#value' => $uid,
+        '#prefix' => '<li>',
+        '#suffix' => String::checkPlain($account->label()) . "</li>\n",
+      );

Me(@jibran) and @tim.plunkett unable to understand why '#prefix' and '#suffix' is used with '#type' => 'hidden'

Comment	File	Size	Author
#7	update_the_markup_in-2049921-7.patch	1.64 KB	jibran
#7	8.3.x: PHP 5.6 & MySQL 5.5 21,476 pass
#5	2049921-5.patch	1.64 KB	jibran
#5	8.3.x: PHP 5.6 & MySQL 5.5 21,473 pass
#5	screenshot-d8.dev 2017-01-29 18-49-25.png	93.96 KB	jibran
#5	screenshot-d8.dev 2017-01-29 18-48-44.png	93.28 KB	jibran

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

swentel CreditAttribution: swentel commented 25 October 2013 at 13:08

Well, it's a neat trick to list the users, but it could probably be nicer :)

Comment #2

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran commented 25 January 2015 at 11:11

Issue summary:

View changes

Can some list the steps to fix this? I can work on that.

Comment #3

25 January 2015 at 11:11

Version:

8.0.x-dev

» 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Comment #4

25 January 2015 at 11:11

Version:

8.1.x-dev

» 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Comment #5

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer and at PreviousNext for University of Technology, Sydney commented 29 January 2017 at 07:54

Version:	8.2.x-dev	» 8.3.x-dev
Category:	Bug report	» Task
Issue summary:	View changes
Status:	Active	» Needs review

File	Size
screenshot-d8.dev 2017-01-29 18-48-44.png	93.28 KB
screenshot-d8.dev 2017-01-29 18-49-25.png	93.96 KB
2049921-5.patch	1.64 KB
8.3.x: PHP 5.6 & MySQL 5.5 21,473 pass

Here we go.
Before

After

Comment #6

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer and at PreviousNext for University of Technology, Sydney commented 29 January 2017 at 07:56

Title:

UserMultipleCancelConfirm::buildForm() using '#prefix' and '#suffix' with '#type' => 'hidden'

» Update the markup in UserMultipleCancelConfirm::buildForm()

Better title.

Comment #7

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer and at PreviousNext for University of Technology, Sydney commented 29 January 2017 at 18:00

File	Size
update_the_markup_in-2049921-7.patch	1.64 KB
8.3.x: PHP 5.6 & MySQL 5.5 21,476 pass

Minor docs improvement.

Comment #8

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett at Acquia commented 30 January 2017 at 22:37

Status:

Needs review

» Reviewed & tested by the community

That is still one of the strangest bits of code I've seen. Thanks @jibran for fixing it!

Comment #9

xjm

she/her

English

CreditAttribution: xjm at Acquia commented 31 January 2017 at 18:03

Version:

8.3.x-dev

» 8.4.x-dev

Comment #10

12 February 2017 at 11:19

xjm committed 9f208e6 on 8.4.x

Issue #2049921 by jibran, tim.plunkett: Update the markup in...

Comment #11

12 February 2017 at 11:20

xjm committed 4082a89 on 8.3.x

Issue #2049921 by jibran, tim.plunkett: Update the markup in...

Comment #12

xjm

she/her

English

CreditAttribution: xjm at Acquia commented 12 February 2017 at 11:20

Version:	8.4.x-dev	» 8.3.x-dev
Status:	Reviewed & tested by the community	» Fixed

+++ b/core/modules/user/src/Form/UserMultipleCancelConfirm.php
@@ -104,22 +105,27 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+    $form['accounts'] = ['#tree' => TRUE];

At first I was going to ask why this was needed since it seemed out of scope. Usually I've seen this used for the data structure, not the rendering. Then I read:
https://www.drupal.org/docs/7/api/form-api/tree-and-parents

Normally when working with submitted data in $form_state['values'] the data is flattened and does not maintain the structure of the $form array used to generate the form. This behavior can be changed using the #tree property.

Okay, that sounds applicable. And the manual testing shows the fix works.

The updated structure is indeed much cleaner and more readable. Thanks! Committed to 8.4.x. I also reconsidered and chose to backport this to 8.3.x because slightly changing the render array structure in alpha is worth these branches not diverging in a confusing and ugly way.

Comment #13

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer and at PreviousNext for University of Technology, Sydney commented 12 February 2017 at 12:09

1 file was hidden/shown/deleted

File	Size
2049921-5.patch	1.64 KB
8.3.x: PHP 5.6 & MySQL 5.5 21,473 pass

Thanks! @xjm. Here is a fun fact at one point we thought this was a security issue. I'm going to create another issue for \Drupal\comment\Form\ConfirmDeleteMultiple::buildForm() to fix the markup.

Comment #14

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer and at PreviousNext for University of Technology, Sydney commented 12 February 2017 at 12:27

Created #2851849: Update the markup in comment module's ConfirmDeleteMultiple::buildForm(). The security issue reminds me that is it worth a security hardening to change the '#type' => 'hidden', to '#type' => 'value', in the follow-up?