Part of #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping

Problem/Motivation

SafeMarkup::checkPlain() marks strings as safe - there are many usages where this is irrelevant because Twig will auto-escape it for us.

Proposed resolution

Remove unnecessary calls and let Twig auto-escape do its thing. Add test coverage if necessary. Also clean up checkPlain attributes properties because these are all escaped by default and in fact all of these are double escaping bugs.

Remaining tasks

  • Add or find test coverage
  • Review
  • Commit

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#6 2557871.6.patch18.35 KBalexpott
#6 2557871.6.test-only.patch2.13 KBalexpott
#2 2557871.2.patch16.22 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Active » Needs review
FileSize
2557871.2.patch16.22 KB
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented

RTBC++

alexpott’s picture

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

I'm going to add some tests for at least the attributes changes since I suspect that these would have caused double escaping bugs in HEAD.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
2557871.6.test-only.patch2.13 KB
2557871.6.patch18.35 KB

Added tests for the attributes changes where possible - and proved we have double escaping bugs in HEAD.

+++ b/core/modules/filter/filter.module
@@ -7,7 +7,6 @@
@@ -437,7 +436,7 @@ function template_preprocess_filter_tips(&$variables) {

+++ b/core/modules/rdf/rdf.module
@@ -443,7 +442,7 @@ function rdf_preprocess_username(&$variables) {
-    $variables['attributes']['content'] = SafeMarkup::checkPlain($variables['name_raw']);
+    $variables['attributes']['content'] = $variables['name_raw'];

This is not actually testable - user names can not have a character that is escaped in them. See UserNameConstraintValidator.

The test only patch is the interdiff :)

The last submitted patch, 6: 2557871.6.test-only.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community

Even better!

catch’s picture

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

Committed/pushed to 8.0.x, thanks!

  • catch committed a7fc1a2 on 8.0.x 
    Issue #2557871 by alexpott: Remove all usages SafeMarkup::checkPlain()...

Status: Fixed » Closed (fixed)

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