Problem/Motivation

SafeMarkup::escape() is unnecessary and badly named.

Using SafeMarkup::escape() inside SafeMarkup::format() unnecessarily marks the arguments passed to SafeMarkup::format() as safe. All usages of SafeMarkup::escape() outside of the SafeMarkup class have been shown to be incorrect.

The name SafeMarkup::escape() suggests it always escapes but only does this if the provided string is not already marked safe. In fact SafeMarkup::checkPlain() always escapes!

Proposed resolution

Remove the method.

Remaining tasks

Review
Commit

User interface changes

None

API changes

SafeMarkup::escape() is removed. All other code is functionally the same.

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Needs review » Needs work
alexpott’s picture

Status: Needs work » Needs review
FileSize
25.76 KB

Patch contains #2501697: Remove SafeMarkup::set in rdf_preprocess_comment(), a working patch from #2505931: Remove SafeMarkup::set in ViewListBuilder and Html::encodeEntities and FormErrorHandlerTest from #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping.

This patch has the nice side effect of no longer mark @ replacements in SafeMarkup::format() as safe unnecessarily.

Status: Needs review » Needs work

The last submitted patch, 3: 2549393.3.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.09 KB
26.27 KB

Fixing the tests. Wouldn't it be nice to have a non safe markup version of SafeMarkup::format.

alexpott’s picture

Category: Task » Bug report
FileSize
1.02 KB

Oh and if anyone needs convincing that these SafeMarkup functions don't need to be removed check this out... our tests are only currently passing because of the order arguments to a t() call. We are relying on the an argument being marked safe and the subsequent ! argument being considered safe because of the earlier one. Methinks this makes this a bug fix.

Status: Needs review » Needs work

The last submitted patch, 6: d8.gonna-fail.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
olli’s picture

