Add @znerol to commit credit

Problem/Motivation

Twig 1.22.0 introduces the concept of cache classes so we don't have to override as much of \Twig_Environment, but it also breaks our overrides.

Proposed resolution

Upgrade Twig to 1.22.0 and introduce our own cache class in this issue. Twig 2.x compatibility will be handled in #2568181: [META] Update to Twig 2.x in Drupal 9.

Remaining tasks

Review, address feedback, commit.

User interface changes

n/a

API changes

There will be some changes to \Drupal\Core\Template\TwigEnvironment which hopefully nobody is relying on but we've seen in contrib people extending that instead of the vendor \Twig_Environment for no good reason. In general those shouldn't break, people would have to be relying on methods we will be removing around caching, storage, etc.

Data model changes

None

CommentFileSizeAuthor
#79 interdiff.txt850 bytesstar-szr
#79 upgrade_to_twig_1_22-2568171-79.patch59.65 KBstar-szr
#76 upgrade_to_twig_1_22-2568171-76.patch59.66 KBcilefen
#76 interdiff-2568171.txt2.31 KBcilefen
#73 interdiff.txt798 bytesstar-szr
#73 upgrade_to_twig_1_22-2568171-73.patch59.9 KBstar-szr
#69 interdiff.txt8.77 KBstar-szr
#69 upgrade_to_twig_1_22-2568171-68.patch60.05 KBstar-szr
#63 interdiff.txt3.46 KBstar-szr
#63 upgrade_to_twig_1_22-2568171-63.patch61.47 KBstar-szr
#58 interdiff.txt5.57 KBstar-szr
#58 upgrade_to_twig_1_22-2568171-58.patch61.12 KBstar-szr
#51 interdiff.txt750 bytesstar-szr
#51 upgrade_to_twig_1_22-2568171-51.patch63.79 KBstar-szr
#50 interdiff.txt907 bytesWim Leers
#50 upgrade_to_twig_1_22-2568171-50.patch63.5 KBWim Leers
#47 interdiff-no-vendor.txt2.28 KBstar-szr
#47 interdiff.txt7.99 KBstar-szr
#47 upgrade_to_twig_1_22-2568171-47.patch62.73 KBstar-szr
#44 interdiff.txt2.31 KBdawehner
#44 2568171-44.patch61.52 KBdawehner
#40 interdiff.txt580 bytesstar-szr
#40 upgrade_to_twig_1_22-2568171-40.patch61.65 KBstar-szr
#32 interdiff.txt677 bytesstar-szr
#32 upgrade_to_twig_1_22-2568171-31.patch61.67 KBstar-szr
#29 interdiff.txt676 bytesstar-szr
#29 upgrade_to_twig_1_22-2568171-29.patch61.67 KBstar-szr
#23 interdiff.txt2.69 KBdawehner
#23 2568171-23.patch61.67 KBdawehner
#19 upgrade_to_twig_1_22_0-2568171-19.patch64.14 KBhussainweb
#19 interdiff-12-19.txt8.35 KBhussainweb
#14 testing-patch-2568171-twig-1-20-14.patch126.18 KBhussainweb
#12 interdiff.txt6.15 KBstar-szr
#12 upgrade_to_twig_1_22_0-2568171-12.patch63.25 KBstar-szr
#9 upgrade_to_twig_1_22_0-2568171-9.patch58.42 KBstar-szr
#4 interdiff.txt935 bytesstar-szr
#4 upgrade_to_twig_1_22_0-2568171-4.patch59.28 KBstar-szr
#2 upgrade_to_twig_1_22_0-2568171-2.patch59.95 KBstar-szr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Cottser created an issue. See original summary.

star-szr’s picture

Status: Active » Needs review
FileSize
59.95 KB
star-szr’s picture

