Updating twig/twig to v1.38.2 (included in Drupal 8.6.11) causes fatal error when embed tags are used in templates

Seems to be solvable in Drupal itself.

The newly introduced '___' in Twig (https://github.com/twigphp/Twig/commit/4eeaf763a2954c2dfbf73fa0194926df9...) doesn't seem to play along nice with the template caching.

Needs tests and more eyes on it, but this patch works for me, let's give it a whirl...

Re-rolling the patch. The patch could be applied to 8.6.11.

We just came to the same conclusion. It looks like this is actually an interaction between some code in Drupal that relies on internal naming within Twig. The linked Twig issue shows that Twig's naming of the internally created template classes has changed to using a triple underscore where it used to use a single underscore, presumably because some checks for its presence were triggering false positives. Drupal itself does something similar in TwigEnvironment, where it amends this behaviour with some of its own. Because of the name mismatch, things go awry.

Right, that'll teach me not to create a patch against the non git-version of Drupal core...

Correct patch format now (Famous Last Words...)

EDIT: Sorry @a.dmitriiev, we seem to have cross-posted there. Your patch is indeed the correct version of my initial patch as well as my patch #10 is.

No problem, Spokje, I just wanted to make my installation working and was not patient to wait, thanks for initial patch.

@a.dmitriiev Did it work for you?

I might be missing something, but the patch doesn't work for me :( I am using particle theme from phase2

Hmm, we're on a custom patternlab implementation theme and for us it seems to work so far.

(It all seemed as a too easy fix anyway...)

We also use components module (https://www.drupal.org/project/components) to place the templates not in theme folder.

It does work for us.

We too are using the Components module (there is probably going to be a lot of overlap with this issue, as it looks to be specific to Twig embeds, and embeds will be much more common if you use the Components module to move your style guide templates to a different location).

We're using "drupal/components": "1.0"

Sorry, I was wrong, I tried on clean installation and on different environment and now can confirm that it works.

Thank you very much.

P.S. I was also stupid enough to miscount the number of underscores in my patch ......
Again, thank you

Increasing the priority to Critical, because sites with the themes that use embed are at the moment broken completely.

Patch #10 seems to at least not break any of the existing tests.

Currently working on a test for this patch.

Can we provide some test to prove the current issue and fix please?

Yeah, NW for tests, thanks everyone! It'd also be good to report this break upstream in Twig because a minor version update shouldn't have broken this either.

Working on Test ATM, upstream was reported (in fact it was where all the "fun" started), link to that issue is in the summary.
And Yay for non-breaking stuff in minor version updates...

EDIT: I'm not a Drupal PHPUnit test guru, so if anybody wants to beat me to it, go for it! :)

The question is whether we should be relying on the naming being in sync. It basically comes down to whether that is part of the "published API", I guess.

Guessing the status update was a mistake.

@xjm: https://github.com/twigphp/Twig/issues/2898

@eelkeblok
> Guessing the status update was a mistake.
Indeed, thanks for fixing that

The patch isn't fixing the errors I'm getting on specific admin routes

@crash_springfield - have you updated to Drupal core 8.6.11 and looked at #3039408: Updating twig/twig to v1.38.0 or v1.38.1 causes fatal error? I had issues on admin routes that the patch there fixed and it is now in 8.6.11. I think this issue only affects themes using the embed function.

So....

Getting core tests to execute is a _lot_ harder than expected, lack of documentation for dummies like me isn't really helping.

This is what I can come up with, can't seem to get it tested locally so I'll just upload it here and let someone with a bigger brain create a real test.
I will look into PHPUnit testing on Core more, but since this is an urgent issue, I'm happy to hand this over for now.

Basically the only thing that needs testing is:

- A Twig template with a {% embed '/something/somewhere' %} ... {% endembed %} breaks Drupal. Even when there's nothing on the ...
- With patch #10 the breakage shouldn't happen.

Hey @Spokje, thanks for taking a shot at it! I think you're on a good track (adding a fixture template and then testing it), but it looks like your patch has some merge conflicts or such in the diff in it and so it won't apply to be tested.

@wells yeah it was updating 8.6.11 that caused the error

@crash_springfield, can you post the specific errors you are getting and on what routes? If it's the same exception as the ones in this issue description, it may also be useful to see the relevant twig template contents.

Made a quick fix for #33 by @Spokje, so the patch can be tested and applied.

@wells here's what I had in my issue that has since been closed as a duplicate:

"/admin/modules/update" and "/admin/modules/install" both work, but "/admin/modules/uninstall" gives the error:

Twig\Error\SyntaxError: An exception has been thrown during the compilation of a template ("Attribute "name" does not exist for Node "Twig\Node\CheckToStringNode".") in "core/themes/stable

"/admin/reports/dblog" gives:

Twig\Error\SyntaxError: An exception has been thrown during the compilation of a template ("Attribute "name" does not exist for Node "Twig\Node\CheckToStringNode".") in "core/themes/classy

"/admin/reports/updates" gives:

Twig\Error\SyntaxError: An exception has been thrown during the compilation of a template("Attribute "name" does not exist for Node "Twig\Node\CheckToStringNode".") in "core/themes/stable

There may be more, but that's all I've come across so far.

Thanks @shaal for correcting my mad mistakes :)

@crash_springfield What theme are you using for Administration theme (Bottom of /admin/appearance)?

This problem only exists in 8.5.x and 8.6.x because #2572605: Make Drupal 8 optionally working with Twig 2, while keeping its dependency on Twig 1 changed the way template classes are being generated. We should work on a test coverage to ensure that is the case. Here's a patch that uses the fix from #2572605: Make Drupal 8 optionally working with Twig 2, while keeping its dependency on Twig 1 which I think we should apply here as well.

@spokje I'm using Adminimal

Thanks @crash_springfield and @Spokje -- that does sound like the issue that #3039408: Updating twig/twig to v1.38.0 or v1.38.1 causes fatal error was supposed to have fixed. Could you open a separate bug report to discuss that issue? (Also be sure you've cleared your site cache.)

Thanks @Spokje and @shaal.

@crash_springfield: checked my theme: all paths you mentioned are working fine on seven.

Also checked paths in Adminimal 8.x-1.4. All working for me.

The patch in #43 applies cleanly and immediately solves the error for me. Working on a PR now to incorporate.

OK -- @lauriii walked me through writing a fail patch (i.e., a test). Here ya go!

Here's an interdiff between #39 and the fail patch in #49.

I think we should include to change to `Twig\Environment` in the patch as well, as that is the new class. The other one is deprecated in 2.7 of twig/twig.

We pair coded the test coverage for #49 and I think it should be ready to go. On top of the automated test coverage, I've tested manually that this fixes the problem.

Thanks @phenaproxima @lauriii (and Acquia) for taking over and especially for the interdiff in #50. Now I see why my test didn't run.

Regarding #51, I think that's out of scope here.

Thanks everyone!

@pdenooijer Please open an issue for that, if one does not already exist.

Yeah that's true, I thought as a bit of boy scouting while we are at it.

Anyway the patch works as my Behat test that where failing are all green now so RTBC +1.

Alright, committed the tests to 8.8.x and 8.7.x, and the full patch to 8.6.x and 8.5.x. Thanks everyone for helping fix this issue quickly! We'll create (yet more...) hotfix releases later today to fix this for themes that were broken by it.

Is the information in comment #49 what fixes this?
I need to fix this in order to apply the 8.6.10 --> 8.6.13 security patch.
Please advise step by step instructions on how to apply this (and will this fix be wiped away with the next theme/core/module update?)

Thanks!

@jeromewiley This was released in 8.6.12. Are you somehow unable to upgrade to 8.6.13 without this applied as a patch?

@cilefen I posted some information in the Drupal slack channel #support (didn't want to add irrelevant thread here) - hope that is ok