Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Part of #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping
Problem/Motivation
SafeMarkup::checkPlain()
is unnecessary.
Proposed resolution
When escaping thing for render arrays we can use #plain_text => $something
or #markup => nl2br(Html::escape($something))
Remaining tasks
Do it
Review
Commit
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#30 | 2560641-2.30.patch | 57.02 KB | alexpott |
#30 | 25-30-interdiff.txt | 2.74 KB | alexpott |
#25 | 2560641-2.25.patch | 55.35 KB | alexpott |
#16 | interdiff.txt | 1.46 KB | lauriii |
#16 | remove_all_usages-2560641-16.patch | 53.45 KB | lauriii |
Comments
Comment #2
alexpottHere's a cut form #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping only involving render arrays.
Comment #3
alexpottFixing the tests.
Comment #5
XanoYour patch looks good. I found some additional usages of
SafeMarkup::checkPlain()
in render arrays. There are many more places where escaped strings eventually end up in render arrays, but those usages are in separate methods and it sounds like we want to convert those in different issues.Comment #7
alexpott@Xano #3 was quite carefully scoped... re-uploading #3
I'm going to separate this out into a separate change... we should deprecate this and remove the checkPlain() rather than remove - it is an unnecessary break.
Options are to be handled in another patch.
Let's leave this change for later.
Comment #8
alexpottCreated #2560851: Deprecate EntityListBuilder::getLabel() and remove usages of SafeMarkup::checkPlain()
Comment #9
lauriiiI guess that is patch from wrong issue..
Comment #10
alexpottOops... too many issues :)
Comment #11
alexpottComment #12
lauriiiI went through all of the SafeMarkup::checkPlain() calls and only one of the remaining ones is in a render array. I have removed that in the latest patch. Otherwise the patch is RTBC but can someone confirm that my change is fine?
Comment #13
alexpottI've made similar changes to @lauriii's several times in the parent issue. I think the changes are correct. The whole SafeMarkup::checkPlain() effect on the title has gone because everything is simpler without it. We're testing that escaping occurs and that tags are stripped in the title in the head section. All looks good to me.
Comment #14
lauriiiThanks @alexpott for confirming that the changes are fine. With that feedback I'll RTBC the whole.
Comment #15
catchDoesn't this change depend on the t() providing a TranslationWrapper issue? It'll work without it, but looks a bit early here. Also this isn't a SafeMarkup::checkPlain() call anyway. Probably just needs dropping from the patch.
Not introduced here but Html::escape() encodes rather than removes. We could probably just do s/removed/encoded/ or not do it here too.
Comment #16
lauriiiFixed points from #15
Comment #18
alexpottLooks like @catch was correct. #16 will be green and the patch to go for...
Comment #19
lauriiiHiding patches
Comment #20
alexpottSince #15 where minor nits setting back to rtbc.
Comment #21
tim.plunkettNot clear why this is being removed. #configuration is used in template_preprocess_block()
Comment #22
alexpott@tim.plunkett - because it is already set correctly...
Comment #23
ianthomas_ukWith #16 applied,
<title>
s for nodes are always double escaped. Before the patch, they are only escaped when rendering from cache (see #2531430: Page's <title> double escaped)Comment #24
alexpottWhat @ianthomas_uk (nice spot btw) is referring to is the title tag on the page. Actually before the patch title tags are escaped correctly ie:
and after they are double escaped
Also note that if you use chrome's or firefox's inspector then it does the a single HTML entity decode for you. In order to see what is really going on you have to view the actual page source.
Comment #25
alexpottFixed it and included the test from #2531430: Page's <title> double escaped - I think we should close that one as a dupe and give credit to everyone who worked on it on this issue too.
Comment #26
lauriiiRTBC if this passes
Comment #27
borisson_Commenting on this issue for credit. (#2531430: Page's <title> double escaped led me here).
Comment #28
alexpottSaving credit. @olli should be added to the commit message if he has not commented by the time this is committed.
Comment #30
alexpottSo testing a page's title escaping and markup using
$this->assertTitle()
is a mess because it is use xpath and xpath completely messes with escaping by decoding html entities. Even using->asXml()
doesn't work because quotes are still decoded :(. We should be doing a regular expression on the raw content.And even the test added by #2531430: Page's <title> double escaped was not asserting what it thought it was as quote are escaped in the title (both in HEAD and in this patch).
We should file a followup to merge NodeTitleXssTest into NodeTitleTest - having two test like this is unnecessary.
I would not be surprised if the changes to assertTitle() cause more test fails but I think we should fix them all here... or move the assertTitle() change into its own issue that blocks this one if there are large amounts of fails.
Comment #31
lauriii#2562359: Merge NodeTitleTest and NodeTitleXSSTest filed the follow-up for the NodeTitleXSSTest and NodeTitleTest. I'm also happy for the current patch.
Comment #33
catchCommitted/pushed to 8.0.x, thanks!