Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
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
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. |
Comment | File | Size | Author |
---|---|---|---|
#19 | 2506133.19.patch | 7.17 KB | alexpott |
#19 | 11-19-interdiff.txt | 3.89 KB | alexpott |
#11 | 2506133.11.patch | 3.7 KB | alexpott |
#11 | 5-11-interdiff.txt | 1.6 KB | alexpott |
#5 | 2506133.5.patch | 3.24 KB | alexpott |
Comments
Comment #1
alexpottWith 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.
Comment #2
alexpottComment #4
alexpottThe overall diff summary for those two runs.
Comment #5
alexpottA spurious file got into the patch :(
Comment #6
Fabianx CreditAttribution: Fabianx as a volunteer commentedLooks great to me!
Comment #7
dawehnerA 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
Comment #8
alexpottI was trying to save the string cast in the case of an object because that'll happen in PHP user land time
Comment #9
dawehnerAh got it. Maybe quickly document it ...
Comment #10
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedDo 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() ?
typo: object's
Comment #11
alexpottThanks for the reviews @dawehner and @pwolanin. Patch addresses all your points.
Comment #12
Fabianx CreditAttribution: Fabianx as a volunteer commentedRTBC - Looks great to me - unless we need new test coverage?
Comment #13
joelpittetAny reason for not extending \Twig_Markup? Save for maybe missmatched constructors?
Comment #14
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented@joelpittet - the constructor isn't part of the interface, so that shouldn't matter
Comment #15
xjmBoth these classes have unit tests, so let's add to them to cover the new code path with the use of the new interface?
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.
Comment #16
joelpittetOk my question still stands why create a new interface over extending Twig_Markup? @alexpott how did you come to that choice from the IS?
Comment #17
twistor CreditAttribution: twistor commented@joelpittet, I believe the reason is to keep SafeMarkup decoupled from Twig.
@alexpott, why doesn't SafeStringInterface actually require the __toString() method?
Comment #18
joelpittetSounds like a decent reason:)
Comment #19
alexpott@joelpittet also
Twig_Markup::count()
onAttribute
would have be very very on since theAttribute
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.
Comment #20
alexpottUpdated summary and added beta evaluation.
Comment #21
alexpottComment #22
alexpottComment #25
alexpott#23 was due to testbot failure - see #2500067: Repeated HEAD failures on multiple bots with apparent 60 min timeout
Comment #27
alexpottDrupal\system\Tests\Module\InstallUninstallTest failed in #26 pressing retest.
Comment #30
star-szrDrupal\simpletest\Tests\SimpleTestBrowserTest:
"0 fails, 0 exceptions" found
Comment #32
Fabianx CreditAttribution: Fabianx as a volunteer commentedRTBC, looks great
Comment #33
xjmThanks for the docs and tests.
Good choice to mark this internal.
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:
Comment #36
joelpittetThis issue may have overlooked a fairly common use case, which now gets double escaped.
Translated attribute values.
Example in core: feed_icon
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?
Comment #37
joelpittetOpened up a follow-up: #2531824: Attribute class to check safe strings before escaping (has tests)
Comment #38
joelpittetAnd 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