Follow-up to #2049921: Update the markup in UserMultipleCancelConfirm::buildForm()

Problem/Motivation

    $form['comments'] = array(
      '#prefix' => '<ul>',
      '#suffix' => '</ul>',
      '#tree' => TRUE,
    );
    // array_filter() returns only elements with actual values.
    $comment_counter = 0;
    $this->comments = $this->commentStorage->loadMultiple(array_keys(array_filter($edit['comments'])));
    foreach ($this->comments as $comment) {
      $cid = $comment->id();
      $form['comments'][$cid] = array(
        '#type' => 'hidden',
        '#value' => $cid,
        '#prefix' => '<li>',
        '#suffix' => Html::escape($comment->label()) . '</li>'
      );
      $comment_counter++;
    }

There is no need to use '#prefix' and '#suffix' with hidden fields.

Proposed resolution

Update the markup, and move the labels to a new list element.

Remaining tasks

Review.
RTBC.
Commit.

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#6 update_the_markup_in-2851849-6.patch3.72 KBjibran
#6 interdiff-7463c1.txt2.37 KBjibran
#5 update_the_markup_in-2851849-5.patch1.35 KByogeshmpawar
#2 update_the_markup_in-2851849-2.patch1.43 KBjibran
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jibran created an issue. See original summary.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer and at PreviousNext for University of Technology, Sydney commented
Status: Active » Needs review
FileSize
update_the_markup_in-2851849-2.patch1.43 KB

Here we go.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs review » Reviewed & tested by the community

Looks great! Thanks

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Needs work

I feel like I committed this patch already but it was a different one.

+++ b/core/modules/comment/src/Form/ConfirmDeleteMultiple.php
@@ -81,24 +81,24 @@ public function getConfirmText() {
+      $labels[$cid] = Html::escape($comment->label());

The escape should no longer be necessary since it's not in the suffix anymore. Can we test that and remove if so? There could even already be test coverage; try putting XSS in there and see what happens. ;) If that passes then we need a small XSS test also.

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location 🇮🇳 Pune(MH)
CreditAttribution: yogeshmpawar at QED42 commented
Status: Needs work » Needs review
FileSize
update_the_markup_in-2851849-5.patch1.35 KB

Updated patch as per comment #4 & also previous patch failed to apply on 8.4.x branch

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer and at PreviousNext for University of Technology, Sydney commented
FileSize
interdiff-7463c1.txt2.37 KB
update_the_markup_in-2851849-6.patch3.72 KB

Fixed #4.

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location 🇮🇳 Pune(MH)
CreditAttribution: yogeshmpawar at QED42 commented

Any Update on this issue ?

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented

We need a review/RTBC here.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at Dx Experts commented
Status: Needs review » Reviewed & tested by the community

Changes look good and make sense, all feedback from @xjm has been addressed, back to RTBC.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at Dx Experts commented
Issue summary: View changes

Quick update of the IS to include the move of the labels.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr at Northern Commerce commented
Title: Update the markup in ConfirmDeleteMultiple::buildForm() » Update the markup in comment module's ConfirmDeleteMultiple::buildForm()

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: update_the_markup_in-2851849-6.patch, failed testing.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs work » Reviewed & tested by the community

Don't know what happened there.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Component: markup » comment.module
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/core/modules/comment/src/Form/ConfirmDeleteMultiple.php
@@ -81,24 +81,24 @@ public function getConfirmText() {
+    $labels = [];
     foreach ($this->comments as $comment) {
       $cid = $comment->id();
+      $labels[$cid] = $comment->label();
       $form['comments'][$cid] = [
         '#type' => 'hidden',
         '#value' => $cid,
-        '#prefix' => '<li>',
-        '#suffix' => Html::escape($comment->label()) . '</li>'
       ];
       $comment_counter++;
     }
+    $form['comments']['labels'] = [
+      '#theme' => 'item_list',
+      '#items' => $labels,
+    ];

    Hm, is this quite right? We're now adding a visible item list of labels that's not connected to the hidden list of comment IDs as it is in HEAD, but rather a parallel child as a not-hidden list. Sorry if I'm being dense and failing to understand what the weird HEAD code is doing. Can we manually test and show the markup of the form in HEAD and with the patch?

  2. +++ b/core/modules/comment/src/Tests/CommentAdminTest.php
@@ -65,22 +66,23 @@ public function testApprovalAdminInterface() {
+    $this->assertRaw(Html::escape($subject));

    Would these assertions maybe make sense as assertEscaped()? https://api.drupal.org/api/drupal/core%21modules%21simpletest%21src%21As...

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs work » Closed (outdated)

This got updated in #1986606: Convert the comments administration screen to a view so this can be closed as outdated.