Problem/Motivation

The only reason we call SafeMarkup::set() in Drupal\Core\Template\Attribute is to mark a safe as safe when it is printed directly in a TwigTemplate we can do this in a better way that does not need to mark the string as safe!

Proposed resolution

Add SafeStringInterface and assume that objects that implement it provide safe markup from their __toString() method.

Remaining tasks

Review & commit

User interface changes

None

API changes

Add SafeStringInterface

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because SafeMarkup::set() works
Issue priority Major because SafeMarkup::set() uses memory that can never be garbage collected. Making Attribute use SafeStringInterface means we save lots of unnecessary calls to SafeMarkup::set().
Prioritized changes The main goal of this issue is performance since memory footprint of an application is extremely important for running concurrent requests on the same server.
Disruption Not disruptive at all.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

Status: Active » Needs review
FileSize
3.39 KB

With patch the permissions page with 11 roles calls SafeMarkup::set() 7623 times and has a peak memory usage of 62M
Without patch the permissions page with 11 roles calls SafeMarkup::set() 12,346 times and has a peak memory usage of 64M

Methinks this rocks.

alexpott’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 1: 2506133.1.patch, failed testing.

alexpott’s picture

The overall diff summary for those two runs.

Run #557e9ddc2499a Run #557e9d7c08bf2 Diff Diff%
Number of Function Calls 762,886 739,833 -23,053 -3.0%
Incl. Wall Time (microsec) 2,015,500 1,948,722 -66,778 -3.3%
Incl. MemUse (bytes) 51,671,720 49,737,704 -1,934,016 -3.7%
Incl. PeakMemUse (bytes) 63,832,920 61,899,456 -1,933,464 -3.0%
alexpott’s picture

Status: Needs work » Needs review
FileSize
3.24 KB

