I was working on #2305799: HTML tags (raw) shown in Status column on Translations page with chx and he suggested setting $settings['twig_auto_reload'] = TRUE;. When I did, I started getting this error :

Uncaught PHP Exception Twig_Error_Loader: "Unable to find template "<span class="status">{{ status }}</span>" (looked into: /var/www/html/d8.dev)." at /var/www/html/d8.dev/core/vendor/twig/twig/lib/Twig/Loader/Filesystem.php line 202, referer: http://localhost/d8.dev/node/1

That error is occurring in function findTemplate in class Twig_Loader_Filesystem. It shouldn't be trying to find a template because it is an inline one.

I thought I had traced the code to show the error but I realized when writing it up that I goofed. I do know, though, that if I turn the twig_auto_reload off and clear the cache that the error goes away. So I'm pretty sure there is a bug here but haven't found the exact code that is at issue, yet. I'm going to keep working on this but if someone has a debugger and can find it faster, feel free to jump in. :)joelpittet

Comments

Cottser’s picture

Issue tags: +Twig

Thanks for the report!

Michelle’s picture

So it turns out the bit of code I suspected causes this actually does thanks to now having a debugger and jerdavis pointing out how I could easily test to see if it was that bit. Here is the code in question:

 if (!class_exists($cls, FALSE)) {
      $cache_filename = $this->getCacheFilename($name);

      if ($cache_filename === FALSE) {
        $compiled_source = $this->compileSource($this->getLoader($inline)->getSource($name), $name);
        eval('?' . '>' . $compiled_source);
      }
      else {

        // If autoreload is on, check that the template has not been
        // modified since the last compilation.
        if ($this->isAutoReload() && !$this->isFresh($cache_filename, $name)) {
          $this->updateCompiledTemplate($cache_filename, $name, $inline);
        }

        if (!$this->storage()->load($cache_filename)) {
          $this->updateCompiledTemplate($cache_filename, $name, $inline);
          $this->storage()->load($cache_filename);
        }
      }

I'm still not understanding enough to quite get what is wrong but it making it so the "else" never runs keeps the issue from happening.

Additional WTF: I wrote all of the above before I glanced at the issue summary and realized that the error I'm getting has changed. It's still all the same section of code and process to recreate but the error is now:

PHP Fatal error: Class '__TwigTemplate_71a5af1b9579abfc302a2eb4cc1ca4dccca70df6829d23271495db8b8daf3510' not found in /var/www/html/d82.dev/core/lib/Drupal/Core/Template/TwigEnvironment.php on line 171

I'm going to keep investigating.

dawehner’s picture

FileSize
1.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,364 pass(es). View

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.

joelpittet’s picture

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.

dawehner’s picture

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’s picture

Thanks for jumping in on this. I'm still very new to Drupal 8 and better at finding bugs than fixing them at this point. :)

dawehner’s picture

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

Michelle’s picture

Heh, that's how I feel as the author of this bug report. I feel like I should be keeping on helping but I'm afraid I got in over my head and also ran out of TCDrupal time. If I see something more I can do to help once it's unblocked, I will jump in. Otherwise I'll leave it to you people that have a clue. :)

jhodgdon’s picture

Priority: Normal » Critical

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.

jhodgdon’s picture

FileSize
2.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,078 pass(es). View
886 bytes

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’s picture

Status: Active » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

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.

dawehner’s picture

Assigned: Unassigned » dawehner

Just in case someone worries,

dawehner’s picture

Status: Needs work » Needs review
FileSize
5.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 42,747 pass(es), 19,462 fail(s), and 1,308 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 14: theme-2317557-14.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch theme-2317557-16.patch. Unable to apply patch. See the log in the details link for more information. View
581 bytes

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.

Status: Needs review » Needs work

The last submitted patch, 16: theme-2317557-16.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,086 pass(es). View

There we go.

andypost’s picture

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

jhodgdon’s picture

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

joelpittet’s picture

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.

dawehner’s picture

