Follow-up to #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping

Problem/Motivation

The DiffFormatter is an HTML display of a diff - it needs to be able to display file diffs in a browser by escaping the markup the text contains. It also adds markup to denote which parts of each line are different - this should not be escaped.

Proposed resolution

Remove usage of SafeMarkup::checkPlain() in DiffFormatter and make it responsible for all escaping.

Remaining tasks

  • Review
  • Commit

User interface changes

None

API changes

The behaviour of HWLDFWordAccumulator is slightly changed to no longer escape markup - but it never should have been doing that.

Data model changes

None

CommentFileSizeAuthor
#5 2560055.5.patch7.23 KBalexpott
#5 2-5-interdiff.txt2.59 KBalexpott
#2 2560055.2.patch7.16 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 2560055.2.patch7.16 KB

The patch adds assertion to the config diff test that exercise every line changed here.

star-szr’s picture

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

Looks great, I like the added test coverage. RTBC if green :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 2560055.2.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 2-5-interdiff.txt2.59 KB
new 2560055.5.patch7.23 KB

Made the assertions even better and fixed an incorrect one.

stefan.r’s picture

stefan.r commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC as this should be green now that we do

+++ b/core/modules/config/src/Tests/ConfigImportUITest.php
@@ -332,10 +332,10 @@ function testImportDiff() {
-    $this->assertText(Html::escape('<p>foobar</p>'));
...
+    $this->assertText(Html::escape("404: '<em>herp</em>'"));
catch’s picture

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

Committed/pushed to 8.0.x, thanks!

  • catch committed 4ba004d on 8.0.x 
    Issue #2560055 by alexpott: Remove all usages SafeMarkup::checkPlain()...

Status: Fixed » Closed (fixed)

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