A spurious file got into the patch :(

Fabianx’s picture

Looks great to me!

dawehner’s picture

+++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
@@ -91,7 +91,7 @@ public static function set($string, $strategy = 'html') {
   public static function isSafe($string, $strategy = 'html') {
-    return isset(static::$safeStrings[(string) $string][$strategy]) ||
+    return $string instanceOf SafeStringInterface || isset(static::$safeStrings[(string) $string][$strategy]) ||
       isset(static::$safeStrings[(string) $string]['all']);
   }

A quick question: which of these 3 cases will be most likely to be the case? It seems to be that swapping the first and second bit of the || would be something we could do, not sure though how much this would actually help

alexpott’s picture

I was trying to save the string cast in the case of an object because that'll happen in PHP user land time

dawehner’s picture

Ah got it. Maybe quickly document it ...

pwolanin’s picture

  1. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
    @@ -91,7 +91,7 @@ public static function set($string, $strategy = 'html') {
       public static function isSafe($string, $strategy = 'html') {
    

    Do we need to document that the argument here and other places in this class can be a string OR an object that responds to __toString() ?

  2. +++ b/core/lib/Drupal/Component/Utility/SafeStringInterface.php
    @@ -0,0 +1,14 @@
    + * Marks an objects __toString() method as returning safe markup.
    

    typo: object's

alexpott’s picture

Thanks for the reviews @dawehner and @pwolanin. Patch addresses all your points.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC - Looks great to me - unless we need new test coverage?

joelpittet’s picture

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -397,7 +398,7 @@ public function escapeFilter(\Twig_Environment $env, $arg, $strategy = 'html', $
-    if ($autoescape && $arg instanceOf \Twig_Markup) {
+    if ($autoescape && ($arg instanceOf \Twig_Markup || $arg instanceOf SafeStringInterface)) {

Any reason for not extending \Twig_Markup? Save for maybe missmatched constructors?

pwolanin’s picture

@joelpittet - the constructor isn't part of the interface, so that shouldn't matter

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests
  1. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
    @@ -91,7 +91,9 @@ public static function set($string, $strategy = 'html') {
       public static function isSafe($string, $strategy = 'html') {
    -    return isset(static::$safeStrings[(string) $string][$strategy]) ||
    +    // Do the instance of checks first to save unnecessarily casting the object
    +    // to a string.
    +    return $string instanceOf SafeStringInterface || isset(static::$safeStrings[(string) $string][$strategy]) ||
    
    +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -397,7 +398,7 @@ public function escapeFilter(\Twig_Environment $env, $arg, $strategy = 'html', $
    -    if ($autoescape && $arg instanceOf \Twig_Markup) {
    +    if ($autoescape && ($arg instanceOf \Twig_Markup || $arg instanceOf SafeStringInterface)) {
    

    Both these classes have unit tests, so let's add to them to cover the new code path with the use of the new interface?

  2. +++ b/core/lib/Drupal/Component/Utility/SafeStringInterface.php
    @@ -0,0 +1,14 @@
    +/**
    + * Marks an object's __toString() method as returning safe markup.
    + */
    +interface SafeStringInterface {
    +}
    

    I think this needs an @see to SafeMarkup::set() (and vice versa), with more docs explaining the relationship, and reference to the same stern warnings about security.

joelpittet’s picture

Ok my question still stands why create a new interface over extending Twig_Markup? @alexpott how did you come to that choice from the IS?

twistor’s picture

@joelpittet, I believe the reason is to keep SafeMarkup decoupled from Twig.

@alexpott, why doesn't SafeStringInterface actually require the __toString() method?

joelpittet’s picture

Sounds like a decent reason:)

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.89 KB
7.17 KB

@joelpittet also Twig_Markup::count() on Attribute would have be very very on since the Attribute object implements \IteratorAggregate

I've marked SafeStringInterface as @internal since I don't think contrib or custom have any use case.

Patch adds tests and documentation requested by @xjm in #15. Thanks for the review.

alexpott’s picture

Issue summary: View changes

Updated summary and added beta evaluation.

alexpott’s picture

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

Status: Needs review » Needs work

The last submitted patch, 19: 2506133.19.patch, failed testing.

Status: Needs work » Needs review

alexpott queued 19: 2506133.19.patch for re-testing.

alexpott’s picture

Status: Needs review » Needs work

The last submitted patch, 19: 2506133.19.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

Drupal\system\Tests\Module\InstallUninstallTest failed in #26 pressing retest.

alexpott queued 19: 2506133.19.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 19: 2506133.19.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review

Drupal\simpletest\Tests\SimpleTestBrowserTest: "0 fails, 0 exceptions" found

Cottser queued 19: 2506133.19.patch for re-testing.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, looks great

xjm’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for the docs and tests.

  1. +++ b/core/lib/Drupal/Component/Utility/SafeStringInterface.php
    @@ -0,0 +1,36 @@
    + * @internal
    

    Good choice to mark this internal.

  2. +++ b/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php
    @@ -99,4 +99,42 @@ public function testActiveTheme() {
    +    // Ensure objects that implement SafeStringInterface are unchanged.
    +    $safe_string = $this->getMock('\Drupal\Component\Utility\SafeStringInterface');
    +    $this->assertSame($safe_string, $twig_extension->escapeFilter($twig, $safe_string, 'html', 'UTF-8', TRUE));
    

    I didn't quite understand what this was testing at first, so I pinged alexpott about it. The idea is that any other objects are cast to a string, and only SafeStringInterface objects are returned early as-is. I've added a sentence to the comment on commit to clarify this (see below) after running it by alexpott.

As a performance/memory improvement, this issue is a prioritized change as per https://www.drupal.org/core/beta-changes and it does not introduce any disruption. Committed and pushed to 8.0.x.

Added the following grammar fixes plus clarifying comment on commit:

diff --git a/core/lib/Drupal/Component/Utility/SafeMarkup.php b/core/lib/Drupal/Component/Utility/SafeMarkup.php
index db7557e..d97dc2f 100644
--- a/core/lib/Drupal/Component/Utility/SafeMarkup.php
+++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
@@ -91,7 +91,7 @@ public static function set($string, $strategy = 'html') {
    *   TRUE if the string has been marked secure, FALSE otherwise.
    */
   public static function isSafe($string, $strategy = 'html') {
-    // Do the instance of checks first to save unnecessarily casting the object
+    // Do the instanceof checks first to save unnecessarily casting the object
     // to a string.
     return $string instanceOf SafeStringInterface || isset(static::$safeStrings[(string) $string][$strategy]) ||
       isset(static::$safeStrings[(string) $string]['all']);
diff --git a/core/lib/Drupal/Component/Utility/SafeStringInterface.php b/core/lib/Drupal/Component/Utility/SafeStringInterface.php
index 91d3537..3729027 100644
--- a/core/lib/Drupal/Component/Utility/SafeStringInterface.php
+++ b/core/lib/Drupal/Component/Utility/SafeStringInterface.php
@@ -12,7 +12,7 @@
  *
  * This interface should only be used on objects that emit known safe strings
  * from their __toString() method. If there is any risk of the method returning
- * user entered data that has not been filtered first, it must not be used.
+ * user-entered data that has not been filtered first, it must not be used.
  *
  * @internal
  *   This interface is marked as internal because it should only be used by
diff --git a/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php b/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php
index 9a0e4e8..e8718d5 100644
--- a/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php
+++ b/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php
@@ -114,6 +114,7 @@ public function testSafeStringEscaping() {
     ));
     $twig_extension = new TwigExtension($renderer);
 
+    // By default, TwigExtension will attempt to cast objects to strings.
     // Ensure objects that implement SafeStringInterface are unchanged.
     $safe_string = $this->getMock('\Drupal\Component\Utility\SafeStringInterface');
     $this->assertSame($safe_string, $twig_extension->escapeFilter($twig, $safe_string, 'html', 'UTF-8', TRUE));

  • xjm committed f6d785e on 8.0.x
    Issue #2506133 by alexpott, joelpittet, dawehner, pwolanin: Replace...

Status: Fixed » Closed (fixed)

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

joelpittet’s picture

This issue may have overlooked a fairly common use case, which now gets double escaped.

Translated attribute values.

Example in core: feed_icon

$variables['attributes']['title'] = t('Message: @escaped', ['@escaped' => '<>']);

Will escape it once in t, if not marked safe, once in attributes, then once more in twig because the string changed in attributes.

If we mark it safe it still gets escaped at least twice...

Maybe we need to avoid htmlspecialchars escaping in attribute if it's marked safe?

joelpittet’s picture

joelpittet’s picture

And found this out when trying to see how far we can get in this issue: #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests