Since twig/twig 3.9: error with "twig_escape_filter" function usage in /core/lib/Drupal/Core/Template/TwigExtension.php

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

MR has failure and will need test coverage.

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.

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 $env->getRuntime(EscaperRuntime::class)->escape($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

@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.

believe still will need test coverage

ok no problem. can someone do it ?

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.

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

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')

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.

That timing was perfect.

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

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

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

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

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.

@julienjoye you are right, 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

@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.

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