Problem/Motivation
If you have some safe strings and you want to safe_join them in a template and pass that as a @placeholder for translation, they get escaped even though they are safe. safe_join actually breaks the Markup object wrapper.
Why this should be an RC target
Without this patch any Markup instance passed to the |safe_join()
Twig filter will be unintentionally escaped.
Disruptions: None, the intended result of this is to concatenate strings safely and return them joined by a separator and each item escaped if an item is not already a Markup instance.
Proposed resolution
Make the safe_join filter return a Markup object.
Remaining tasks
PatchTestsReview
User interface changes
Less things getting accidentally escaped.
API changes
safe_join will return a TwigMarkup/Markup object rather than a string.
Data model changes
None
Why this should be an RC target
Expectations of how safe_join works with MarkupInterface will be way off and this is not disruptive.
Comment | File | Size | Author |
---|---|---|---|
#88 | drupal-2579091-88.patch | 7.06 KB | prudloff |
#76 | 2579091-76.patch | 8.19 KB | jofitz |
#76 | interdiff-2579091-74-76.txt | 1.44 KB | jofitz |
#74 | 2579091-74.patch | 8.14 KB | jofitz |
#74 | interdiff-2579091-71-74.txt | 1 KB | jofitz |
Comments
Comment #2
star-szrComment #3
joelpittetThis seems like the appropriate take on this. RTBC after tests in place:)
Comment #4
star-szrComment #5
dawehnerGiven the domain I'm curious whether \Twig_Markup would be more correct to use here.
Comment #6
star-szrThat would be great but \Twig_Markup doesn't implement MarkupInterface so it doesn't really solve the problem I don't think.
Edit: Unless we allow \Twig_Markup wherever we allow MarkupInterface…
Comment #7
dawehnerSure it doesn't, but you also doesn't need it, see
core/lib/Drupal/Core/Template/TwigExtension.php:407
Comment #8
star-szrThat's not the problem this issue is trying to solve. It's trying to solve the interaction between Twig and SafeMarkup which ultimately leads to \Drupal\Component\Utility\SafeMarkup::isSafe():
So to go with \Twig_Markup we'd need to update that as well and maybe other places. Then we are adding Twig stuff to Utility which I'm guessing is not OK.
Comment #9
dawehnerMH, well I don't get that. Where do we use the result of the twig level back outside of the level of twig?
Maybe it simply helps to write a test, but its not obvious for me why we need this. Do we use safe_join internally somehow?
Comment #10
star-szrWe want to use safe_join and be able to use it as a @placeholder. There is a failing patch at #2151109-52: Convert theme_system_modules_details() to Twig which kinda demonstrates the problem.
t() (t filter or trans tag) is one way that Twig things make their way outside of Twig.
Comment #11
lauriiiWriting tests for this
Comment #12
lauriiiComment #13
dawehnerHuch, where do we use SafeJoin in that test?
Comment #14
lauriiiCopying code starting from TwigExtension line 175:
Comment #16
star-szrI think it's just single escaping here, not double escaping.
Thanks for working on the tests @lauriii!
Comment #17
dawehnerWell, this almost sounds for me like TwigNodeTrans should use the same escaping strategy as normal twig and not use the one from t()? Well its tricky but doing things the wrong way, just because it works, potentially bites you on the longrun.
Comment #18
star-szr@dawehner the t() filter does not go through TwigNodeTrans as far as I know so an approach like that would only get us so far. Other suggestions for solving this are welcome but right now it's a big WTF that safe_join actually loses safeness. Would adding our own TwigMarkup (or whatever you want to call it) that implements MarkupInterface make things any better?
Comment #19
lauriiiThis is blocking one of the last theme function conversions
Comment #20
joelpittetTests in place and more kittens. @dawehner feel free to un-RTBC if you have a better solution but this one gets us to unblock some changes where we can use safe_join filter in the template more reliably.
Comment #22
dawehnerYou know, I would try out something like this.
Changed return value, let's document that.
Comment #24
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI'm not so sure about #22. Maybe a code or issue comment can clarify?
Personally, I think a
\Drupal\Core\Template\TwigMarkup
that extendsTwig_Markup
and implementsMarkupInterface
would make a lot of sense.Comment #26
joelpittetStill on board with #12. What is wrong with
Markup
and why does Twig need to be in the name?Comment #27
joelpittet@dawehner for that to work I thinkyou need to add:
'needs_environment' => TRUE
Comment #28
dawehnerWell, you know its the interaction between layers. The twig layer should not have to pass its own internal objects to other systems, kind of similar to views, which should not use
SafeString::create()
.Sounds like a great suggestion even ideally you should not have to. Doing that consequently might require changes in quite some inner parts of twig.
Comment #29
joelpittet@dawehner shouldn't have to change inner parts of Twig because all it's checks are for
instanceof Twig_Markup
. If you extend it should be fine.The explanation:
Sounds like a recipe for cruft, because we already have quite a few
SomethingStringInterface
's laying about and recently added quite a few more. It feels like we are going down a messy path.Comment #30
joelpittetWell let's see how this plays out... I removed the stuff to do with t() because it's a bit out of scope and t() already returns a
MarkupInterface
object.Comment #31
joelpittetMy unnecessary and wrong changes that parent class has already, removed.
Comment #34
joelpittetThis produces unexpected results in the templates without this fix.
Comment #35
dawehnerJust a quick driveby review.
Should be "\Drupal\Core\Template\TwigMarkup"
Comment #36
joelpittet@dawehner thanks, is that true if they are in the same namespace?
Comment #37
xjmThanks for tagging this for rc target triage! To have committers consider it for inclusion in RC, we should add a statement to the issue summary of why we need to make this change during RC, including what happens if we do not make the change and what any disruptions from it are. We can add a section
<h3>Why this should be an RC target</h3>
to the summary. "Unexpected results in the template" is a start, but kind of vague, and doesn't summarize the impact and disruption. :)Comment #38
joelpittetComment #39
joelpittetComment #40
joelpittetAdded RC target message to IS.
Comment #41
dawehnerI'm wondering whether we should have a test twig example as well?
Comment #42
xjmComment #44
dawehnerI disagree with myself. This is not a bug of the integration between systems, but rather a problem in itself, so this means a unit test is the appropriate tool for the job.
Comment #45
lauriiiRerolled & made this follow our BC policy about markup changes
Comment #48
DuaelFrI just faced this issue while genuinely using the Zen theme that tries to use Components to build the page.
In the Drupal's html.html.twig file, it extends a component to render the
<head>
and pass it atitle
variable containing the safe_join'ed string we usually use. That string got escaped twice, reviving the good old'
issue if you have some quote in your title.Just to see what happens, I rerolled #45 against 8.3.x.
Comment #50
DuaelFrIt seems you were a bit too quick deleting some parts of that preprocess ;)
I also fixed the template in the stable theme.
Comment #52
DuaelFrIt seems that
stable_preprocess_system_modules_details()
does nothing more thantemplate_preprocess_system_modules_details()
now but breaking the tests by pre-rendering the required/missing modules.The
\Drupal\system\Tests\Module\DependencyTest
test passes locally so let's see what the testbot think about the entire coverage.Comment #53
GoZ CreditAttribution: GoZ at Centarro commentedClass files should not have @file doc block.
Everything else seems OK. Thanks DuaelFr
Comment #54
DuaelFr#53 DonePlease, ignore this patch.
Comment #55
GoZ CreditAttribution: GoZ at Centarro commentedInterdiff is wrong, and
stable_preprocess_system_modules_details()
is back in your patchComment #56
DuaelFrMy bad... I forgot to commit #52 on my local branch so I made my patch on #50. Stupid me.
That's fixed!
Comment #57
GoZ CreditAttribution: GoZ at Centarro commentedLast patch passed and reviewed. Thx Duaelfr
Comment #58
alexpottWe need a change record for this. Plus are we sure about:
This change. We're changing something from a proper html list to a comma separated list.
Comment #60
joelpittetThis looks like it just needs a CR. Rerolling.
Comment #61
DuaelFrAs @alexpott asked in #58 I'd like to ensure that we agree to replace these HTML lists to comma separated lists.
HTML lists are better for a11y because they allow to navigate from one item to the next easily. I'm not sure it's highly needed here but it's still better for people using a screen reader.
According to the answers here, I'll rewrite the patch to keep an HTML list in the modules dependencies, or not.
Comment #62
DuaelFrComment #63
DuaelFrMessed with tags, sorry :/
Comment #64
lauriiiI created change record for this. We still have to solve #58 & #61.
Comment #66
andypostChange of list to comma separated is fine except BC, otoh this early render was required to prevent double escaping at initial conversion to twig. So here should maintainers decide
I don't think that a lot of custom/contrib themes override this templates, other preprocessing is impossible here because the list is rendered so we should be safe
Removal of early render is good & inline with http://cgit.drupalcode.org/drupal/tree/core/themes/stable/templates/admi...
But it breaks BC for stable theme
Comment #67
xjmI think we should address #58. We have an inline comma list thing that styles semantic HTML lists (which can be created with render arrays) as comma-separated lists. That's almost always the right thing to use for such comma-separated lists.
I still think this feature should not be added because it actually encourages undesirable sanitization patches. My feedback from the original safe markup issue still applies WRT this feature.
Comment #70
andypostComment #71
zahord CreditAttribution: zahord as a volunteer and at Skilld commentedre-rolled patch in comment #60 using drupal 8.7.x
Comment #72
zahord CreditAttribution: zahord as a volunteer and at Skilld commentedComment #74
jofitz CreditAttribution: jofitz at jofitz commentedRe-rolled patch from #60.
Interdiff against #71 for reference.
Comment #76
jofitz CreditAttribution: jofitz at jofitz commentedFix deprecation notice and coding standards error.
Comment #83
GaëlGHere's a reroll for 9.4. Works on 9.3 too.
Comment #88
prudloff CreditAttribution: prudloff at Insite commentedHere is a reroll for Drupal 10.2.3.