+++ b/core/lib/Drupal/Component/Utility/Html.php
@@ -348,4 +348,48 @@ public static function decodeEntities($text) {
+   * Encodes all HTML entities.
...
+    return htmlspecialchars($text, ENT_QUOTES, 'UTF-8');
...
+   * Encodes all HTML entities if $text is not marked safe.

How about "Encodes special characters to HTML entities."? Maybe rename to Html::escape() or ::checkPlain()?

Wim Leers’s picture

+++ b/core/lib/Drupal/Component/Utility/Html.php
@@ -348,4 +348,48 @@ public static function decodeEntities($text) {
+  public static function encodeEntities($text) {
...
+  public static function encodeEntitiesIfUnsafe($text) {

Why have both of these?

alexpott’s picture

Title: Remove Safemarkup::escape() and rely more on twig autoescaping » [PP-2] Remove Safemarkup::escape() and rely more on twig autoescaping
Status: Needs review » Postponed

re #10 yeah we might not need it by the time we get to this patch due to changes by the SafeMarkup::set() removal patches. Since this is the last patch in the chain I'll postpone in on #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping.

I basically wrote all these patches first to prove that it was actually possible to do. Now that that is proved I'll adjust the issue statuses accordingly.

Wim Leers’s picture

Cool :)

joelpittet’s picture

Issue summary: View changes
alexpott’s picture

Wim Leers’s picture

Title: [PP-2] Remove Safemarkup::escape() and rely more on twig autoescaping » [PP-1] Remove Safemarkup::escape() and rely more on twig autoescaping
xjm’s picture

Priority: Major » Critical
Issue tags: +Triaged D8 critical

Discussed with @catch, @alexpott, @webchick, and @effulgentsia. This issue is critical because of the unreliability, unpredictability, and confusion for using SafeMarkup::escape().

plach’s picture

What's the relation of this issue with #2555473: Deprecate SafeMarkup::checkPlain() for Drupal 8.0.x? Do they need to be both critical? I'm asking mainly because #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping is major (sorry if this has already been answered).

alexpott’s picture

Title: [PP-1] Remove Safemarkup::escape() and rely more on twig autoescaping » Remove Safemarkup::escape() and rely more on twig autoescaping
Status: Postponed » Needs review
FileSize
9.39 KB

Actually since we have Html::escape() this is totally possible to do now. I've moved twig_drupal_join_filter() inside TwigExtension because this gives it access to TwigExtension::escapeFilter() which is basically the equivalent to SafeMarkup::escape() - it also means safe_join supports everything the twig does which is a nice side effect. Also we don't need to mark the output of safe_join filter as safe since Twig's filter system actually does that for us.

alexpott’s picture

@plach SafeMarkup::escape()'s removal is critical because the method is very confusing it only escapes if the string is not safe.

Status: Needs review » Needs work

The last submitted patch, 18: 2549393-2.17.patch, failed testing.

star-szr’s picture

Title: Remove Safemarkup::escape() and rely more on twig autoescaping » Remove SafeMarkup::escape() and rely more on Twig autoescaping
dawehner’s picture

Status: Needs work » Needs review
FileSize
12.31 KB
3.79 KB
+++ b/core/themes/engines/twig/twig.engine
@@ -151,29 +151,3 @@ function twig_without($element) {
-    $output .= $separator . SafeMarkup::escape($item);

OH wow, this never made sense

Just some minor tweaks: small doc fix, add typehint, fix tests by using @title instead of !title, and a unit test

dawehner’s picture

Issue summary: View changes
alexpott’s picture

Issue tags: +beta target

The plan is to get this in before the next beta so that we can announce all the changes to the SafeMarkup API at the same time.

alexpott’s picture

+++ b/core/modules/content_translation/src/ContentTranslationHandler.php
@@ -290,8 +290,8 @@ public function entityFormAlter(array &$form, FormStateInterface $form_state, En
-        $t_args = array('%language' => $languages[$form_langcode]->getName(), '%title' => $entity->label(), '!title' => $title);
-        $title = empty($source_langcode) ? t('!title [%language translation]', $t_args) : t('Create %language translation of %title', $t_args);
+        $t_args = array('%language' => $languages[$form_langcode]->getName(), '%title' => $entity->label(), '@title' => $title);
+        $title = empty($source_langcode) ? t('@title [%language translation]', $t_args) : t('Create %language translation of %title', $t_args);

It is funny how many different patches have addressed this hilarity. The %title marks the label as safe - making the resulting string safe even though it using !title. Will be great to get this fixed.

alexpott’s picture

Marked #2506035: Do not add placeholders from SafeMarkup::format() to the safe list as a duplicate and ticked @xjm in the credit box. Hopefully @pwolanin will comment on this issue so he can get credit too.

alexpott’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
alexpott’s picture

When this lands we will need to update [#2549395]

dawehner’s picture

Some quick expanding of the test coverage

andypost’s picture

2 nits

  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -142,7 +142,7 @@ public function getFilters() {
    +      new \Twig_SimpleFilter('safe_join', [$this, 'safeJoin'], array('needs_environment' => true, 'is_safe' => array('html'))),
    

    please do not mix 2 array syntax

  2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -514,4 +514,30 @@ public function renderVar($arg) {
    +  public function safeJoin(\Twig_Environment $env, $value, $glue = '') {
    +    $separator = '';
    ...
    +      $output .= $separator . $this->escapeFilter($env, $item, 'html', NULL, TRUE);
    +      $separator = $glue;
    

    I see no need in $separator

lauriii’s picture

#30.2 it seems to be there because we don't want to add the separator for the first item so its an empty variable set a little earlier.

andypost’s picture

FileSize
1.37 KB
13.26 KB

1. For such long strings better to use short-syntax as few above lines doing
2. since PHP 5.4 $this could be used in anonymous functions, and array mapping more readable

dawehner’s picture

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -530,14 +530,10 @@ public function renderVar($arg) {
-    $separator = '';
-    $output = '';
-    foreach ($value as $item) {
+    return implode($glue, array_map(function($item) use ($env) {
       // If $item is not marked safe then it will be escaped.
-      $output .= $separator . $this->escapeFilter($env, $item, 'html', NULL, TRUE);
-      $separator = $glue;
-    }
-    return $output;
+      return $this->escapeFilter($env, $item, 'html', NULL, TRUE);
+    }, $value));

Nice refactoring!

dawehner’s picture

Some first idea of the change to the CR:

diff --git a/foo.txt b/foo.txt
index 516faa1..867a13a 100644
--- a/foo.txt
+++ b/foo.txt
@@ -4,6 +4,7 @@
   <li>SafeMarkup::xssFilter()</li>
   <li>SafeMarkup::placeholder() - there is no direct replacement for this. Use \Drupal\Component\Utility\SafeMarkup::format() with $args = [&#039;%placeholder&#039; =&gt; &#039;thing to be placeholdered&#039;]</li>
   <li>SafeMarkup::set() - use one of the strategies in https://www.drupal.org/node/2311123</li>
+  <li>SafeMarkup::escape() - Pretty much the same as SafeMarkup::checkPlain() applies here. Rely on twig autoescape, use #plain_text or use Html::escape() if you require explicit filtering.</li>
 </ul>
 <h3>How to filter markup</h3>
 The SafeMarkup::xssFilter() method is removed in favour of filtering using the render system. By default all #markup is filtered using the admin tag list. However the list of allowed tags can be changed using #allowed_tags. 
When this lands we will need to update [#2549395]

So should we update the CR first or the other way round? This comment + tag kinda leads to a circular dependency thing :)

stefan.r’s picture

Patch looks good!

+++ b/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php
@@ -124,6 +127,31 @@ public function testSafeStringEscaping() {
+    SafeMarkup::setMultiple([$string => ['html' => TRUE]]);

Heh :)

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -514,4 +514,26 @@ public function renderVar($arg) {
+   * Joins several strings together safely.
...
+   *   The pieces to join.
...
+   *   The delimiter with which to join the string. Defaults to an empty string.
...
+   * @return string
+   *   The imploded string.

First we talk about joining, then imploding, so maybe "The joined string"?

alexpott’s picture

Addressing #35.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record updates

I added the suggested change for CR from @dawehner to the actual CR. The change itself looks good for me.

+++ b/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php
@@ -124,6 +127,31 @@ public function testSafeStringEscaping() {
+
+

Extra line change can be changed while commiting

stefan.r’s picture

Thanks @laurii

I made a small update to the CR, maybe someone can check whether the bit about SafeMarkup::escape() still looks sane.

alexpott’s picture

@stefan.r I was going to add the stuff about SafeMarkup::isSafe() to the CR - nice work.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 82db483 on 8.0.x
    Issue #2549393 by alexpott, dawehner, lauriii, andypost, xjm: Remove...

Status: Fixed » Closed (fixed)

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