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] Get ready to upgrade to Twig 2.x.

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

Files: 
CommentFileSizeAuthor
#79 interdiff.txt850 bytesCottser
#79 upgrade_to_twig_1_22-2568171-79.patch59.65 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,576 pass(es). View
#76 upgrade_to_twig_1_22-2568171-76.patch59.66 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,542 pass(es). View
#76 interdiff-2568171.txt2.31 KBcilefen
#73 interdiff.txt798 bytesCottser
#73 upgrade_to_twig_1_22-2568171-73.patch59.9 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,262 pass(es). View
#69 interdiff.txt8.77 KBCottser
#69 upgrade_to_twig_1_22-2568171-68.patch60.05 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,615 pass(es), 300 fail(s), and 203 exception(s). View
#63 interdiff.txt3.46 KBCottser
#63 upgrade_to_twig_1_22-2568171-63.patch61.47 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,248 pass(es). View
#58 interdiff.txt5.57 KBCottser
#58 upgrade_to_twig_1_22-2568171-58.patch61.12 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 74,675 pass(es), 16,380 fail(s), and 12,137 exception(s). View
#51 interdiff.txt750 bytesCottser
#51 upgrade_to_twig_1_22-2568171-51.patch63.79 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,143 pass(es). View
#50 interdiff.txt907 bytesWim Leers
#50 upgrade_to_twig_1_22-2568171-50.patch63.5 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,137 pass(es). View
#47 interdiff-no-vendor.txt2.28 KBCottser
#47 interdiff.txt7.99 KBCottser
#47 upgrade_to_twig_1_22-2568171-47.patch62.73 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,136 pass(es), 1 fail(s), and 0 exception(s). View
#44 interdiff.txt2.31 KBdawehner
#44 2568171-44.patch61.52 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 62,327 pass(es), 30,083 fail(s), and 8,148 exception(s). View
#40 interdiff.txt580 bytesCottser
#40 upgrade_to_twig_1_22-2568171-40.patch61.65 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,125 pass(es). View
#32 interdiff.txt677 bytesCottser
#32 upgrade_to_twig_1_22-2568171-31.patch61.67 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean. View
#29 interdiff.txt676 bytesCottser
#29 upgrade_to_twig_1_22-2568171-29.patch61.67 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean. View
#23 interdiff.txt2.69 KBdawehner
#23 2568171-23.patch61.67 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git]. View
#19 upgrade_to_twig_1_22_0-2568171-19.patch64.14 KBhussainweb
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,116 pass(es). View
#19 interdiff-12-19.txt8.35 KBhussainweb
#14 testing-patch-2568171-twig-1-20-14.patch126.18 KBhussainweb
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 64,342 pass(es), 17,449 fail(s), and 7,329 exception(s). View
#12 interdiff.txt6.15 KBCottser
#12 upgrade_to_twig_1_22_0-2568171-12.patch63.25 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,010 pass(es). View
#9 upgrade_to_twig_1_22_0-2568171-9.patch58.42 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,896 pass(es). View
#4 interdiff.txt935 bytesCottser
#4 upgrade_to_twig_1_22_0-2568171-4.patch59.28 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,899 pass(es). View
#2 upgrade_to_twig_1_22_0-2568171-2.patch59.95 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,897 pass(es). View

Comments

Cottser created an issue. See original summary.

Cottser’s picture

Status: Active » Needs review
FileSize
59.95 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,897 pass(es). View
Cottser’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.

Cottser’s picture

FileSize
59.28 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,899 pass(es). View
935 bytes

This removes that change so it doesn't conflict with #2568181: [meta] Get ready to upgrade to Twig 2.x. Or at least doesn't conflict as much…

Cottser’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?

Cottser’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

Cottser’s picture

Status: Needs work » Needs review
FileSize
58.42 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,896 pass(es). View

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

Cottser’s picture

Status: Needs review » Needs work
Cottser’s picture

Picking this up again.

Cottser’s picture

Status: Needs work » Needs review
FileSize
63.25 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,010 pass(es). View
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.

Cottser’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

FileSize
126.18 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 64,342 pass(es), 17,449 fail(s), and 7,329 exception(s). View
+++ 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
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,116 pass(es). View

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

FileSize
61.67 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git]. View
2.69 KB

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.

Cottser’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?

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

Cottser’s picture

Status: Needs work » Needs review
FileSize
61.67 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean. View
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 :)

Cottser’s picture

Status: Needs work » Needs review
FileSize
61.67 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean. View
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

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

Cottser’s picture

Status: Needs work » Needs review
FileSize
61.65 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,125 pass(es). View
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()).

Cottser’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
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 62,327 pass(es), 30,083 fail(s), and 8,148 exception(s). View
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.

Cottser’s picture

Status: Needs work » Needs review
FileSize
62.73 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,136 pass(es), 1 fail(s), and 0 exception(s). View
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
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,137 pass(es). View
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.

Cottser’s picture

FileSize
63.79 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,143 pass(es). View
750 bytes

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.

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

Cottser’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?

Cottser’s picture

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

Cottser’s picture

Assigned: Fabianx » Unassigned
FileSize
61.12 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 74,675 pass(es), 16,380 fail(s), and 12,137 exception(s). View
5.57 KB

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.

Cottser’s picture

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

Cottser’s picture

Status: Needs work » Needs review
FileSize
61.47 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,248 pass(es). View
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.

Cottser’s picture

Assigned: Unassigned » Cottser

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] Get ready to upgrade to Twig 2.x

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

Cottser’s picture

FileSize
60.05 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,615 pass(es), 300 fail(s), and 203 exception(s). View
8.77 KB

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.

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

Cottser’s picture

Assigned: Cottser » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
FileSize
59.9 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,262 pass(es). View
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.

Cottser’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
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,542 pass(es). View

this is the suggestions from #74.

Cottser’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?

Cottser’s picture

FileSize
59.65 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,576 pass(es). View
850 bytes

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?

Cottser’s picture

Assigned: Unassigned » Fabianx
Issue summary: View changes
Related issues: +#2568181: [meta] Get ready to upgrade to Twig 2.x

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] Get ready to upgrade to Twig 2.x. 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,...
Cottser’s picture

Status: Fixed » Closed (fixed)

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