Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The |passthrough
filter was introduced in an effort to identify and "tag" when to use !
with the {% trans %}
tag.
Proposed resolution
- Remove the
|passthrough
filter and use|raw
.
Remaining tasks
TBD
User interface changes
None
API changes
None
Comment | File | Size | Author |
---|---|---|---|
#6 | interdiff-3-6.txt | 1.9 KB | tadityar |
#6 | passthrough-raw-2282101-6.patch | 6.31 KB | tadityar |
#3 | passthrough-raw-2282101-3.patch | 6.34 KB | tadityar |
Comments
Comment #1
joelpittetComment #2
tadityar CreditAttribution: tadityar commentedSo do I have to create a new filter named 'raw' or do I just remove passthrough filter and change occurences of
|passthrough
in twig files with|raw
?Comment #3
tadityar CreditAttribution: tadityar commentedComment #5
joelpittetThanks for tackling this @tadityar! Here some notes that may help on your patch in #3
Nice!
This case may need to stay but changed to raw as this trans function maps to the t function.
This test should stay, though also be |raw.
Comment #6
tadityar CreditAttribution: tadityar commentedChanged the patch based on #5!
Comment #8
joelpittetHmm, I wonder what's up here... it would be interesting to see the compiled php from this and what it's producing. I think the |raw escape filter is evaluated before the translation filter, just a guess.
Thanks again for working on this. Any guesses?
Comment #9
tadityar CreditAttribution: tadityar commented@joelpittet if so then how do we change the order?
No guesses here.
Comment #10
joelpittetThere is a priority value on the Twig extension, it would likely need to go before... but this is just a guess and probably not a good way to go about this... Needs to be played with or stepped through to find out whats going on for sure.
Comment #11
davidhernandezHow can I manually test this?
Comment #12
effulgentsia CreditAttribution: effulgentsia commentedAs of #2435493: Change String::format()'s '@' and '%' placeholders to be auto-escaped rather than always-escaped, we no longer have any core non-test use cases for the
|passthrough
filter. Do we want to use this issue to remove that filter entirely? Or do we want to leave that filter in to support a hypothetical contrib/custom use-case where the filter would be needed? If we want to keep it, +1 to renaming it to "raw", but what would such a use case be and is there a better way of supporting it?Comment #13
effulgentsia CreditAttribution: effulgentsia commentedComment #14
markhalliwellHmm, that's a good question. I'm not entirely sure we need either now TBH, unless we intend to keep parity with the
!
prefix used in String::format().The only reason I introduced the
|passthrough
"filter" in #1927584: Add support for the Twig {% trans %} tag extension was because the native internal|raw
filter isn't actually defined as a "filter" and isn't detectable in the code (or at least it wasn't at the time). I opened this issue to eventually address this one off "hack" until something better came along, which I think #2435493: Change String::format()'s '@' and '%' placeholders to be auto-escaped rather than always-escaped addresses quite well.I think we should just go ahead and kill the
|passthrough
filter entirely and stop worrying about trying to support!
prefixed variables or trying to detect the native|raw
filter in{% trans %}
tags.Comment #15
Fabianx CreditAttribution: Fabianx commentedThe |raw filter will never be safe, unless you rename it in a first 'node visitor pass' to passthrough within those elements.
Twig has internal optimizations to remove |raw, which I don't think the trans code is capable of:
e.g.
{{ foo }} gets translated to twig_print_var(drupal_escape_filter('foo'))
while
{{ foo|raw }} gets translated to twig_print_var('foo')
This is how it works :).
On the other hand:
{% set x = foo|raw %}
IIRC should mark x as safe always and not be optimized out, so probably this is fine to remove and leave to that special case if you really need raw.
Comment #16
markhalliwellYes, trans has difficulty in detecting the internal
|raw
"filter", likely due to internal processes. It's not treated the same as other filters IIRC.I agree that a simple work around would be to set a new variable outside of the trans tag if it's really needed. That being said, I do not think core has any real case for this, especially after #2435493: Change String::format()'s '@' and '%' placeholders to be auto-escaped rather than always-escaped made it in.
We should just remove
|passthrough
filter entirely and update the change record accordingly.Comment #17
effulgentsia CreditAttribution: effulgentsia at Acquia commentedWhat if we went further and removed
|raw
as well? Core's last usage of it is in #2500747: Remove 'html' option from theme('time') with a patch there to fix it. Removing the filter entirely would nudge contrib to better practices (i.e., let Drupal's autoescaping/SafeMarkup implementations do their job, don't try to circumvent them). And if there's a legitimate use case for it (which I'm skeptical about), there can be a contrib module that adds it.Comment #18
joelpittet@effulgentsia I'd really rather not remove
|raw
. There will be way to many support questions on why Twig's auto-escape is on but|raw
doesn't work.I agree we shouldn't encourage it's use but I'd really not want to address the consequences of removing this feature of upstream Twig. I'll be sure to include that in the document I'm writing up for tomorrow for the D8 Theme Guide on security.
Comment #19
Gábor Hojtsy#2495179: Twig placeholder filter should not map to raw filter is related and currently critical.
Comment #20
effulgentsia CreditAttribution: effulgentsia at Acquia commented#18 is reasonable. Restoring issue title and scope to #16.
Comment #21
stefan.r CreditAttribution: stefan.r commented#2558791: "!"-prefixed tokens should Xss::filterAdmin() but not affect safeness repurposes it, i.e. admin XSS filtering if unsafe and printing as-is if safe.
Comment #22
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIs there a use-case for a Twig author to ever make the decision of whether an unsafe variable should be filtered vs. escaped? Seems to me that if a variable should be filtered, then a preprocess function or similar should do it, not Twig. Therefore, unless I'm wrong about that, I'd rather we remove the filter rather than requiring core to support poor practices.
Comment #23
joelpittetre #22 I'd prefer getting raw values (un-pre-escaped) so I have the flexibility to deal with them how they will be marked up in twig. Twig can deal with the escaping and we have control if we ever need it.
I know some don't share this opinion but having this control in twig even if it's not best practice is important and I'll likely find a decent usecase when in practice not unlike PHP filter is not best practice and security vector but in a pinch can be harmless and can save the day or even just allow for a quick prototype to prove a bit of code will work(then refactor), yet still a "possibly" dangerous if used with unsafe values(and other reasons). Feels like a bit of hand-holding that is one step too far in the name of security.
2¢
Comment #24
davidhernandezWhat he said ^. And we like to avoid preprocess as much as possible.
Comment #25
joelpittetJust found some new minor new information I should share regarding next release of twig. (we are on 1.20)
{% raw %}{% endraw %}
is depreciated in 1.21 and replaced with verbatimThis is not the
|raw
filter, which still exists, but just an FYI.The verbatim tag existed since 1.12
Comment #26
joelpittetApparently this was removed in #2571695: Remove !placeholder and unsafe string return from SafeMarkup::format()