Twig cache bug, potentially harmful

[alpha-and-omega]

According to documentation:

Twig_Loader_Filesystem support absolute and relative paths. Using relative paths is preferred as it makes the cache keys independent of the project root directory (for instance, it allows warming the cache from a build server where the directory might be different from the one used on production servers)

This implies that absolute paths should create different cache files. But right now absolute paths do not work as intended. They share same cached files, this can be harmful in production environment.

Twig version: v1.33.2
composer require twig/twig

<?php
require_once (__DIR__."/vendor/autoload.php");
$path = getcwd()."/tpl";
echo "path=$path\n";
$twig_loader = new Twig_Loader_Filesystem($path);
$twig = new Twig_Environment($twig_loader, array(
                        'cache' => '/tmp/twig-cache',
                        'auto_reload' => true,
                        ));

echo $twig->render('page.html.twig');
?>

Just create two versions of this file in different folders and create different tpl/page.html.twig files.
Once cache created, both php will output same cached result. Opcode disabled.

[alpha-and-omega]

After searching source code I can conclude, that this behavour is not even coded. So documentation is incorrect or this part of code was removed.

My quick hack to solve a problem without changing twig:

$twig_loader = new Twig_Loader_Filesystem($path,"/invalid_rootpath");

But this should be fixed in twig.

[bayne]

I believe I was also running into this issue, received these errors after updating to v1.34.0:

PHP Fatal error:  Cannot redeclare class Twig_Environment in /var/local/releases/20170605172933/vendor/twig/twig/lib/Twig/Environment.php on line 18

Rolling back to v1.33.2 fixes it

[Tobion]

This really seems to be a problem.
What happens is that the common root path is automatically removed in https://github.com/twigphp/Twig/blob/2.x/lib/Twig/Loader/Filesystem.php#L145. So it does not matter if you pass a relative path or an absolute path. The cache key for both templates (with different roots) will just be tpl/page.html.twig as getcwd() is removed from it.

IMO the removal of the root dir should only happen if the original path was not absolute. So it needs to depend on the path being used. Doesn't seem that trivial to do in the current code though.

[stof]

I think the issue here is that you are trying to use 2 different environments sharing the same cache folder.
The only way to make this work would be to ensure that they will never use the same template cache key when using the same template name (as the template name will resolve to a different file). Solving this use case requires using the template absolute path again as cache key, which forbids doing the warmup on a different machine and copying the cache.
I don't think it is worth it IMO. Just use separate cache folders if you have several environments with different configurations

[Tobion]

I'd say it should be as the documentation says. If you use relative paths, then the cache key does not use the absolute path. But if you use absolute paths, the cache key should use the absolute path. The current implementation does not respect if the original path was relative or not, making implementation != doc.

[stof]

@Tobion I don't understand what you mean when saying if you use absolute paths. Using them where ?

[Tobion]

new Twig_Loader_Filesystem('tpl', '/myproject/');

vs

new Twig_Loader_Filesystem('/myproject/tpl');

The first one should use relative cache keys for all templates in/myproject/tpl and the second one shouldn't. This also works most of the time. But in the bug report given this breaks because /myproject/ === getcwd() (the default rootDir) and thus removing the rootDir from the original absolute path.

[Tobion]

Also the bundle configuration doc does not seem to document this the way it seems intended.

http://symfony.com/doc/current/reference/configuration/twig.html#paths

uses

twig:
    # ...
    paths:
        '%kernel.project_dir%/vendor/acme/foo-bar/templates': ~

when the following would be enough and relative to the project dir (https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/TwigBundle/Resources/config/templating.xml#L12)

twig:
    # ...
    paths:
        'vendor/acme/foo-bar/templates': ~

[stof]

Well, passing absolute paths for the loading paths is still supported (and works with older versions of Twig too). So I don't think you sentence does not seem to document this the way it seems intended is true. Paths passed to the loader can be absolute too.

[Tobion]

Yes. But if you use absolute paths, the cache key should also depend on the absolute path!

Only if you use relative paths this should not be the case.

Using relative paths is preferred as it makes the cache keys independent of the project root directory

Currently it does not matter if the path is relative or not as the loader will strip absolute paths that equal the root path magically.

[stof]

@Tobion if you are using the second argument of the loader, you will also face cache corruption issues if you have several twig environments using the same cache folder for similar template relative paths.

[Tobion]

This ticket is about the other case: if you don't care about the second argument and want to use absolute paths, then you can still get cache conflicts.

[jvasseur]

I personally think the current behavior is the correct one. If you use something like new Twig_Loader_Filesystem(__DIR__ . '/templates'); the loader will receive an absolute path but you still want the cache keys to work independently of where your application is located in the filesystem.

The documentation should probably be updated to reflect the current behavior instead.

[alpha-and-omega]

I'll explain. Right now, these two lines can use different templates but share same cached files:

Twig_Loader_Filesystem("/www/development/templates");
Twig_Loader_Filesystem("/www/production/templates");

This contradicts documentation and on this planet is obviously unexpected behavior.
Rootpath (second argument) is not an issue, there is nothing wrong with it, forget it.

[stof]

@alpha-and-omega the Twig_Environment gets cache names as input, and and has a cache folder.
If you have several environments with different configurations, you must either ensure that they don't share the same cache folder, or ensure that they will never reuse the same template name.
Sharing the same cache folder for 2 environments with different configuration is opening the door to cache corruption (as the cache will be written by the other environment too).
Instead of using /tmp/twig-cache as cache folder, create a different cache folder for each project.

[alpha-and-omega]

@stof yes, I understand that your suggestion is to change the documentation to use different cache folders. This approach is inconvenient and can lead to other path-related errors. Why force one solution instead of giving a choice ?

[stof]

@alpha-and-omega changing the filesystem loader again will not solve the cache corruption issue when reusing the same cache folder for multiple environments with different configurations. It will only remove corruption in one case, at the cost of loosing a feature.

[alpha-and-omega]

@stof are you agree that this feature is flawed ?

[stof]

which feature are you talking about when saying "this feature" ?

What I agree on is that reusing the same cache folder for multiple environments with different configurations will never be reliable (even if you use the same source file, the output of the compilation may still need to be different for them)

[fabpot]

Closing as there is no more activity here.