FileSize
3.7 KB
7.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,121 pass(es). View

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.

joelpittet’s picture

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

andypost’s picture

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

joelpittet’s picture

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

joelpittet’s picture

FileSize
740 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,115 pass(es), 1 fail(s), and 0 exception(s). View

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)

joelpittet’s picture

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

Status: Needs review » Needs work

The last submitted patch, 26: 2317557-fail.patch, failed testing.

dawehner’s picture

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

dawehner’s picture

Status: Needs work » Needs review

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

Michelle’s picture

I'm not sure what is needed to show that it fails. What started it was this patch: https://www.drupal.org/node/2305799#comment-9023531 . chx suggested I turn twig_auto_reload on and I started getting the error. If I turned it off, the error went away.

I'm not sure if it's relevant to this issue but, just in case, I also had the twig_debug set to true. According to https://www.drupal.org/node/1903374 twig_auto_reload should have taken its value from twig_debug when I commented out the line that set it to true but it doesn't appear to since the error would go away even though twig_debug was still true.

andypost’s picture

Issue summary: View changes

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

joelpittet’s picture

@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’s picture

@andypost - My question was because #30 says, "We though still need some proper failing test." I was trying to figure out if that is something I can do because I'm still trying to help out here. I guess I'm still out of my depth, though. :( As for the twig_debug, I don't know if it is related or not but thought it was worth mentioning just in case my having it on was a factor even though it didn't appear to be.

joelpittet’s picture

@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:)

joelpittet’s picture

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.
dawehner’s picture

Assigned: dawehner » Unassigned

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.

Michelle’s picture

@joelpittet - My point was that the docs are wrong. I had twig_debug = true the whole time I was testing this bug and I either had twig_auto_reload = true or commented out. I never explicitly set it to false. If the docs are correct, then I should have been hitting the bug the entire time as twig_auto_reload would have been true the whole time since I never forced it to false. But the bug didn't happen when the line setting it to true was commented out. It's a different issue, I realize, but I thought it was worth mentioning here because it could affect testing of this bug if people are going by the docs.

dawehner’s picture

FileSize
7.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,159 pass(es). View

Rerolled against HEAD.

Cottser’s picture

@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

Michelle’s picture

@Cottser - Well, I don't know why the bug would only happen when I had it set to true, then. That makes no sense. But I don't want to be sidetracking this issue. I only mentioned it here because it could affect testing this issue. When I get a chance, I'll investigate that problem separately and file an issue if I can figure it out.

dawehner’s picture

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

jhodgdon’s picture

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.

dawehner’s picture

FileSize
3.9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,252 pass(es), 2 fail(s), and 0 exception(s). View
9.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,248 pass(es). View

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

The last submitted patch, 44: theme-2317557-FAIL.patch, failed testing.

Cottser’s picture

Status: Needs review » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,247 pass(es). View
1.66 KB

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

Thank you for the review!

Cottser’s picture

Status: Needs review » Reviewed & tested by the community

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.

Cottser’s picture

Status: Reviewed & tested by the community » Needs work

Oops, I think we need to remove:

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

Status: Needs work » Reviewed & tested by the community
FileSize
10.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2317557-theme-49.patch. Unable to apply patch. See the log in the details link for more information. View
618 bytes

Sure, let's do it properly.

Cottser’s picture

+1, thanks!

chx’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 50: 2317557-theme-49.patch, failed testing.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
10.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,692 pass(es), 1 fail(s), and 0 exception(s). View

Inline Render Test conflict. Rerolled, no changes.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 54: renderinline_not-2317557-54-reroll-50.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
FileSize
10.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
469 bytes

Adding back the missing use.

Cottser’s picture

FileSize
10.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,842 pass(es). View
1.02 KB

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

The last submitted patch, 56: 2317557-56.patch, failed testing.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 08b9294 on 8.0.x
    Issue #2317557 by dawehner, Cottser, joelpittet, jhodgdon: Fixed...

Status: Fixed » Closed (fixed)

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