Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Twig 1.38.0 classes Twig_Loader_Filesystem and Twig_Environment don't accept constructor parameters #2907

Closed
nizamani opened this issue Mar 20, 2019 · 12 comments

Comments

@nizamani
Copy link

Twig version v1.38.0 removed constructor parameters from Twig_Loader_Filesystem and Twig_LoaderInterface classes, both of these classes used to accept parameters $path and $options. This is how our current code in v1.35.4 defines loader.
This change breaks our code since loader is not set because these parameters were removed. Our code now throws following exception:
Fatal error: Uncaught exception 'LogicException' with message 'You must set a loader first.'
We load twig templates at lots of places, is there a way to fix this error in our codebase without having to edit 100s of files?

@xabbuh
Copy link
Contributor

xabbuh commented Mar 20, 2019

Are you sure? The method signature doesn't seem to have changed in a while: https://github.com/twigphp/Twig/blob/1.x/src/Loader/FilesystemLoader.php#L37

@nizamani
Copy link
Author

Yes I am sure for me this file twig/twig/src/Loader/FilesystemLoader.php just has following code:

namespace Twig\Loader;

class_exists('Twig_Loader_Filesystem');

if (\false) {
    class FilesystemLoader extends \Twig_Loader_Filesystem
    {
    }
}

@xabbuh
Copy link
Contributor

xabbuh commented Mar 20, 2019

But in that case you should see code similar to what I linked to for the Twig_Loader_Filesystem which is a class alias. So the question rather is which concrete issues do you run into?

@nizamani
Copy link
Author

Issue is when I update from Twig v1.35.4 to v1.38.0 I get this error:
Fatal error: Uncaught exception 'LogicException' with message 'You must set a loader first.'
Maybe this is because our code is not written correctly since v1.35.4 to v1.38.0 is not a breaking change according to semver.
What changes would you suggest we should make to our code so it doesn't throw this error

@xabbuh
Copy link
Contributor

xabbuh commented Mar 20, 2019

Can you show a bit of how Twig is set up in your case and the full stack trace of this exception?

@nizamani
Copy link
Author

We are using Auryn\Injector to setup twig loader and pass path and options vars to twig using injector code is like this:

// vars
$paths = "/path/to/templates";
$options = array(
"cache" => "/path/to/cache",
"autoescape" => true
);
$base = "path/to/views";

// injector
$injector = new Auryn\Injector;
$injector->define("Twig_Loader_Filesystem", [":paths" => $paths]);
$injector->define("Twig_Environment", [":options" => $options]);

// tell the Injector class to inject an instance of Twig_Loader_Filesystem any time
// it encounters an Twig_LoaderInterface type-hint
$injector->alias('Twig_LoaderInterface', 'Twig_Loader_Filesystem');

// controller
$controller = $injector->make("Controller");

I am getting following error:

Fatal error: Uncaught exception 'LogicException' with message 'You must set a loader first.' in /projectroot/vendor/twig/twig/src/Environment.php on line 10896 LogicException: You must set a loader first. in /projectroot/vendor/twig/twig/src/Environment.php on line 10896 Call Stack: 1.1175 2020672 1. Raven_ErrorHandler->handleException() /projectroot/vendor/sentry/sentry/lib/Raven/ErrorHandler.php:0

@xabbuh
Copy link
Contributor

xabbuh commented Mar 20, 2019

Maybe the injector doesn't take class aliases into account. And since the type hint in the Environment class was changed that causes issues here. I guess changing it like this could help then:

use Twig\Loader\FilesystemLoader;

// ...

$injector->alias(FilesystemLoader::class, 'Twig_Loader_Filesystem');

@nizamani
Copy link
Author

nizamani commented Mar 20, 2019

This doesn't works besides this code was working in v1.37.1, something in v1.38.0 changed that caused this issue. I think this issue is because type hint in Environment class was changed. Would you suggest a workaround for that which won't require changing lots of code.

Code breaks here:
$twigEnvironment->load('path/to/templates/index.html');

Where $twigEnvironment is object of Twig\Environment

@stof
Copy link
Member

stof commented Mar 21, 2019

Well, what changed in 1.38 is that Twig_Loader_Filesystem was the actual class in 1.37 and Twig\Loader\FilesystemLoader was the class alias, and now it is the opposite.

Maybe the Auryn\Injector does not handle class aliases properly.

@Cryszon
Copy link

Cryszon commented Mar 21, 2019

Can confirm this issue with PHP-DI as well.

Here's my definition:

$c[Twig_Environment::class] = function(ContainerInterface $c) {
  $loader = new Twig_Loader_Filesystem();
  $basePath = __DIR__ . "/../view";
  $loader->addPath($basePath);
  $loader->addPath($basePath . "/pages", "pages");
  $twig = new Twig_Environment($loader, $c->get("settings")["twig"]);

  $twig->addExtension(new TwigExtension());
  $twig->addExtension(new TestAuthTwigExtension());

  return $twig;
};

The controller is initialized with __construct(Twig_Environment $twig) and calling $twig->render($tpl) throws a LogicException with "You must set a loader first." message. There was no error before updating.

For PHP-DI (probably applies other DI containers as well) I managed to fix it by aliasing Twig_Environment to \Twig\Environment and requiring it instead:

$c[\Twig\Environment::class] = DI\get(Twig_Environment::class);
__construct(\Twig\Environment $twig)

@fabpot
Copy link
Contributor

fabpot commented Mar 21, 2019

I'm closing this issue as the problems comes from the DI libraries you are using and not from Twig.

@fabpot fabpot closed this as completed Mar 21, 2019
@exts
Copy link

exts commented Apr 4, 2019

Took me a while to figure out what this issue was. Seems like this has changed after v1.37.1 and the DI container solution works fairly well.

Edit: just checked the change log, yeah

* made namespace classes the default classes (PSR-0 ones are aliases now) 1.38.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

6 participants