+++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
@@ -216,7 +95,7 @@ public function getTemplateClass($name, $index = NULL) {
-      $this->templateClasses[$cache_index] = $this->templateClassPrefix . hash('sha256', $this->loader->getCacheKey($name)) . (NULL === $index ? '' : '_' . $index);
+      $this->templateClasses[$cache_index] = $this->getTemplateClassPrefix() . hash('sha256', $this->loader->getCacheKey($name)) . (NULL === $index ? '' : '_' . $index);

Technically this one doesn't have to happen here, I may move this into the general Twig 2.0 compatibility issue which I'm creating shortly.

star-szr’s picture

This removes that change so it doesn't conflict with #2568181: [META] Update to Twig 2.x in Drupal 9. Or at least doesn't conflict as much…

star-szr’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
@@ -0,0 +1,120 @@
+    else {
+      // @todo Add race condition handling stuff around here?
+      // Compile and eval? How do we get to the Twig env from here?
+      // Maybe we can just fall back to the null cache somehow or incorporate this into write().
+    }

Based on my testing so far we don't need to do any special handling for the race condition anymore so this can potentially be removed but needs some more confirmation and testing. #2429659: Race conditions in the twig template cache

dawehner’s picture

Great work!

  1. +++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
    @@ -66,7 +51,6 @@ class TwigEnvironment extends \Twig_Environment {
         $this->templateCacheFilenamePrefix = $twig_extension_hash;
    

    Afaik we don't need this either anymore.

  2. +++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
    @@ -84,118 +68,13 @@ public function __construct($root, CacheBackendInterface $cache, $twig_extension
    +      // @todo Try to use $this->setCache()?
    

    IMHO pointless as its just the BC layer we we no longer have to care about.

  3. +++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
    @@ -84,118 +68,13 @@ public function __construct($root, CacheBackendInterface $cache, $twig_extension
    +      $options['cache'] = new TwigPhpStorageCache($cache, $twig_extension_hash);
    
    +++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
    @@ -0,0 +1,120 @@
    +class TwigPhpStorageCache implements \Twig_CacheInterface {
    

    Just an idea, we could write a NestedTwigCache which gets the PHP and the NULL one injected?

  4. +++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
    @@ -0,0 +1,120 @@
    +  protected $cache_object = NULL;
    ...
    +    $this->cache_object = $cache;
    

    Let's rename it to "cache"

  5. +++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
    @@ -0,0 +1,120 @@
    +    return $this->storage()->exists($key);
    

    ah nice!

  6. +++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
    @@ -0,0 +1,120 @@
    +      // @todo Add race condition handling stuff around here?
    +      // Compile and eval? How do we get to the Twig env from here?
    +      // Maybe we can just fall back to the null cache somehow or incorporate this into write().
    

    I think we would maybe better have a workaround, as one process might cleanup the files, while another has had made the has() call already.

  7. +++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
    @@ -0,0 +1,120 @@
    +    $cid = 'twig:' . $key;
    +    if ($cache = $this->cache_object->get($cid)) {
    +      return $cache->data;
    +    }
    

    What should we do in that case? Are we fine with the cache to disappear at some point in time?

star-szr’s picture

Yeah I thought it would be cool if we could kind of do a chained cache similar to how we do Twig loaders using the vendor \Twig_Loader_Chain. Now I have a better idea of how it would actually be implemented after having some distance from all the other changes. Thanks for the review, if nobody beats me to it I'll work on this again tonight :)

webchick’s picture

star-szr’s picture

Status: Needs work » Needs review
FileSize
58.42 KB

Thanks @webchick here's a tiny reroll so this still applies, no other changes yet.

star-szr’s picture

Status: Needs review » Needs work
star-szr’s picture

Picking this up again.

star-szr’s picture

Status: Needs work » Needs review
FileSize
63.25 KB
6.15 KB

Addressing feedback from #6, I don't know the answer to #6.7 so just put some code in for now.

This brings in the concept of a chained cache class. Overall I'm not sure how much further this gets us unfortunately. I tested the eight combinations of returning FALSE or throwing exceptions in TwigPhpStorageCache. I tested each scenario with warm and cold Twig caches and found no differences.

has() load() write() Result
TRUE Normal Normal OK
TRUE Normal Exception OK
TRUE Exception Normal Class not found
TRUE Exception Exception Class not found
FALSE Normal Normal Class not found
FALSE Normal Exception OK
FALSE Exception Normal Class not found
FALSE Exception Exception OK

It falls down if load() in the TwigPhpStorageCache class fails because when it gets to the Null cache class there is of course nothing to load. So this seems to be the race condition again and I don't know any way we can fix this in Twig cache class land because load() only has the cache key so it can't compile the template source as far as I can tell unless we reverse engineer the cache key and inject the Twig environment or something similarly absurd.

So maybe we still are stuck with overriding loadTemplate() for ourselves…

Feedback, please! And let's see what the testbots say.

Edit: Actually I think I changed something partway through testing because now simply returning FALSE from has() is resulting in a class not found, updated the table accordingly after retesting. A bit whack-a-mole.

star-szr’s picture

So removing the $cache->has() from \Drupal\Core\Template\TwigCacheChain::load() fixes that case (returning FALSE from has() in TwigPhpStorageCache) but results in a WSOD when returning FALSE from has() and throwing an exception in the cache writing but only when the Twig cache is warm.

Fun stuff.

hussainweb’s picture

+++ b/core/composer.json
@@ -17,7 +17,7 @@
-    "twig/twig": "~1.21.2",
+    "twig/twig": "~1.20",

I am wondering why we are doing this here. I am uploading a patch with changes in core/lib from #12 and Twig 1.20.0. If this patch passes (as #12 is passing), that means we can safely go ahead with this version constraint. Otherwise, we should raise the constraint to ~1.22.

For reference, this is the command I used to load twig 1.20.

hw@d8:/var/www/d8task/core-[git 8.0.x] $ composer require twig/twig:"1.20"
./composer.json has been updated
Loading composer repositories with package information
Updating dependencies (including require-dev)
  - Removing twig/twig (v1.22.0)
  - Installing twig/twig (v1.20.0)
    Downloading: 100%

Writing lock file
Generating autoload files
> Drupal\Core\Composer\Composer::preAutoloadDump
> Drupal\Core\Composer\Composer::ensureHtaccess
dawehner’s picture

I am wondering why we are doing this here. I am uploading a patch with changes in core/lib from #12 and Twig 1.20.0. If this patch passes (as #12 is passing), that means we can safely go ahead with this version constraint. Otherwise, we should raise the constraint to ~1.22.

Sigh, ... because twig 1.22 introduced some additional APIs we want to implement to be able to switch to twig 2, see issue summary.

@cottser
I'll give feedback later. Great research!

Status: Needs review » Needs work

The last submitted patch, 14: testing-patch-2568171-twig-1-20-14.patch, failed testing.

hussainweb’s picture

@dawehner: I think one of us is misunderstanding. I am expecting the patch in #14 to fail as the API changes were described as BC breaking somewhere. If the tests pass, then all is fine. Otherwise, we should raise it to ~1.22.

The last submitted patch, 14: testing-patch-2568171-twig-1-20-14.patch, failed testing.

hussainweb’s picture

Title: Upgrade to Twig 1.22.0 and implement our own cache class » Upgrade to Twig 1.22 and implement our own cache class
FileSize
8.35 KB
64.14 KB

As expected, the patch in #14 failed. I have updated the version constraint to ~1.22. The interdiff is from that patch. Also, it seems there was a new version 1.22.1 in the meantime, which is included in the patch.

hussainweb’s picture

Status: Needs work » Needs review
dawehner’s picture

@hussainweb
Thank you for moving the issue back into scope :)

hussainweb’s picture

@dawehner, :)

I think it was still in scope. The line I was picking about was in the patch and important. It said that the changes could work with Twig 1.20, which was certainly not the case. Anyway, it's now set at ~1.22 as originally intended.

dawehner’s picture

Just some minor improvements. I think this is looking pretty good. It would be great if fabian could have a look at it, fabien of course too.

Status: Needs review » Needs work

The last submitted patch, 23: 2568171-23.patch, failed testing.

The last submitted patch, 23: 2568171-23.patch, failed testing.

star-szr’s picture

@dawehner thanks! Those changes look great. I was wondering if there was some nice assertion thingy to use :)

+++ b/core/lib/Drupal/Core/Template/TwigCacheChain.php
@@ -76,14 +71,12 @@ public function load($key) {
+    // We want to write to all of them.

I'm wondering if this fixes the problems I saw, I can test later. Would this result in always eval()'ing the template?

star-szr’s picture

I also wonder if other than the assertion this might be of interest to upstream since there is already a Twig loader chain.

webchick’s picture

Priority: Major » Critical

Due to the BC break, I think this needs to be critical.

star-szr’s picture

Status: Needs work » Needs review
FileSize
61.67 KB
676 bytes

Not sure about the criticalness. To me all this may be considered internal but not sure.

I assume this will work better for the assertions.

Status: Needs review » Needs work

The last submitted patch, 29: upgrade_to_twig_1_22-2568171-29.patch, failed testing.

dawehner’s picture

Due to the BC break, I think this needs to be critical.

Mh, I think its a BC break for internal APIs not for themes, so I'm not convinced that this needs to be done now.
On the other hand we want to follow twig 1.x at least in order to be able to catch security issues.

+++ b/core/lib/Drupal/Core/Template/TwigCacheChain.php
@@ -26,7 +26,7 @@ class TwigCacheChain implements \Twig_CacheInterface {
-    assert('Drupal\\Component\\Assertion\\Inspector::assertAllObjects($collection, \'\\Twig_CacheInterface\'');
+    assert('Drupal\\Component\\Assertion\\Inspector::assertAllObjects($caches, \'\\Twig_CacheInterface\'');
     $this->caches = $caches;

Ups, this was a c&p problem :)

star-szr’s picture

Status: Needs work » Needs review
FileSize
61.67 KB
677 bytes

Just a heads up, always trying to write the cache seems to result in a WSOD with cold caches.

Trying this assert business again. \\ \ \ \\ \ \ The docs seem to be quite wrong on this.

Edit: Interdiff is from #23.

Status: Needs review » Needs work

The last submitted patch, 32: upgrade_to_twig_1_22-2568171-31.patch, failed testing.

The last submitted patch, 29: upgrade_to_twig_1_22-2568171-29.patch, failed testing.

The last submitted patch, 32: upgrade_to_twig_1_22-2568171-31.patch, failed testing.

dawehner’s picture

If we assertion is the problem, just remove it. I was just adding as I considered it to be a better solution compared to the if instanceof calls. Do we actually pass something in which does not implement the interface?

dawehner’s picture

+++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
@@ -84,118 +59,15 @@ public function __construct($root, CacheBackendInterface $cache, $twig_extension
+    $php_storage_cache = new TwigPhpStorageCache($cache, $twig_extension_hash);
+    $null_cache = new \Twig_Cache_Null();
...
+    if ($options['cache'] === TRUE) {
+      $options['cache'] = new TwigCacheChain([$php_storage_cache, $null_cache]);
     }

We should certainly explain why we need both here: race conditions when some process clears it + general fallback

star-szr’s picture

Agreed we need a comment there. I don't think it's the assertions causing problems now. When there is a cold cache if you don't return after the phpstorage write you get a WSOD.

FWIW those if's were modeled after the twig loader chain class.

star-szr’s picture

Status: Needs work » Needs review
FileSize
61.65 KB
580 bytes

I'm not sure always trying to write the cache helps us, at least not in the scenario @dawehner brought up in #6.6:

I think we would maybe better have a workaround, as one process might cleanup the files, while another has had made the has() call already.

Here's the code from the vendor \Twig_Environment with the BC handling removed (we're not using the BC layer):

if (!$this->cache->has($key) || ($this->isAutoReload() && !$this->isTemplateFresh($name, $this->cache->getTimestamp($key)))) {
  $this->cache->write($key, $this->compileSource($this->getLoader()->getSource($name), $name));
}

$this->cache->load($key);

So this just adds back the return because things seems to be failing at least with how the vendor Null cache is implemented (it's just an eval()).

star-szr’s picture

Status: Needs review » Needs work

I forgot to add a comment explaining why we are adding the null cache class to the chain, so needs work for that at least and I'm done for the night.

dawehner’s picture

Assigned: Unassigned » Fabianx

@Cottser Ah yeah, well I think this is a bug, see https://github.com/twigphp/Twig/issues/1832 ... I mean basically for now we never get to the null callback, as we always write to just the phpstorage based one.

Assigning to fabian for a review, as proper subsystem maintainer.

Wim Leers’s picture

  1. +++ b/core/lib/Drupal/Core/Template/TwigCacheChain.php
    @@ -0,0 +1,95 @@
    +  /**
    +   * The cache classes to chain.
    +   *
    +   * @var \Twig_CacheInterface[]
    +   */
    +  protected $caches = [];
    

    It's not just the cache classes to chain. It's an ordered list of cache classes to chain. Right now, the comment indicates that the order doesn't matter, but it of course does matter greatly.

  2. +++ b/core/lib/Drupal/Core/Template/TwigCacheChain.php
    @@ -0,0 +1,95 @@
    +      catch (\RuntimeException $e) {
    +      }
    ...
    +      catch (\RuntimeException $e) {
    +      }
    

    This is very strange, at least needs a comment.

  3. +++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
    @@ -0,0 +1,120 @@
    +    // extensions, so we use the twig extension hash which varies by extensions
    

    Nit: s/twig/Twig/

  4. +++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
    @@ -0,0 +1,120 @@
    +    // Save the last modification time.
    

    Nit: Everywhere else in this class, we say mtime, so shouldn't we do here too?

dawehner’s picture

Status: Needs work » Needs review
FileSize
61.52 KB
2.31 KB

Let's at least temporarily try it out to see why its problematic, so we can document it properly

Status: Needs review » Needs work

The last submitted patch, 44: 2568171-44.patch, failed testing.

The last submitted patch, 44: 2568171-44.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
FileSize
62.73 KB
7.99 KB
2.28 KB

@dawehner thanks for that upstream issue, and thanks @fabpot for the quick turnaround there.

The try/catch is so that the cache class can notify the cache chain to go to the next cache on failure or some other condition. I used RuntimeException because the Filesystem vendor cache class already uses that.

Here is a patch that subclasses the new vendor Null cache class' has() method to check if the buffer contains the eval()'d template. Based on our chained loader's implementation of load() it will never get to the vendor's Null cache class load() because we check has() first, and the Null cache class' has() method always returns false. But I'm pretty sure we do want to check has() first.

I don't have time to thoroughly test this now but we'll see what the bots say about this theory.

Edit: Just a note that I think #44's interdiff (and patch) is from #23, and this interdiff is from #40 with #44's interdiff applied :)

Wim Leers’s picture

That looks much better :)

Status: Needs review » Needs work

The last submitted patch, 47: upgrade_to_twig_1_22-2568171-47.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
63.5 KB
907 bytes

https://github.com/twigphp/Twig/issues/1832 causes Twig's NULL cache to now return the class name for generateKey() (see https://github.com/twigphp/Twig/commit/9fde02fe552256722466c9985520b6f50...), and so… this assertion no longer is true:

    // This should return false after rebuilding the service container.
    $new_cache_filename = $this->container->get('twig')->getCacheFilename($template_filename);
    $this->assertFalse($new_cache_filename, 'Twig environment does not return cache filename after caching is disabled.');

AFAICT we should replace it with a TwigEnvironment::getCache() assertion. This patch does that.

star-szr’s picture

Thanks @Wim Leers!

Also private messes things up so this brings some more things over from the vendor Null class. If there's a better way to do this I'm all ears. But I tested by just removing the PhpStorage cache class from the chain and it didn't work at all of course because it wasn't able to check the private buffer property.

The last submitted patch, 47: upgrade_to_twig_1_22-2568171-47.patch, failed testing.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Template/TwigCacheNull.php
@@ -0,0 +1,40 @@
+  public function has($key) {
+    return isset($this->buffer[$key]);
+  }
...
+  public function write($key, $content) {
+    $this->buffer[$key] = $content;
+  }
+
...
+  public function load($key) {
+    eval('?>' . $this->buffer[$key]);
+
+    unset($this->buffer[$key]);
+  }

Does this then actually works as a fallback? Let's assume has() return TRUE, and then in the meantime something clears the stored templates ...

star-szr’s picture

I think it would be good to again test what happens when parts of the PhpStorageCache fail as I did in #12 but that doesn't really handle the race condition scenario, just makes sure we have fallbacks.

I still think the scenario you described would result in class not found because from loadTemplate() as soon as has() is TRUE then no writing happens, it goes right to load. So once you get to that point if the PhpStorage suddenly goes away then there would be nothing to load from the null cache as nothing has been written.

@dawehner I am assuming in #53 you're still talking about clearing the PhpStorage cache storage, not the Null cache storage.

@joelpittet on IRC mentioned locking, I have no idea if that's something we can incorporate here or not.

dawehner’s picture

@joelpittet on IRC mentioned locking, I have no idea if that's something we can incorporate here or not.

Mh, how would that help? We aren't in the process of writing, but rather on loading. Just the files disappear from disk, we should provide some fallback for that.

dawehner’s picture

The easiest thing we could do is to simply remove the chain and put the fallback logic into load of the PHPstorage cache. This feels like a good fix for the problem.
@Cottser @Fabianx Any thoughts about that?

star-szr’s picture

Discussed in IRC, we have a plan. Working on it.

star-szr’s picture

Right now I think we still need to override loadTemplate() but putting this up as kind of progress. It should fail some tests.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
    @@ -0,0 +1,133 @@
    +  public function load($key) {
    +    // if (!$this->storage()->load($key)) {
    

    Just as idea, we could actually provide some testing, by mocking the way how storage and return has() TRUE but then let load() fail. In that particular case though, write() would not be triggered and this protection would never help. Mh, this is tricky.

  2. +++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
    @@ -0,0 +1,133 @@
    +    if (TRUE) {
    ...
    +    }
    

    Can we check whether we have $this->templateSourceCache available? Otherwise we can really not do anything beside failing.

Status: Needs review » Needs work

The last submitted patch, 58: upgrade_to_twig_1_22-2568171-58.patch, failed testing.

The last submitted patch, 58: upgrade_to_twig_1_22-2568171-58.patch, failed testing.

star-szr’s picture

Commented upstream because I think we are stuck: https://github.com/twigphp/Twig/issues/1811#issuecomment-140833294

star-szr’s picture

Status: Needs work » Needs review
FileSize
61.47 KB
3.46 KB

Feedback on this approach please (humans and testbots).

This would need a PR upstream but want to see what people here think before sending it.

dawehner’s picture

This looks pretty solid for me. I'd though add a comment in the Twig_Environment class explaining why we need such a call (race conditions ... )

Fabianx’s picture

So - I read this quickly when it was assigned to me and it looked good.

The biggest problem we faced in the race condition is that the template _could_ be loaded, but then the class was not there.

The problem with the approach here is that we don't have the class name available, so we can't do the if !(class_exists($class, FALSE) dance ...

That makes this a little more problematic, than I would like to and I agree that we likely need to fix this in PHPStorage itself.

znerol’s picture

The biggest problem we faced in the race condition is that the template _could_ be loaded, but then the class was not there.

What exactly is the root of this problem? Storage load() must not return TRUE unless the file has been successfully included. I frankly do not expect that this is an issue with the current implementation of php storage.

On the other hand the upstream code clearly contains a race condition and we used to work around it by overriding loadTemplate(). In my opinion we should file an upstream issue and stick to the override for now.

star-szr’s picture

Assigned: Unassigned » star-szr

Yes, just spoke with @znerol at the extended sprints in Barcelona. The critical is just make this work for Twig 1.x, and making our stuff 2.x compatible would be a major: #2568181: [META] Update to Twig 2.x in Drupal 9

Working on a new patch now.

znerol’s picture

On the other hand the upstream code clearly contains a race condition and we used to work around it by overriding loadTemplate(). In my opinion we should file an upstream issue and stick to the override for now.

Filed: https://github.com/twigphp/Twig/issues/1836

star-szr’s picture

Something along these lines.

Interdiff is from #58 because I reverted all of #63.

Status: Needs review » Needs work

The last submitted patch, 69: upgrade_to_twig_1_22-2568171-68.patch, failed testing.

The last submitted patch, 69: upgrade_to_twig_1_22-2568171-68.patch, failed testing.

star-szr’s picture

I think the new Null cache class (upstream) in both 1.22 and 1.x-dev doesn't protect against redeclaring classes. May need to add our own null cache for the time being.

star-szr’s picture

Assigned: star-szr » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
FileSize
59.9 KB
798 bytes

Thanks to @znerol for the pair programming, found there is an extra layer here that is no longer needed.

Historically (all the way back to #1696786: Integrate Twig into core: Implementation issue) before there was such thing as cache classes in Twig we didn't have has() so we did this inside our loadTemplate() method:

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

Now having this extra load/write/load is just causing problems because when caching is disabled (therefore the upstream Null cache class is being used):

  • has() always returns false
  • load() never returns anything

Therefore with the patch in #69 when Twig cache is disabled (such as during install) the Null cache tries to write the same class multiple times even if the template is only called once.


Attached removes the now unnecessary hunk. If a race condition occurs we compile and eval. Really the only changes from the upstream loadTemplate are:

  • Removal of BC handling
  • Addition of race condition handling
znerol’s picture

  1. +++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
    @@ -0,0 +1,125 @@
    +  /**
    +   * Template source cache for when the template cannot be loaded from storage.
    +   *
    +   * @var array
    +   */
    +  protected $templateSourceCache = [];
    ...
    +    // Store the compiled template in case loading from PhpStorage fails.
    +    $this->templateSourceCache[$key] = $content;
    

    Looks like dead code. It seems that `load()` ignores `templateSourceCache`.

  2. +++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
    @@ -0,0 +1,125 @@
    +    // We override the cache filename in order to avoid issues with not using
    +    // shared filesystems. The Twig templates for example rely on available Twig
    +    // extensions, so we use the Twig extension hash which varies by extensions
    +    // and their mtime.
    

    I realize that this comment is from the old code, it was a bit unclear already there. However, now it is technically incorrect. We do not override anything here.

    Maybe just delete the comment here and instead add a description to the class which explains the reason for the twig extension hash.

    Perhaps something like this:

    This class is designed to work on setups with multiple webheads using a local filesystem for the twig cache. When generating the cache key, a hash value depending on the enabled extensions is included. This prevents stale templates from being reused when twig extensions are enabled or disabled.
    @see \Drupal\Core\DependencyInjection\Compiler\TwigExtensionPass

  3. +++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
    @@ -0,0 +1,125 @@
    +    return $this->storage()->load($key);
    

    The `Twig_CacheInterface` does not specify a return value and the twig implementations never return anything, so we shouldn't either.

star-szr’s picture

Status: Needs review » Needs work

Thanks! Will work on those tomorrow.

cilefen’s picture

Status: Needs work » Needs review
FileSize
2.31 KB
59.66 KB

this is the suggestions from #74.

star-szr’s picture

Awesome thanks @cilefen!

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
    @@ -0,0 +1,118 @@
    +   */
    +  protected $cache = NULL;
    +
    

    Its not really optional, isn't it? The NULL indicates that somehow.

  2. +++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
    @@ -0,0 +1,118 @@
    +   *
    +   * @var \Drupal\Core\PhpStorage\PhpStorageFactory
    ...
    +   *
    +   * @return \Drupal\Component\PhpStorage\PhpStorageInterface
    ...
    +    }
    +    return $this->storage;
    

    Let's typehint the interface consistently, I'd say.

  3. +++ b/core/lib/Drupal/Core/Template/TwigPhpStorageCache.php
    @@ -0,0 +1,118 @@
    +    else {
    +      return REQUEST_TIME;
    +    }
    

    Just curious whether we think this is the right thing. In case the cache entry is gone, do we maybe better want to use 0, so things are invalidated?

star-szr’s picture

I think 0 does make more sense if we don't know the state of the cache. This addresses #78.

cpj’s picture

Status: Needs review » Reviewed & tested by the community

I've reviewed the changes, which make sense and look fine to me. I've also done some local testing, including enabling / disabling parts of the new TwigPhpStorageCache, and it seems to behave as expected. Therefore I'm marking it as RTBC, but perhaps others also want to test ?

znerol’s picture

It seems to me that 1.22.0 now takes into account the loaded extensions when computing the class hash (see also PR 1824). So I think we might ditch twig_extension_hash and the compiler pass. Maybe in a follow-up?

star-szr’s picture

Assigned: Unassigned » Fabianx
Issue summary: View changes
Related issues: +#2568181: [META] Update to Twig 2.x in Drupal 9

Thank you @cpj, assigning to @Fabianx because I'd like him to have a look before we commit.

I created a follow-up to look at removing TwigExtensionPass: #2571613: Consider removing TwigExtensionPass in favor of Twig's newly built-in handling of changed extensions

This issue and patch is just so we're safe to upgrade the 1.x path, and 2.x compatibility will be handled in #2568181: [META] Update to Twig 2.x in Drupal 9. Updated the issue summary with that as well.

Fabianx’s picture

Assigned: Fabianx » Unassigned

RTBC + 1 - This looks much nicer as an overall solution and also has less invasive changes.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 8b60468 and pushed to 8.0.x. Thanks!

  • alexpott committed 8b60468 on 8.0.x
    Issue #2568171 by Cottser, dawehner, hussainweb, Wim Leers, cilefen,...
star-szr’s picture

Status: Fixed » Closed (fixed)

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