renderInline not compatible with twig_auto_reload

Thanks for the report!

I really don't like the fix, though this does fix it. It almost feels like you wanna have two twig enviroments, one for inline, one for proper rendering.

They seem to have the ability to have them side by side here... http://twig.sensiolabs.org/doc/functions/template_from_string.html

I've not compared how we did ours vs that one, though I really should.

Oh nice! I guess we can indeed use the Twig_loader_chain which is used inside the extension. And get rid of most of your custom code again!

@michelle
I am the author of that inline template functionality so it is kind of my obligation to jump in.

The work is currently blocked by #2251113: Use container parameters instead of settings as it will touch the same exact same places.

This is causing me fatal errors when I go to the Manage Display page in Field UI repeatedly. It is Critical, as the errors are fatal.

If the other issue is resolving soon, OK... I guess it is critical also.

The patch in #3 didn't quite work for me, but I got it to work with this slightly different patch. It's probably not a real fix but if you are being blocked in debugging other stuff by this problem, at least this kludge will possibly get you going again.

xjm: I do not think the patch in #10 is viable. It is a kludge, lacks documentation, etc. It was just something to work around the real problem, which is that the Twig "render inline" functionality is kind of broken with respect to auto-reload.

Just in case someone worries,

Just some general work so far. I am not convinced that the test is working properly, because there should be some container rebuild in the middle of it, though I cannot figure out how to do that properly.

The order sometimes indeed matter.

Merged in #2305799: HTML tags (raw) shown in Status column on Translations page as it was marked as duplicate of this issue.

There we go.

is there a tests or steps (page) to reproduce the error?
The fixed autoescape content translation status is not related to bug and have a patch in #2250841: Adding a inline template for content translation status

Current reproduce: Go to a content type's Manage Display page, with twig autoload turned on, and refresh the page once or twice.

I gave this a test as @jhodgdon mentioned in #20 It surely failed before the patch and this fixes it in a much more elegant solution.

You rock @dawehner!!! Couple questions below:

