Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#36 | 2549393-3.36.patch | 13.27 KB | alexpott |
#36 | 32-36-interdiff.txt | 583 bytes | alexpott |
#32 | remove-2549393-32.patch | 13.26 KB | andypost |
#32 | interdiff.txt | 1.37 KB | andypost |
#31 | remove-2549393-31.patch | 13.34 KB | lauriii |
Comments
Comment #2
alexpottComment #3
alexpottPatch 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.
Comment #5
alexpottFixing the tests. Wouldn't it be nice to have a non safe markup version of SafeMarkup::format.
Comment #6
alexpottOh 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.
Comment #8
alexpottComment #9
olli CreditAttribution: olli commentedHow about "Encodes special characters to HTML entities."? Maybe rename to Html::escape() or ::checkPlain()?
Comment #10
Wim LeersWhy have both of these?
Comment #11
alexpottre #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.
Comment #12
Wim LeersCool :)
Comment #13
joelpittetComment #14
alexpottComment #15
Wim Leers#2549791: Remove SafeMarkup::xssFilter() and provide ability to define #markup escaping strategy and what tags to filter landed.
Comment #16
xjmDiscussed with @catch, @alexpott, @webchick, and @effulgentsia. This issue is critical because of the unreliability, unpredictability, and confusion for using
SafeMarkup::escape()
.Comment #17
plachWhat'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).
Comment #18
alexpottActually since we have
Html::escape()
this is totally possible to do now. I've movedtwig_drupal_join_filter()
insideTwigExtension
because this gives it access toTwigExtension::escapeFilter()
which is basically the equivalent to SafeMarkup::escape() - it also meanssafe_join
supports everything the twig does which is a nice side effect. Also we don't need to mark the output ofsafe_join
filter as safe since Twig's filter system actually does that for us.Comment #19
alexpott@plach
SafeMarkup::escape()
's removal is critical because the method is very confusing it only escapes if the string is not safe.Comment #21
star-szrComment #22
dawehnerOH 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
Comment #23
dawehnerComment #24
alexpottThe 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.
Comment #25
alexpottIt 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.
Comment #26
alexpottMarked #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.
Comment #27
alexpottComment #28
alexpottWhen this lands we will need to update [#2549395]
Comment #29
dawehnerSome quick expanding of the test coverage
Comment #30
andypost2 nits
please do not mix 2 array syntax
I see no need in $separator
Comment #31
lauriii#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.
Comment #32
andypost1. 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 readableComment #33
dawehnerNice refactoring!
Comment #34
dawehnerSome first idea of the change to the CR:
So should we update the CR first or the other way round? This comment + tag kinda leads to a circular dependency thing :)
Comment #35
stefan.r CreditAttribution: stefan.r commentedPatch looks good!
Heh :)
First we talk about joining, then imploding, so maybe "The joined string"?
Comment #36
alexpottAddressing #35.
Comment #37
lauriiiI added the suggested change for CR from @dawehner to the actual CR. The change itself looks good for me.
Extra line change can be changed while commiting
Comment #38
stefan.r CreditAttribution: stefan.r commentedThanks @laurii
I made a small update to the CR, maybe someone can check whether the bit about SafeMarkup::escape() still looks sane.
Comment #39
alexpott@stefan.r I was going to add the stuff about SafeMarkup::isSafe() to the CR - nice work.
Comment #40
catchCommitted/pushed to 8.0.x, thanks!