Problem/Motivation

This happens when you have a GeneratedLink or SafeString object in a table select, possibly only if it's the title element.

See https://qa.drupal.org/pifr/test/742093, TMGMT tests are failing badly right now due to this.

Casting to a string is not an option, then the link is escaped.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#11 interdiff.txt4.43 KBdawehner
#11 2571593-11.patch3.57 KBdawehner
#8 fatal_error_cannot_use-2571593-7.patch2.69 KBlauriii
#5 2571593-5.patch12.18 KBdawehner

Comments

Berdir created an issue. See original summary.

effulgentsia’s picture

effulgentsia commented
Priority: Major » Critical

Promoting to Critical until there's an argument for why it's not. Likely broken by #2568977: Replace SafeMarkup::format() in the link generator - it's a bad example to everyone.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Assigned: Unassigned » lauriii
dawehner’s picture

dawehner
Primary language German
commented

I'm sorry but are you sure this is the proper idea here? Wasn't it always just a random coincidence that it works?

dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Needs review
StatusFileSize
new 2571593-5.patch12.18 KB

Just wrote some unit test, was bored.

Status: Needs review » Needs work

The last submitted patch, 5: 2571593-5.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Patch has a bunch of unrelated date time changes.

Not sure what you mean with #4, also don't really know what the code there is supposed to do but it definitely worked before.

maybe we just need a is_array() check there?

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Assigned: lauriii » Unassigned
Status: Needs work » Needs review
StatusFileSize
new fatal_error_cannot_use-2571593-7.patch2.69 KB

Thanks for the tests @dawehner!

imiksu’s picture

imiksu
Location Finland
commented
Status: Needs review » Needs work 
+++ b/core/tests/Drupal/Tests/Core/Render/Element/TableSelectTest.php
@@ -0,0 +1,55 @@
+    $form['#options'][] = [
+      'title' => new Link('my-text', Url::fromRoute('<front>'))
+    ];
+
+    $form['#attributes'] = [];
+
+    Tableselect::processTableselect($form, $form_state, $complete_form);
+  }
+  ¶
+}
+
+}

Extra whitespace

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/lib/Drupal/Core/Render/Element/Tableselect.php
@@ -217,10 +217,17 @@ public static function processTableselect(&$element, FormStateInterface $form_st
+                  $title = t('Update @title', array(
+                    '@title' => $element['#options'][$key]['title']['data']['#title'],
+                  ));

Should we do something similar to the link as well ... ?

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new 2571593-11.patch3.57 KB
new interdiff.txt4.43 KB

Let's fix the critical and just the critical, keep the behaviour as it used to be it.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Reviewed & tested by the community

Unit tests pass and this fixes the fatal error & keeps the functionality as it was before we broke it in the other issue (no $title is set in this case). I also reconfirmed that this fixes the test failures in TMGMT.

The last submitted patch, 5: 2571593-5.patch, failed testing.

alexpott’s picture

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

Committed 340d11b and pushed to 8.0.x. Thanks!

  • alexpott committed 340d11b on 8.0.x 
    Issue #2571593 by dawehner, lauriii, Berdir: Fatal error: Cannot use...

Status: Fixed » Closed (fixed)

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