TwigEnvironment is unable to cache inline templates because it sends invalid filenames to MTimeProtectedFastFileStorage

I came across this error on a fresh test install in a WAMP environment:

Warning: rename(sites/sub2.mysite.com/files/php/twig/.0f1a6329b1,sites/sub2.mysite.com/files/php/twig/c9d064de_{# inline_template_start #}{{ items | safe_join(separator) }}_6a27047eeba00af1c60ff2f4c59e9e614903a38f7c90d62ed5441a2edb82b31b/af6b9e719be73471b4c423e44df1bdaa8520aa7777452c9215b053536ac7f833.php): The filename, directory name, or volume label syntax is incorrect. (code: 123) in Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage->save() (line 91 of C:\wamp\www\d8\core\lib\Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage.php).

Applied #6 patch and reinstalled with no error.

Thanks, on my phone now but I think this was changed recently in another issue. I will try to link it here a bit later.

The method was overridden in #2544932: Twig should not rely on loading php from shared file system. This needs automated test coverage.

I'll work on a test.

This adds a test for the expected cached filename.

file_exists() might be heavy here from a performance perspective because as far as I know getCacheFilename() will be called quite a bit.

Other than that, getCacheFilename() is deprecated and will be removed in Twig 2.0, but it looks like we can contain this logic in our custom Twig cache class (#2426563-95: Ignore: Patch testing issue contains the cache class and some other 2.0 fixes).

Did someone tried out the patch in #2568171: Upgrade to Twig 1.22 and implement our own cache class to see whether its fixed there already? If it is, we should just need the test coverage which is part of this particular patch.

We can be certain we're dealing with an inline template and therefore no file exists.

Also #2568171: Upgrade to Twig 1.22 and implement our own cache class might solve this.

+++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
@@ -127,6 +127,11 @@ public function getCacheFilename($name) {
+      $name = 'inline-template-' . sha1($name);

+++ b/core/modules/system/src/Tests/Theme/TwigEnvironmentTest.php
@@ -65,6 +65,13 @@ public function testInlineTemplate() {
+    $expected = $hash . '_inline-template-' . sha1($name) . '_' . $class;

IMHO we should use the same hash as in other places which is sha256 using the hash() function in php.

I implemented #16 on my WAMP environment with 8.beta15 (since it was much shorter then #2568171).

The 'rename' warning still appears twice: The mkdir perhaps only appears upon install.
The message DOES appear on /admin/modules, and it does NOT appear on /admin/config .

Warning: rename(sites/d8b150917.dd/files/php/twig/.83cc9b8281,sites/d8b150917.dd/files/php/twig/7f30d3f0_inline-template-....php): No such file or directory in Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage->save() (line 91 of core\lib\Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage.php).
Warning: rename(sites/d8b150917.dd/files/php/twig/.8ca07c4a5d,sites/d8b150917.dd/files/php/twig/7f30d3f0_inline-template-....php): No such file or directory in Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage->save() (line 91 of core\lib\Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage.php).

Just making the hash change. This needs more manual testing on Windows I think, thanks for testing @johnv!

The comment makes more sense above the line it's talking about.

Interdiff is from #16.

For a performance perspective it's faster crc32 t'han sha256. Actually (i think) we are using crc32 in core in other places.

Some installer fails from the looks of it.

Twig upstream uses sha256, so I think we should just stick with that. https://github.com/twigphp/Twig/blob/9df64cf582ea3a4c33da3b9ef2ffe82f061...

We'll need to refactor this before/after #2568171: Upgrade to Twig 1.22 and implement our own cache class anyway and it will end up in the generateKey method of the cache class.

+++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
@@ -126,6 +126,11 @@ public function getCacheFilename($name) {
+    if (strpos($name, '{# inline_template_start #}') !== FALSE) {
+      // $name is an inline template, and can have characters that are not valid
+      // for a filename.
+      $name = 'inline-template-' . hash('sha256', $name);
+    }

Could we not use a more generic regex and remove just the following chars "{ #}".

Patch #21 is better.
Both pages /admin/modules and /admin/config have no messages anymore.
But pressing [Clear all caches] on /admin/config/development/performance still generatess:
Warning: rename(sites/d8b150917.dd/files/php/twig/.373c5f4b30,sites/d8b150917.dd/files/php/twig/7eee24fc_block--system-messages-block.html.twig_NNN.php): No such file or directory in ...\core\lib\Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage.php on line 91

#2568171: Upgrade to Twig 1.22 and implement our own cache class is now committed. See #15, #16, #25.
Is this patch still needed?

It needs more testing, I'm thinking we still need to make a change to fix this but it would likely be in \Drupal\Core\Template\TwigPhpStorageCache now.

This is what I would propose. The interdiff is after a reroll.

@Cottser, Sorry, cannot test this ATM , since I am working on beta15, en the new patch requires an update of the environment.

@johnv thanks, I don't think at this point it makes sense to create a patch for beta15.

It would be really nice to get this in, I'm not sure if it actually needs manual testing but it would be great to get a confirmation from someone who's experiencing this problem.

For now it would be great to get the code reviewed.

@Cottser I think this should be really easy to write an automated test for since I broke it recently.

@alexpott there are tests, do you mean add more tests? :)

Yeah I think @alexpott means more like an integration test.

This should be better because it will actually throw an exception from the storage. Thank you @alexpott on IRC for clarifying!

Missing interdiff.

Verified that #38 fixes the errors on Windows. Thanks, @Cottser!

I've also been using it for a few days on a dev box, and can confirm that it fixes the PHP warning. I don't know enough about the Twig caching system to RTBC it though. :(

When it's not an inline template $name is the name of the template, like "node.html.twig", or in a worse case something like "@node/node.html.twig". If we consider further optimizing this it should be in a follow-up IMO, this bug is a serious issue for Windows and other users.

If we think people will have really long inline templates then it's relevant but should still be follow up I think. Thanks!

Unrelated failure in migrate, @mdrummond mentioned earlier he'd seen that before so I am guessing it's a random fail we have.

Same fail, Drupal\user\Tests\Migrate\MigrateUserProfileEntityDisplayTest. @larowlan on IRC said that this is a known random segfault.

Pushed to 8.0.x, with this additional interdiff applied on commit.

C:\Bitnami\drupal-8.0.0.rc2-0\apps\drupal\htdocs>drush ws
Warning: rename(sites/default/files/php/twig/.6e35038013,sites/default/files/php/twig/3b9a7135_block--system-messages-block.html.twig_1b89fea44fd124de67ee5957081934a6aa2d86b3295c1a8dd2211f

Drupal 8.0.0.rc2, under Windows 10 Bitnamis framework.
Fresh installation worked OK, start installing modules and suddenly Bam!! no more Drupal 8.
Checked that patch is already applied on this version.
Rebuild caches, same error.
Same error for block--system-branding-block.
Both files are generated on sites/default/files/php/twig/ but cannot be renamed to its own folder.

#61 doesn't appear to be the same issue as this (this issue was about file names, the file names in #61 look valid). @hatuhay could you file a new issue with steps to reproduce the issue if available?

Where is the followup? I see the same bug.

I seem to have the same thing suddenly happening with 8.0.0 (windows 10)

Warning: rename(sites/drupal8-acquia.dd/files/php/twig/.4fc79a9d06,sites/drupal8-acquia.dd/files/php/twig/7754c0f9_status-messages.html.twig_68d30c89960f316b7d3c7db2b571b5a73aaf8c7e32649f091d44943a5c3ce5ba/d89b2148112336754491be6bd185430cd0aafadc74263ffc86f74c8430598d2c.php): No such file or directory in Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage->save() (line 91 of core\lib\Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage.php).

#2606772: Long Twig cache directories can cause failures on some filesystems

This problem still seems to be part oft drupal 8.0.1.

What can I do?

See the issue referenced above. What you are seeing is not this issue, but one that gives a similar error.

I still have the problem in 8.0.2

@emena: You'll need to give us more information for us to be able to help you.

Also, if you're on Windows, you're likely seeing this issue: #2606772: Long Twig cache directories can cause failures on some filesystems, which results in a similar error but has a different cause.