twig/twig 3.11.0 introduces (for Drupal) breaking changes

Well, that escalated quickly....

https://git.drupalcode.org/issue/drupal-3467293/-/pipelines/249780

Not quite sure why the "Updated deps" did pick up on the one failing kernel test and not and the gazillion others, but here we are.

Also not sure why tests are failing, red crosses are showing, but the overall test-report shows

 0 failures 0 errors
100% success rate

...

Looks like all/the majority of failures are:

    Since twig/twig 3.11: Changing the value of a "filter" node in a
    NodeVisitor class is not supported anymore.

Well, I suppose we shouldn't do that anymore then...

Anyway, what started as a quick fix as a nice start of my day turned into something a bit more involved.
Going to fill up on caffeine before looking at this.

So our problem comes from https://github.com/twigphp/Twig/commit/e8141bbd348582e44715a98201d7f6b86...
and seems to be that we swap out the escape for our own drupal_escape filter here: https://git.drupalcode.org/project/drupal/blob/973b6ec18b9649af4deb81e72...

Yep, this is above my head.

I would guess we need to do something with the EscaperRuntime class.

https://github.com/twigphp/Twig/issues/4102#issue-2323595412

So all of the test failures seem to be deprecation warnings:

1) D:\htdocs\drupal\vendor\symfony\deprecation-contracts\function.php:25
Since twig/twig 3.11: Changing the value of a "filter" node in a NodeVisitor class is not supported anymore.

To prevent out 'Updated deps' run to fail and hide problems with other updates, let's try suppressing that deprecation error and open a follow-up to deal with it properly.

Opened follow-up 📌 [PP-1] twig/twig 3.11.0 introduces (for Drupal) breaking changes Postponed to fix the deprecation and remove the suppression.

Nice catch!

Tests are green so don't see any issue there or when applied locally with using 3.11

Believe this is fine.

Aren't we losing some test coverage by deleting those lines? It looks minimal but I assume it was added for a reason, if we can work around the issue instead of deleting the code then we probably should.

Aren't we losing some test coverage by deleting those lines?

From the comment above the to-be-deleted test-case:

Manually change $templateClassPrefix to force a different template classname, [snipped]

There is no more $templateClassPrefix in twig/twig 3.11.0 and AFAICT there's no way to change the template classname anymore, since it's now hard-coded to `'__TwigTemplate_'`. (https://github.com/twigphp/Twig/commit/215f9880f1bbd10cf01317c4cb1b40a4e...)

So IMHO we're deleting a test for a situation that isn't possible anymore (changing the template class prefix).
Also \Twig\Environment::$templateClassPrefix was already private (at least in 3.10), so it seemed to be frowned upon to change it to begin with.

Happy to be convinced otherwise :)

For reference, this test was added in #2752961: No reliable method exists for clearing the Twig cache →

Test coverage removal explanation makes sense to me.

Committed/pushed to 11.x - I think we probably want to backport this to 10.4 too, so moving for backport there.

catch → closed merge request !9152

Backport looks good. Tagging for release notes.

Committed 0193b22 and pushed to 10.4.x. Thanks!

catch → closed merge request !9235

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