Problem/Motivation

TwigEnvironment is sending invalid filenames to the storage backends when using inline templates.

In this method:


namespace Drupal\Core\Template;

// [...]

/**
   * Implements Twig_Environment::loadTemplate().
   *
   * We need to overwrite this function to integrate with drupal_php_storage().
   *
   * This is a straight copy from loadTemplate() changed to use
   * drupal_php_storage().
   *
   * @param string $name
   *   The template name or the string which should be rendered as template.
   * @param int $index
   *   The index if it is an embedded template.
   *
   * @return \Twig_TemplateInterface
   *   A template instance representing the given template name.
   *
   * @throws \Twig_Error_Loader
   *   When the template cannot be found.
   * @throws \Twig_Error_Syntax
   *   When an error occurred during compilation.
   */
  public function loadTemplate($name, $index = NULL) {

The $name parameter can either be a path to a twig template, or an inline template.

In that same method, the $cache_filename used to store the compiled template is generated like this:

$cache_filename = $this->getCacheFilename($name);

And getCacheFilename($name) is expecting $name to be a path, not an inline template:

  public function getCacheFilename($name) {
    // [...]

    if (!$this->cache) {
      return FALSE;
    }

    $class = substr($this->getTemplateClass($name), strlen($this->templateClassPrefix));

    // The first part is what is invalidated.
    return $this->templateCacheFilenamePrefix . '_' . basename($name) . '_' . $class;
  }

This can result in generating a cache_filename with invalid characters such as this one:

eab26728_{# inline_template_start #}{{ items | safe_join(separator) }}_6a27047eeba00af1c60ff2f4c59e9e614903a38f7c90d62ed5441a2edb82b31b

On POSIX compatible systems I believe this is not an issue, but those characters are not allowed on other platforms. Anyways, having such a filename does not look goooood.

Later on, when MTimeProtectedFastFileStorage tries to store the data using the weird filename you get an error:

rename(sites/default/files/php/twig/.348cbdff96,sites/default/files/php/twig/eab26728_{# inline_template_start #}{{ items | safe_join(separator) }}_6a27047eeba00af1c60ff2f4c59e9e614903a38f7c90d62ed5441a2edb82b31b/681514bc3d30ec66b9f65af2fb67575dd13c2ac9e54f4e046da77e7e526d2753.php): The filename, directory name, or volume label syntax is incorrect. (code: 123)

One of the templates it is chocking on is this one:

{# inline_template_start #}{{ items | safe_join(separator) }}

Proposed resolution

Fix getCacheFilename($name) to account for $name having the possiblity of being a path or an inline template.

Remaining tasks

--

User interface changes

--

API changes

--

Data model changes

--

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because twig template caching is failing for inline templates.
Issue priority Major because twig caching is not working and warnings are issued upon failure flooding the system logs.
Files: 
CommentFileSizeAuthor
#59 interdiff.txt712 byteseffulgentsia
#39 interdiff.txt1.82 KBCottser
#38 twigenvironment_is-2558885-38.patch3.09 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,650 pass(es). View
#38 twigenvironment_is-2558885-38-testonly.patch2.1 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,714 pass(es), 1 fail(s), and 2 exception(s). View
#30 interdiff.txt2.16 KBCottser
#30 twigenvironment_is-2558885-31.patch2 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,725 pass(es). View
#30 twigenvironment_is-2558885-31-testonly.patch1.01 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,226 pass(es), 317 fail(s), and 22 exception(s). View
#21 interdiff.txt1.68 KBCottser
#21 twigenvironment_is-2558885-21.patch1.79 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,252 pass(es). View
#20 interdiff.txt1.49 KBCottser
#20 twigenvironment_is-2558885-20.patch1.79 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,252 pass(es). View
#16 2558885.15.patch1.77 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,133 pass(es). View
#11 2558885-11.patch1.74 KBjhedstrom
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,830 pass(es). View
#11 2558885-11-TEST-ONLY.patch1 KBjhedstrom
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,825 pass(es), 1 fail(s), and 0 exception(s). View

Comments

david_garcia created an issue. See original summary.

david_garcia’s picture

Issue summary: View changes
david_garcia’s picture

Title: Twig is unable to store inline templates because it sends invalid filenames to MTimeProtectedFastFileStorage » TwigEnvironment is unable to cache inline templates because it sends invalid filenames to MTimeProtectedFastFileStorage
david_garcia’s picture

Issue summary: View changes
david_garcia’s picture

Issue summary: View changes
david_garcia’s picture

Status: Active » Needs review
FileSize
758 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,559 pass(es). View
othermachines’s picture

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.

Cottser’s picture

Component: render system » theme system
Issue tags: +Twig

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.

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
Related issues: +#2544932: Twig should not rely on loading php from shared file system

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

jhedstrom’s picture

Assigned: Unassigned » jhedstrom

I'll work on a test.

jhedstrom’s picture

Assigned: jhedstrom » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1 KB
1 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,825 pass(es), 1 fail(s), and 0 exception(s). View
1.74 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,830 pass(es). View

This adds a test for the expected cached filename.

The last submitted patch, 11: 2558885-11-TEST-ONLY.patch, failed testing.

The last submitted patch, 11: 2558885-11-TEST-ONLY.patch, failed testing.

Cottser’s picture

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

dawehner’s picture

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.

alexpott’s picture

Related issues: +#2568171: Upgrade to Twig 1.22 and implement our own cache class
FileSize
1.77 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,133 pass(es). View

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.

dawehner’s picture

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

johnv’s picture

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

Status: Needs review » Needs work
Cottser’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing
FileSize
1.79 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,252 pass(es). View
1.49 KB

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

Cottser’s picture

FileSize
1.79 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,252 pass(es). View
1.68 KB

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

Interdiff is from #16.

oriol_e9g’s picture

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

Status: Needs review » Needs work

The last submitted patch, 21: twigenvironment_is-2558885-21.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review

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.

dawehner’s picture

+++ 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 "{ #}".

johnv’s picture

Status: Needs review » Needs work

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

johnv’s picture

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

Cottser’s picture

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.

Cottser’s picture

Status: Needs work » Needs review
FileSize
1.01 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,226 pass(es), 317 fail(s), and 22 exception(s). View
2 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,725 pass(es). View
2.16 KB

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

The last submitted patch, 30: twigenvironment_is-2558885-31-testonly.patch, failed testing.

johnv’s picture

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

The last submitted patch, 30: twigenvironment_is-2558885-31-testonly.patch, failed testing.

Cottser’s picture

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

alexpott’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

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

Cottser’s picture

Status: Needs work » Needs review

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

dawehner’s picture

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

Cottser’s picture

Issue tags: -Needs manual testing, -Needs tests
FileSize
2.1 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,714 pass(es), 1 fail(s), and 2 exception(s). View
3.09 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,650 pass(es). View

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

Cottser’s picture

FileSize
1.82 KB

Missing interdiff.

The last submitted patch, 38: twigenvironment_is-2558885-38-testonly.patch, failed testing.

The last submitted patch, 38: twigenvironment_is-2558885-38-testonly.patch, failed testing.

mikeker’s picture

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

MattA’s picture

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

david_garcia’s picture

This might be overkill but...

The original concern over file_exists() performance related.

Using strpos means scanning through the whole template when this is not an inline template.

What about something like this?

    if (strcasecmp(substr($name, 0, 27), '{# inline_template_start #}') === 0) {
      // $name is an inline template, and can have characters that are not valid
      // for a filename. $hash is unique for each inline template so we just use
      // the generic name 'inline-template' here.
      $name = 'inline-template';
    }
    else {
      $name = basename($name);
    }
Cottser’s picture

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.

david_garcia’s picture

Status: Needs review » Reviewed & tested by the community

That's true, only filenames will make it here. Looks good to go.

Cottser’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 38: twigenvironment_is-2558885-38.patch, failed testing.

Cottser’s picture

Status: Needs work » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 38: twigenvironment_is-2558885-38.patch, failed testing.

Cottser’s picture

Status: Needs work » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 38: twigenvironment_is-2558885-38.patch, failed testing.

The last submitted patch, 38: twigenvironment_is-2558885-38.patch, failed testing.

  • effulgentsia committed 7ed830c on 8.0.x
    Issue #2558885 by Cottser, jhedstrom, david_garcia, alexpott:...
effulgentsia’s picture

Status: Needs work » Fixed
FileSize
712 bytes

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

Status: Fixed » Closed (fixed)

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

hatuhay’s picture

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.

hatuhay’s picture

Version: 8.0.x-dev » 8.0.0-rc2
Status: Closed (fixed) » Active
jhedstrom’s picture

Version: 8.0.0-rc2 » 8.0.x-dev
Status: Active » Closed (fixed)

#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?

hass’s picture

Where is the followup? I see the same bug.

h_razack’s picture

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

Cottser’s picture

drupalfan2’s picture

This problem still seems to be part oft drupal 8.0.1.

What can I do?

mikeker’s picture

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

emena’s picture

I still have the problem in 8.0.2

mikeker’s picture

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