1. +++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
   @@ -10,6 +10,8 @@
   +use Twig_Loader_Chain;
   +use Twig_Loader_String;
   
   @@ -66,9 +68,10 @@ public function __construct(\Twig_LoaderInterface $loader = NULL, $options = arr
   +    $this->loader = new Twig_Loader_Chain([$loader, new Twig_Loader_String()]);
   

   Just curious as I haven't seen this style in core with using the use statements instead of just rooting them with \. It's not done with \Drupal so wondering if there was a policy change on this? Same question for the square bracket array syntax (which I absolutely love btw;)

2. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
   @@ -137,8 +137,25 @@ public function overview(Request $request, $entity_type_id = NULL) {
   -          // @todo Remove as part of https://drupal.org/node/2250841.
   -          $status = '<span class="status">' . $status . '</span>' . (!empty($translation['outdated']) ? ' <span class="marker">' . $this->t('outdated') . '</span>' : '');
   +          $args  = array('status' => $status);
   +          if (empty($translation['outdated'])) {
   +            $status = array(
   +              'data' => array(
   +                '#type' => 'inline_template',
   +                '#template' => '<span class="status">{{ status }}</span>',
   +                '#context' => $args,
   +              )
   +            );
   +          }
   +          else {
   +            $status = array(
   +              'data' => array(
   +                '#type' => 'inline_template',
   +                '#template' => '<span class="status">{{ status }}</span><span class="marker">{{ \'outdated\'|t }}</span>',
   +                '#context' => $args,
   +              )
   +            );
   +          }
   

   Can we remove this as it's being taken care of and looks a bit better in #2250841: Adding a inline template for content translation status Though I agree with you it should be in the template if possible. Though I think it should be left out here.

3. +++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
   @@ -66,9 +68,10 @@ public function __construct(\Twig_LoaderInterface $loader = NULL, $options = arr
        parent::__construct($loader, $options);
   +
   +    $this->loader = new Twig_Loader_Chain([$loader, new Twig_Loader_String()]);
   

   You can pass NULL to the loader option here in the __construct() or pass the new Twig_Loader_Chain to that param, both will do the same thing.

Thank you for the review!

Still cannot reproduce the failure in a simple and reproducible way.

You can pass NULL to the loader option here in the __construct() or pass the new Twig_Loader_Chain to that param, both will do the same thing.

True, though i prefer to call the parent with as much as possible. You never know what the parent might do at some point.

One thing I found while testing: I need twig_cache ON (the default unless you uncomment). I have all 3 usually uncommented for testing in my local.settings.php but to test I just uncommented the twig_autoload line and that seemed to work well like @jhodgdon mentioned. Twig is trying to look up the cached inline template on the second request for the same template.

Not sure that helps, but thought I'd share:)

@dawehner please file a "fail" patch (test only), to make sure that it covers bug

FYI, it doesn't at the moment, just tried it locally.

Well here's a simple fail patch based on what you had in there, though unfortunately the awesome code above doesn't solve it:( I kinda still want that code to go in regardless of whether it fixes the patch because it's much more elegant of a solution (less custom drupal code)

I'm an idiot, disregard #26 the element is getting #printed = true...

@dawehner please file a "fail" patch (test only), to make sure that it covers bug

Let me quote myself:

Still cannot reproduce the failure in a simple and reproducible way.

Back to needs review. We though still need some proper failing test.

@Michelle As you can see #26 shows that test reproduces error, not sure "twig_debug" (test could generate permutations) there was no mentions about.

At this point #22 looks rtbc!

@andypost I agree, it solves the problem with a manual test. Though @dawehner and I are both kinda struggling to come up with an automated test to trigger this bug.

@dawehner maybe just a WebTestBase/SimpleTest hitting a path twice with a inline template with that setting for twig_auto_reload turned on would suffice?

@Michelle thank you for helping out! twig_debug will actually implicitly turn on twig_auto_reload ( think the docs above it in the local settings file indicate as much). So both should trigger the error as long as caching is still on. It looks for the cached class though it didn't used to generate it in the same place I guess with how it was previously implemented.

At least that's my take on the issue:)

For reference:

line 104 of core/vendor/twig/twig/lib/Twig/Environment.php
$this->autoReload = null === $options['auto_reload'] ? $this->debug : (bool) $options['auto_reload'];

and sites/example.settings.local.php

 * Automatically recompile Twig templates whenever the source code changes. If
 * you don't provide a value for twig_auto_reload, it will be determined based
 * on the value of twig_debug.

Yes indeed, I guess some webtest could trigger it as well. Though it would be good to understand it,
so the integration test could be written as well. Failing on lower levels always helps to better understand the problem, once
there is a failure.

Rerolled against HEAD.

@Michelle - the docs are right, there must be something else happening in this case. twig_debug definitely implies twig_auto_reload = TRUE, the only way I know to make it do otherwise is to set twig_auto_reload = FALSE.

Also worth mentioning the way to change the twig_* settings has changed:

https://www.drupal.org/node/1922666
https://www.drupal.org/node/2317985

Are we sure this still fails? admin/modules/uninstall

Well the way to set this setting has changed: you cannot do it in your local default.settings.php or a local overrides file any more, it has to go into a YML file like *.services.yml instead. I'm also not sure the uninstall page had this problem; the one I knew about was the field UI manage display/form display pages.

Well the way to set this setting has changed: you cannot do it in your local default.settings.php or a local overrides file any more, it has to go into a YML file like *.services.yml instead. I'm also not sure the uninstall page had this problem; the one I knew about was the field UI manage display/form display pages.

Sure, not innocent here.

After dealing with the manage display/form page I realized that it is impossible to test with a pure KernelTest, because there is a class_exists() call inside the Twig Environment class. In PHP
you cannot unload an existing class, so we need some webtest ...

I just have to say, wow the functional changes here are really great. So much cleaner! Just have a few small points below, otherwise RTBC from me. Thanks @dawehner.

1. +++ b/core/modules/system/src/Tests/Theme/TwigEnvironmentTest.php
   @@ -40,6 +41,27 @@ public function testInlineTemplate() {
   +
   +
   +    // Enable twig_auto_reload and twig_debug.
   

   Nitpick alert: one extra blank line I think.

2. +++ b/core/modules/system/src/Tests/Theme/TwigEnvironmentTest.php
   @@ -40,6 +41,27 @@ public function testInlineTemplate() {
   +    // Ensure to call it twice that twig caching is triggered.
   

   Maybe "Render it twice so that twig caching is triggered."

3. +++ b/core/modules/system/src/Tests/Theme/TwigSettingsTest.php
   @@ -118,4 +118,20 @@ function testTwigCacheOverride() {
   +  /**
   +   * Tests twig inline templates with autoreload.
   +   */
   +  public function testTwigInlineWithAutoreload() {
   

   auto_reload and AutoReload please :)

I really like that basically removing code helps to solve bugs.

Thank you for the review!

Works for me! Test coverage looks great and this removes a lot of "special case" code in favour of using the standard loader chain mechanism.

Oops, I think we need to remove:

  /**
   * The string loader implementation used for inline template rendering.
   *
   * @var \Twig_Loader_String
   */
  protected $stringLoader;

Sure, let's do it properly.

+1, thanks!

Inline Render Test conflict. Rerolled, no changes.

Adding back the missing use.

Sorry for the noise. Some redundant tests snuck in there too. Interdiff from #54. Thanks for catching this @joelpittet!

I totally saw that use and ignored it in the re-roll. Damn, slipping...

Committed/pushed to 8.0.x, thanks!