Problem/Motivation

with custom theme template when using escape filter with other strategy than html, since drupal 10.3 and twig update it produce fatal error:

Call to undefined function Drupal\Core\Template\twig_escape_filter() dans Drupal\Core\Template\TwigExtension->escapeFilter() (ligne 464 de ***/core/lib/Drupal/Core/Template/TwigExtension.php).

it is related to changes in twig escape function visibility: twig/doc/filters/escape.rst
https://github.com/twigphp/Twig/blob/04b1ad3cedfb81f48b6a7ec8d77a2116c604c7d6/doc/filters/escape.rst

Steps to reproduce

update to drupal 10.3 whith composer, it update twig to 3.10.3
use escape filter with another strategy than html, for example: escape('js') or escape('html_attr')
The filter is wrapped to drupal implementation, where fallback use the twig function locally without context. As the implementation of this function change, it is not defined as this so it don't work

Proposed resolution

for now, try using $env->getRuntime(EscaperRuntime::class)->escape() instead of twig_escape_filter() on line 464 of /core/lib/Drupal/Core/Template/TwigExtension.php
as suggested in
https://github.com/twigphp/Twig/blob/3.x/doc/deprecated.rst

line 464 become
return $env->getRuntime(EscaperRuntime::class)->escape($return, $strategy, $charset, $autoescape);
instead of
return twig_escape_filter($env, $return, $strategy, $charset, $autoescape);
insert this line after line 8:
use Twig\Runtime\EscaperRuntime;

for future, as escape filter is overriden only for html strategy, use setEscaper() method instead of overriding filter.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#31 fix-twig-escape-filter-usage-3457168-31.patch729 bytesjulienjoye

Issue fork drupal-3457168

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

raphaelbertrand created an issue. See original summary.

raphaelbertrand’s picture

raphaelbertrand commented
Version: 9.3.x-dev » 10.3.x-dev
raphaelbertrand’s picture

raphaelbertrand commented
Issue summary: View changes
gauravvvv’s picture

gauravvvv commented
Version: 10.3.x-dev » 11.x-dev

Gauravvvv opened merge request !8546

gauravvvv’s picture

gauravvvv commented
Status: Active » Needs review
cilefen’s picture

cilefen commented

Do we know which commit in 10.3.x broke this?

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs tests

MR has failure and will need test coverage.

raphaelbertrand’s picture

raphaelbertrand commented

I tried locally by replacing line 464 with :
return (string) $env->getRuntime(EscaperRuntime::class)->escape($arg, $strategy, $charset, $autoescape);
instead of return $env->getRuntime(EscaperRuntime::class)->escape($env, $return, $strategy, $charset, $autoescape);
and insert use Twig\Runtime\EscaperRuntime; at the begining of file

raphaelbertrand changed the visibility of the branch 3457168-since-twigtwig-3.9 to hidden.

raphaelbertrand’s picture

raphaelbertrand commented

mistake, as the return type is string|null:
i propose this correction :
return $env->getRuntime(EscaperRuntime::class)->escape($arg, $strategy, $charset, $autoescape);
instead of return twig_escape_filter($env, $return, $strategy, $charset, $autoescape);
and insert use Twig\Runtime\EscaperRuntime; at the begining of file

for future, i think that as escape filter is overriden only for html strategy, it will be better to use setEscaper() method instead of overriding filter, but it need to use twig 3.10 minimum and it need more changes than this quick patch

raphaelbertrand’s picture

raphaelbertrand commented

@cilefen the change that broke this is internal at twig as the twig_escape_filter() is now declared as internal in twig, deprecated, and no more usable directly. As 10.3 use twig 3.9 or 3.10, it bring these changes into drupal.

raphaelbertrand changed the visibility of the branch 3457168-since-twigtwig-3.9 to active.

raphaelbertrand’s picture

raphaelbertrand commented
Issue summary: View changes
raphaelbertrand’s picture

raphaelbertrand commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

believe still will need test coverage

