Problem/Motivation

Now that we have #2555931: Add #plain_text to escape text in render arrays it's time to remove some #markup + SafeMarkup::checkPlain() calls.

Proposed resolution

Convert them to #plain_text.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug, #markup + SafeMarkup::checkPlain() no longer makes any sense.
Issue priority Major because we want to remove SafeMarkup::checkPlain().
Disruption This issue is not disruptive to core or contrib.

Remaining tasks

  • Patch
  • Review

User interface changes

Should be none.

API changes

n/a

Data model changes

n/a

CommentFileSizeAuthor
#16 2559605-16.patch23.7 KBstefan.r
#16 interdiff-14-16.txt1 KBstefan.r
#14 interdiff-10-14.txt2.65 KBstefan.r
#14 2559605-14.patch23.19 KBstefan.r
#10 2559605-10.patch24.47 KBstefan.r
#10 interdiff-7-10.txt3.92 KBstefan.r
#7 interdiff.txt3.49 KBstar-szr
#7 convert_markup-2559605-7.patch21.25 KBstar-szr
#6 interdiff.txt9.03 KBstar-szr
#6 convert_markup-2559605-6.patch17.77 KBstar-szr
#3 interdiff.txt936 bytesstar-szr
#3 convert_markup-2559605-3.patch14.07 KBstar-szr
#2 convert_markup-2559605-2.patch13.82 KBstar-szr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Cottser created an issue. See original summary.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Status: Active » Needs review
FileSize
convert_markup-2559605-2.patch13.82 KB

Here's all the ones I can find.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
FileSize
convert_markup-2559605-3.patch14.07 KB
interdiff.txt936 bytes

After self-reviewing this one I realized this one needs another approach, but it might just be clearer to Html::escape() here. Would like a second opinion on it.

The last submitted patch, 2: convert_markup-2559605-2.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 3: convert_markup-2559605-3.patch, failed testing.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
convert_markup-2559605-6.patch17.77 KB
interdiff.txt9.03 KB

There's definitely some things that are checking for a '#markup' key specifically. I haven't looked into the entity reference things yet.

This patch tries using Html::escape() in Drupal\rest\Plugin\views\display\RestExport, and also removes unused uses for SafeMarkup.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
FileSize
convert_markup-2559605-7.patch21.25 KB
interdiff.txt3.49 KB

Updating the ER tests, hopefully on the right track here.

The last submitted patch, 6: convert_markup-2559605-6.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs review » Needs work 
+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -318,10 +318,10 @@ public function render() {
-      $build['#markup'] = SafeMarkup::checkPlain($build['#markup']);
+      $build['#markup'] = Html::escape($build['#markup']);

This use #plain_text and unset #markup.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Title: Convert #markup + SafeMarkup::checkPlain() to #plain_text » Convert SafeMarkup::checkPlain() in render arrays to #plain_text
Status: Needs work » Needs review
FileSize
interdiff-7-10.txt3.92 KB
2559605-10.patch24.47 KB

This implements #9, also looked for any further possible conversions and found another 2

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@stefan.r I think the DiffFormatter changes will cause double escaping in the the config diff. The parent issue contains a test for this. I'd rather do the very simple and easy to follow changes here than make the changes to DiffFormatter without testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs review » Needs work

Needs for #11

The last submitted patch, 10: 2559605-10.patch, failed testing.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Status: Needs work » Needs review
FileSize
2559605-14.patch23.19 KB
interdiff-10-14.txt2.65 KB

Status: Needs review » Needs work

The last submitted patch, 14: 2559605-14.patch, failed testing.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Status: Needs work » Needs review
FileSize
interdiff-14-16.txt1 KB
2559605-16.patch23.7 KB

Let's see if this fixes the test fail

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

I'm wondering whether we should here think about whether using checkPlain or the corresponding #plain_text is the right thing to do for all those places ...
but I guess at least for now it is the most practical thing to just convert and rethink the usecases later ...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@dawehner yeah - this patch is functionally equivalent to HEAD - I think we should just do the conversions are be done.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

#18 was also meant to RTBC this.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

+1 for the RTBC

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
@@ -480,16 +480,16 @@
-    $expected = Html::escape($serializer->serialize($entities, 'json'));
+    $expected = $serializer->serialize($entities, 'json');
...
-    $expected = Html::escape($serializer->serialize($entities, 'xml'));
+    $expected = $serializer->serialize($entities, 'xml');

I'm not convinced that these test changes aren't hiding a regression.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@Cottser the problem with that test is that is it testing the render array rather than the output - we have tests that confirm #plain_text will always be escaped and we're testing here that #plain_text is set. Afaics there is no regression here.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented 
+++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
@@ -480,16 +480,16 @@ public function testLivePreview() {
-    $rendered_json = $build['#markup'];
-    $this->assertEqual($rendered_json, $expected, 'Ensure the previewed json is escaped.');
+    $rendered_json = $build['#plain_text'];
+    $this->assertTrue(!isset($build['#markup']) && $rendered_json == $expected, 'Ensure the previewed json is escaped.');

@@ -505,7 +505,7 @@ public function testLivePreview() {
-    $rendered_xml = $build['#markup'];
+    $rendered_xml = $build['#plain_text'];
     $this->assertEqual($rendered_xml, $expected, 'Ensure we preview xml when we change the request format.');

These are the lines that prove we don't have a regression.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Yup, just came to that same conclusion myself. Back to RTBC, thanks :)

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented

It's mostly just that this assertion message is no longer accurate IMO, that's what initially threw me:

Ensure the previewed json is escaped

That would be true if we rendered the render array, but since we just check it directly it's not (yet) escaped.

Edit: But like you said we have separate tests that prove that. Just kinda makes this a weak integration test to me.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented

I had that same thought, technically we could render it if we're unsure #plain_text works as expected, I think we did something similar in the checkPlain() issue? @cottser feel free to update if you feel it makes the test stronger.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented

I could go either way on it, we can do it in a follow-up perhaps so as not to hold this up.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented

Cool

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

I also think the straight conversion is OK here.

Committed/pushed to 8.0.x, thanks!

  • catch committed e580940 on 8.0.x 
    Issue #2559605 by Cottser, stefan.r: Convert SafeMarkup::checkPlain() in...

Status: Fixed » Closed (fixed)

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