Twig: BC break regarding relative paths in 1.25 due to realpath changes (#2127)

[Nicofuma]

The 1.25 version does not allow relative loading paths ($this->paths) in the filesystem loader anymore.

The new normalizePath() method is more strict than realpath() as it does not allow relative paths (any extra ../ will be ignored). It means that if you have a FilesystemLoader configured with a relative directory in the paths list, the result of findTemplate() will be different than in the 1.24 version.

The result will also be wrong because the is_file() check (https://github.com/twigphp/Twig/blob/1.x/lib/Twig/Loader/Filesystem.php#L200) will succeed but the returned path won't lead to an existing file, as a result a warning will be thrown by getSource().

Reproducer (expecting true with both 1.24 and 1.25):

<?php

include 'vendor/autoload.php';

$parts = explode('/', __FILE__);
$filename = array_pop($parts);
$dirname = array_pop($parts);

$loader = new Twig_Loader_Filesystem('../'.$dirname);
$r = new ReflectionMethod($loader, 'findTemplate');
$r->setAccessible(true);
var_dump(__FILE__ === $r->invoke($loader, $filename));

[fabpot]

The reproducer is not really interesting as it is just the result of the change. Where is it a problem for you?

[Nicofuma]

Actually the goal of the reproducer was to show the difference of behavior between 1.24 and 1.25 but it's not interesting actually.
The issue is that if you use a relative path (../my-dir/ per example), in 1.25 findTemplate('test') will return something like my-dir/test while it should return ../my-dir/test. Another related issue is that the is_file() check is done with $path.'/'.$shortname (../my-dir/test in my example). It means that the is_file check will pass but the path returned by the find_template function does not lead to any file and the caller (getSouce) will raise a warning when trying to get the file content.

[boutetnico]

Also got affected by this BC after updating from Twig 1.24.1 to 1.25.
Fixed it by replacing my relative path '../templates' by __DIR__ . '/../templates'.

[stof]

AFAIK, Twig always documented that it expects an absolute path in the loader. (Btw, relative paths are fragile as they are relative to the current working dir, and it is easy to run the same codebase from different directories)

[kimpepper]

I ran into this issue when loading twig files from a symfony console phar.

[UlrichKu]

2149 is 70% duplicate.

It adds information/problems with symlinks which can point to arbitrary other locations and thus won't work with the shortening of "normalizePath()".

[enzolutions]

Inside the Drupal Console project we have the same issue inside a phar, as @kimpepper reported.

We had to return to _1.23.1_ and all works as expected.

[marc1706]

@stof I don't see any mention of relative paths or absolute paths in the loader (see e.g. http://twig.sensiolabs.org/doc/api.html#twig-loader-filesystem). Only the cache seems to explicitly ask for absolute paths.
I do understand your argument with possible issues caused by improperly using relative paths. This does however not change the fact that the documentation does not state the need for an absolute path and until now twig has supported relative paths for the filesystem loader. Therefore this is a clear BC break that I think should be addressed.
If you want to enforce using absolute paths then I think this would fit better with a clear BC break in a major version (i.e. twig 2.x).

[jayesbe]

I don't know if this is related.. but since the update to 1.25, I am now getting this as soon as my app is hit

Fatal error: Call to undefined method Twig_Node_Module::setFilename() in /vagrant/vendor/twig/twig/lib/Twig/Compiler.php on line 88

[omerta]

I have the same problem.
I save my templates in two different directories.
I have this directory tree:

├── module
│   ├── controllerA.php
│   └── templates
│       └── form.html
├── shared
│   └── templates
│       └── base.html

And my module/controllerA.php is:

<?php
require_once '../twig/vendor/autoload.php';
$loader = new Twig_Loader_Filesystem(array('templates/','../shared/templates/'));

$twig = new Twig_Environment($loader);
$template = $twig->loadTemplate('form.html');

echo $template->render(array('nombre' => 'Gottfried','apellido' => 'Leibniz'));
 ?>

My shared/templates/base.html is:

<h1>Base</h1>

{% block body %}{% endblock %}

Finally my module/template/form.html is:

{% extends 'base.html' %}

{% block body %}
  <h2>Hola Mundo</h2>
  <p>{{ nombre }} y {{ apellido }}</p>
{% endblock %}

The error is:

PHP Warning:  file_get_contents(shared/templates/base.html): failed to open stream: No such file or directory in /var/www/test_twig/twig/vendor/twig/twig/lib/Twig/Loader/Filesystem.php on line 131, referer: http://test.com/module/

I guest that the problem is in the normalizePath() that delete .., changing the true path ../shared/templates/base.html to shared/templates/base.html.

If you disable the call of method normalizePath(), for example replacing the line 131 of vendor/twig/twig/lib/Twig/Loader/Filesystem.php from

return $this->cache[$name] = $this->normalizePath($path.'/'.$shortname);
to
return $this->cache[$name] = $path.'/'.$shortname;

the template rendered.

[stof]

@fabpot the issue is that we always expected Twig_Loader_Filesystem to receive absolute paths when working on your refactoring and reviewing it, but the user base does not agree.
I suggest making Twig_Loader_Filesystem turn paths to absolute ones when configuring it.

[stof]

@omerta the workaround for now is to configure your loader with absolute paths (which is way more reliable than relying on the cwd when your code is called btw):

$loader = new Twig_Loader_Filesystem(array(__DIR__.'/templates/', __DIR__.'/../shared/templates/'));

$twig = new Twig_Environment($loader);
$template = $twig->loadTemplate('form.html');

On a side note, you don't need to call loadTemplate() yourselves. You should use Twig_Environment::render() directly (and let it deal with the template loading internally)

[fabpot]

@stof Converting relative paths to absolute ones is not a good idea. Let me explain why and some ideas on how to fix this issue.

The main problem is that we are using the template path for two different things:

• to get the contents on disk (and here, we do need an absolute path at some point, using getcwd() for a relative path is an option);
• as a cache key (this should be relative as it should not depend on where the project is stored).

I've worked on a patch to fix that but it's somehow convoluted. But I'm wondering why we need the path for the cache key, it looks like using a normalized template name would be more than enough. Something like this:

public function getCacheKey($name)
{
    // will throw an exception if there is an issue finding this template
    $this->findTemplate($name);

    // this is unique and should be good as a cache key
    return $this->normalizeName($name);
}

Any drawback?

[stof]

@fabpot using the file name means that the cache is invalidated if you overwrite the template in a new folder (as Twig_Loader_Filesystem supports having multiple folders). So we need to involve the base folder being used in some way. Normalizing alone would break BC.

[fabpot]

@stof Here is my WIP: https://github.com/twigphp/Twig/compare/1.x...fabpot:loader-relative-paths?expand=1 What do you think?

[stof]

looks good, except that you should be careful about windows support (where the path separator in realpath will not be /) and about support for phars (where realpath($rootPath) may be false)

[fabpot]

phars are taken into account as if realpath() returns false, we just keep the path as is, which means that the absolute path is used for the cache key, no big deal. PR here (I will add some more tests): #2195