raphaelbertrand’s picture

raphaelbertrand commented

ok no problem. can someone do it ?

raphaelbertrand’s picture

raphaelbertrand commented

smustgrave want test coverage but still nobody to help to do it ?
i don't have time to do it actually and i am still with drupal 10.3.

rajab natshah’s picture

rajab natshah
he/him
commented

Tested MR8546 manually, Working, Thank you :)
This is a refactor change, current automated testing should pass.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Can we add a simple test that does this?

use escape filter with another strategy than html, for example: escape('js') or escape('html_attr')

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs work » Needs review
Issue tags: -Needs tests

To be honest I was just checking a related issue on how this class is covered and it seems pretty good. Also aren't we technically doing the same only through a different path?

I'd say we need no extra coverage here.

bbrala’s picture

bbrala
Location Netherlands
commented

That timing was perfect.

I've added the tests requested to test the fallback of the filter.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Ran test-only feature https://git.drupalcode.org/issue/drupal-3457168/-/jobs/2883906 which shows the coverage

Looking at the code change nothing stands out.

LGTM

  • catch committed e51c4c71 on 10.4.x 
    Issue #3457168 by raphaelbertrand, gauravvvv, bbrala, longwave: Since...

  • catch committed c0a194b9 on 11.0.x 
    Issue #3457168 by raphaelbertrand, gauravvvv, bbrala, longwave: Since...

  • catch committed 128310c8 on 11.x 
    Issue #3457168 by raphaelbertrand, gauravvvv, bbrala, longwave: Since...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

This looks good. Just a 1-1 swap. Committed/pushed to 11.x and cherry-picked back through to 10.3.x, thanks!

catch closed merge request !8546

  • catch committed eaa70724 on 10.3.x 
    Issue #3457168 by raphaelbertrand, gauravvvv, bbrala, longwave: Since...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

julienjoye’s picture

julienjoye commented
StatusFileSize
new fix-twig-escape-filter-usage-3457168-31.patch729 bytes

Hey.

It seems that this fix introduces fatal errors on my Drupal instance (with `10.3.6` update), on every places where I use an `html_attr` escape filter.
Here is the error I get:

The website encountered an unexpected error. Try again later.

Error: Object of class Drupal\Core\Url could not be converted to string in Twig\Template->display() (line 350 of /app/vendor/twig/twig/src/Template.php).
Twig\Template->render(Array) (Line: 35)
Twig\TemplateWrapper->render(Array) (Line: 33)

Solution:
Use 

return $env->getRuntime(EscaperRuntime::class)->escape($return, $strategy, $charset, $autoescape);

instead of

return $env->getRuntime(EscaperRuntime::class)->escape($arg, $strategy, $charset, $autoescape);

in `app/core/lib/Drupal/Core/Template/TwigExtension.php`.

raphaelbertrand changed the visibility of the branch 3457168-since-twigtwig-3.9 to active.

raphaelbertrand changed the visibility of the branch 3457168-since-twigtwig-3.9 to hidden.

raphaelbertrand’s picture

raphaelbertrand commented
Issue summary: View changes

@julienjoye you are right, by mistake the fix have left behind the preprocess of the beginning of the function.
Your code seem to be the good one, but i can't reopen this issue as i am not a maintainer.

raphaelbertrand’s picture

raphaelbertrand commented

@julienjoye according to https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-...
it seem you need to open a new issue to provide your fix as this one is in status "closed (fixed)"

Closed (fixed)
This status is used exclusively by the Project issue tracking system to close "Fixed" issues automatically after two weeks of inactivity. You should not need to set this status yourself. The issue is no longer current. Issues that have reached this status should typically not be reopened, but instead, a new issue should be opened, providing a link to the closed issue. Closed issues do not appear in the default view of the issue queue. This provides a cleaner queue, while still maintaining the issues for historical reasons.

julienjoye’s picture

julienjoye commented

@raphaelbertrand Yes no pb. I will do it. Thanks ;)