Problem/Motivation

  • Now that we have SafeMarkup::escape(), which has more intelligence than String::checkPlain(), there's no reason not to use it within String::format().
  • Doing so will allow '@' to be safely used for variables containing HTML without double-escaping.

Proposed resolution

See issue title and patch.

Remaining tasks

Review. Commit.

User interface changes

None.

API changes

Only a break for people who were passing already-escaped text/HTML as @ or % placeholder values and wanting that to get double-escaped. This is unlikely. Such places will now need to call String::checkPlain() themselves on those placeholders to achieve that double-escaping.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because D7 always escaped these placeholders, so it's not yet been decided to do otherwise.
Issue priority Major because it allows easy fixing of many double-escaping bugs left throughout Drupal.
Disruption Only disruptive for modules that are passing already escaped text and wanting it to get double escaped. They now need to be more explicit about wanting that.
CommentFileSizeAuthor
#12 interdiff.txt925 byteseffulgentsia
#12 string-format-autoescape-12.patch8.56 KBeffulgentsia
#7 interdiff.txt1.13 KBeffulgentsia
#7 string-format-autoescape-7.patch8.55 KBeffulgentsia
#6 interdiff.txt2.28 KBeffulgentsia
#6 string-format-autoescape-6.patch8.33 KBeffulgentsia
#4 string-format-autoescape-4-tests-only.patch5.95 KBeffulgentsia
#2 string-format-autoescape-2-tests-only.patch5.6 KBeffulgentsia
string-format-autoescape.patch6.27 KBeffulgentsia
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented

That is a good idea to do regardless of what else we do.

Do we need additional tests for this?

Else this would be RTBC from my side.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
FileSize
string-format-autoescape-2-tests-only.patch5.6 KB

Here's the test addition, but without the change to String. The new test fails and I'm curious if there's any existing test to catch the double-escaping that would happen from the changes to install.core.inc without the corresponding fix to String.

Status: Needs review » Needs work

The last submitted patch, 2: string-format-autoescape-2-tests-only.patch, failed testing.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Status: Needs work » Needs review
FileSize
string-format-autoescape-4-tests-only.patch5.95 KB

Well, not surprising that we don't have full test coverage of the installer. Ok, taking those hunks out then, and attempting some changes that are more interesting. This is still without String changes, so I hope some tests catch these double-escaping problems.

Status: Needs review » Needs work

The last submitted patch, 4: string-format-autoescape-4-tests-only.patch, failed testing.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Status: Needs work » Needs review
FileSize
string-format-autoescape-6.patch8.33 KB
interdiff.txt2.28 KB

Great. At least some RDF tests are catching the double-escaping bugs. So, here's the original changes to String that fixes them. This patch therefore now demonstrates the key benefit of the change, which is we can start using @ even for variables that already contain rendered HTML.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
FileSize
string-format-autoescape-7.patch8.55 KB
interdiff.txt1.13 KB

Some docs improvements based on what I encountered today while working on this issue.

Status: Needs review » Needs work

The last submitted patch, 7: string-format-autoescape-7.patch, failed testing.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented

Nice work!

effulgentsia queued 7: string-format-autoescape-7.patch for re-testing.

The last submitted patch, 7: string-format-autoescape-7.patch, failed testing.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Status: Needs work » Needs review
FileSize
string-format-autoescape-12.patch8.56 KB
interdiff.txt925 bytes

Wow. A test that fails based on just docs. Reworded the docs to not trigger that failure.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented
Status: Needs review » Reviewed & tested by the community

Lol, nice.

I re-reviewed the patch and this is RTBC.

The beta evaluation made me laugh :) - Yes, I really want to keep double-escaped text1111!!!!1111

j/k - Let's get this in.

amateescu’s picture

amateescu CreditAttribution: amateescu commented

I think the changes in the twig files really show that this is a very nice idea/patch :) Since this is an area that is used a lot at runtime, are there any performance concerns for the switch from String::checkPlain() to SafeMarkup::escape()?

+++ b/core/lib/Drupal/Component/Utility/String.php
@@ -74,19 +74,21 @@ public static function decodeEntities($text) {
+   *   - !variable: Inserted as is, with no sanitization or formatting. Only
+   *     use this when the resulting string is being generated for one of:

Just a very minor point here, "use" can go back up at the end of the previous line. Can be easily fixed on commit.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented

#14: One function call more overall, but if escaped already, saving the call to ::checkPlain().

So in the worst case one function call more, in the best case, saving the check_plain operation.

star-szr’s picture

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

This seems to make a lot of sense, thank you very much @effulgentsia!

alexpott’s picture

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

Nice. Committed b33a46b and pushed to 8.0.x. Thanks!

Thank you for adding the beta evaluation to issue summary.

  • alexpott committed b33a46b on 8.0.x 
    Issue #2435493 by effulgentsia: Change String::format()'s '@' and '%'...

Status: Fixed » Closed (fixed)

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