The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Problem/Motivation
twig/twig Released version 3.11.0 (See https://github.com/twigphp/Twig/releases/tag/v3.11.0)
In it they dropped \Twig\Environment::$templateClassPrefix and hard-coded it to its previous default value '__TwigTemplate_'. (See https://github.com/twigphp/Twig/commit/215f9880f1bbd10cf01317c4cb1b40a4e...)
This makes core/tests/Drupal/KernelTests/Core/Theme/TwigEnvironmentTest.php fail with a merry
Drupal\KernelTests\Core\Theme\TwigEnvironmentTest::testTemplateInvalidation
ReflectionException: Property Twig\Environment::$templateClassPrefix does not exist
core/tests/Drupal/KernelTests/Core/Theme/TwigEnvironmentTest.php:227
EDIT: After fixing the above, deprecation warnings pop-up which we suppress in this issue and fix in a follow-up
Steps to reproduce
Take a look at the test results of the daily 'Updated deps' run, like this one: failed run
Proposed resolution
Since we're testing a not-really-feasible-to-happen-in-real-life-situation in that test, see comment above the failing bit in the test below, I think it's OK to remove the lines that fail from the test.
This wouldn't be a problem on a real site where you reload the page.
Also I think we need to bump the version of twig/twig in our composer.*.
Removing a test will certainly pass on previous versions, but also we don't provide test-coverage for that specific bit in versions lower than version 3.11.0.
Also suppress the deprecation warning in core/.deprecation-ignore.txt that shows after we do the above.
There's a follow-up here to deal with that properly #3467979: Fix deprecation introduced in twig/twig 3.11.0
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Issue fork drupal-3467293
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:
- 3467293-10.4.x
changes, plain diff MR !9235 - 3467293-twigtwig-3.11.0-removed changes, plain diff MR !9152
Comments
Comment #3
spokjeComment #4
spokjeWell, 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
...
Looks like all/the majority of failures are:
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.
Comment #5
spokjeSo our problem comes from https://github.com/twigphp/Twig/commit/e8141bbd348582e44715a98201d7f6b86...
and seems to be that we swap out the
escapefor our owndrupal_escapefilter here: https://git.drupalcode.org/project/drupal/blob/973b6ec18b9649af4deb81e72...That seems to be handled by the constructor now.
Comment #6
spokjeYep, this is above my head.
I would guess we need to do something with the
EscaperRuntimeclass.https://github.com/twigphp/Twig/issues/4102#issue-2323595412
Comment #7
spokjeComment #8
spokjeComment #9
spokjeComment #10
spokjeSo all of the test failures seem to be deprecation warnings:
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.
Comment #11
spokjeComment #12
spokjeOpened follow-up #3467979: Fix deprecation introduced in twig/twig 3.11.0 to fix the deprecation and remove the suppression.
Comment #13
spokjeComment #14
spokjeComment #15
spokjeComment #16
smustgrave commentedNice catch!
Tests are green so don't see any issue there or when applied locally with using 3.11
Believe this is fine.
Comment #17
longwaveAren'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.
Comment #18
spokjeFrom the comment above the to-be-deleted test-case:
Manually change $templateClassPrefix to force a different template classname, [snipped]There is no more
$templateClassPrefixintwig/twig 3.11.0and 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::$templateClassPrefixwas alreadyprivate(at least in 3.10), so it seemed to be frowned upon to change it to begin with.Happy to be convinced otherwise, but I would need pointers where to look for replacement code to use in the to-be-deleted testcase :)
Comment #19
spokjeComment #20
spokjeFor reference, this test was added in #2752961: No reliable method exists for clearing the Twig cache. I skimmed that issue (very) quickly, but 160+ comments from around 2018 about the inner workings of core aren't that easy to comprehend.
At the very least,
\Twig\Environment::$templateClassPrefixseems to be alreadyprivatesince then.FWIW: I also just notice that an integration test in
twig/twigthat looks kinda similar to the one we're discussing here has been removed without replacement in the same commit as mentioned in the IS (https://github.com/twigphp/Twig/commit/215f9880f1bbd10cf01317c4cb1b40a4e...).Comment #22
catchTest 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.
Comment #25
spokjeComment #27
catchBackport looks good. Tagging for release notes.
Committed 0193b22 and pushed to 10.4.x. Thanks!