Comments

dawehner created an issue. See original summary.

lauriii’s picture

Status: Active » Needs review
StatusFileSize
new597 bytes

There is only one usage which is:

$title = t('Update @title', array('@title' => $title));

. Triggering the tests to see what happens..

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/lib/Drupal/Core/Render/Element/Table.php
@@ -200,7 +200,7 @@ public static function processTable(&$element, FormStateInterface $form_state, &
             if (isset($title) && $title !== '') {
-              $title = t('Update !title', array('!title' => $title));
+              $title = t('Update @title', array('@title' => $title));
             }

So yeah I think its totally fine at that point to assume that $title is either as SafeString object OR we deal with an unescaped string.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/lib/Drupal/Core/Render/Element/Table.php
@@ -200,7 +200,7 @@ public static function processTable(&$element, FormStateInterface $form_state, &
             if (isset($title) && $title !== '') {
-              $title = t('Update !title', array('!title' => $title));
+              $title = t('Update @title', array('@title' => $title));
             }

Does anyone know what the dickens this is doing here? It seems very strange to have this in a generic render element.

alexpott’s picture

It's not part of theme_table in Drupal 7.

alexpott’s picture

In was introduced in #80855: Add element #type table and merge tableselect/tabledrag into it with no apparent tests. Weird.

dawehner’s picture

@alexpott
To be clear, merging tableselect into #type table feels wrong, well, this is history.

Some lines higher you see if ($row['selected']) so this is how we know that something changed magically on tableselect. Its incredible how complex it is to figure out the proper title.

dawehner’s picture

So I think removing that behaviour would be out of scope of the issue. Is there anything you think we can do about it?

alexpott’s picture

@dawehner given you've done the work perhaps a comment would help. I think @ is correct because auto-escape but it would be awesome to have a test.

lauriii’s picture

Assigned: Unassigned » lauriii

Working on the test case

lauriii’s picture

Assigned: lauriii » Unassigned
StatusFileSize
new2.5 KB
new3.08 KB
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Works for me

The last submitted patch, 12: replace_remaining-2569293-12-test-only.patch, failed testing.

The last submitted patch, 12: replace_remaining-2569293-12-test-only.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestTableForm.php
@@ -0,0 +1,53 @@
+        '#title' => SafeString::create('<em>flamingo</em>'),

Let's use $this->t() here... something like $this->t('My favourite fruit is <strong>@fruit</strong>', ['@fruit' => 'bananas'])

lauriii’s picture

Status: Needs work » Needs review
StatusFileSize
new3.15 KB
new1.17 KB
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Yeah its indeed better if we are more similar to the actual reality.

alexpott’s picture

Priority: Major » Critical

This is part of the critical meta.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 55118ad and pushed to 8.0.x. Thanks!

diff --git a/core/modules/system/tests/modules/form_test/src/Form/FormTestTableForm.php b/core/modules/system/tests/modules/form_test/src/Form/FormTestTableForm.php
index c7fc112..1a75386 100644
--- a/core/modules/system/tests/modules/form_test/src/Form/FormTestTableForm.php
+++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestTableForm.php
@@ -8,7 +8,6 @@
 namespace Drupal\form_test\Form;
 
 use Drupal\Core\Form\FormStateInterface;
-use Drupal\Core\Render\SafeString;
 
 class FormTestTableForm extends FormTestTableSelectFormBase {
 
@@ -26,7 +25,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
     $form['table'] = [
       '#type' => 'table',
       '#tableselect' => TRUE,
-      '#empty' => t('Empty text.'),
+      '#empty' => $this->t('Empty text.'),
     ];
     $form['table']['row'] = [
       'data' => [

Fixed on commit.

  • alexpott committed d85ee2e on 8.0.x
    Issue #2569293 by lauriii, alexpott, dawehner: Replace remaining !...

Status: Fixed » Closed (fixed)

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