search

fvsch / kirby-twig

Twig templating support for Kirby CMS 2. For Kirby 3, use https://github.com/amteich/kirby-twig
MIT License
70 stars 8 forks source link

Check Twig Class instead of autoload #4

Closed nunocodex closed 8 years ago

nunocodex commented 8 years ago

In the end I see you check the composer autoload but I think the best way is check if the Twig class exists.

fvsch commented 8 years ago

On the technical side:

You mean loading vendor/autoload.php in a try/catch, silencing the error, then checking if the Twig_Environment class exists?

Or do you mean that some people may be using Composer already and may add Twig to their project's requirements?

On the use case side:

What is the use case for this kind of change?

Right now it seems very few people are using Composer to install Kirby, since it's not supported out of the box and you have to write your own mappings.

Thanks for your input, it's much appreciated. :)

nunocodex commented 8 years ago

I think instead to check if the autoload.php file exists you can check if the twig class exists in the try/catch. This leave all 2 way you mentioned before.

nunocodex commented 8 years ago

Reading much better you can check before the twig class, if not exists check the autoload, is the best way i can thinking.

fvsch commented 8 years ago

I'm not 100% sure how that would look. Can you write a code example here or submit a pull request?

nunocodex commented 8 years ago

No problem I added a PR https://github.com/fvsch/kirby-twig/pull/5

fvsch commented 8 years ago

Fixed in https://github.com/fvsch/kirby-twig/commit/6c43d747b52d9c9bf0c3201f2926db2f4929d7d0

I also renamed the main class from TwigComponent to KirbyTwigComponent. Since Twig uses Twig_*, it was probably low risk but still confusing to use TwigComponent.