zendframework / zend-expressive-twigrenderer

Twig integration for Expressive
BSD 3-Clause "New" or "Revised" License
25 stars 14 forks source link

Optional twig extension #43

Closed geerteltink closed 6 years ago

geerteltink commented 6 years ago

This PR adds a TwigExtensionFactory so you can override the TwigExtension if you want. Currently you cannot change the default absolute_url, asset, path and url functions. By using an extension factory you can extend and change the default extension or completely replace it.

Currently, it still is hardcoded in the TwigEnvironmentFactory. Ideally, this extension should be loaded as a regular extension. However doing so, it can't be enabled by default as it requires the ServerUrlHelper and UrlHelper. When adding it to the ConfigProvider we can't check if both dependencies are available or exit silently as the container should throw an exception if no service is returned.

Fixes #41

geerteltink commented 6 years ago

@mwop @webimpress ready for review and merge.

This code is never reached because the config is checked at the start as well. Remove it or leave it?

https://github.com/zendframework/zend-expressive-twigrenderer/blob/81e7a4a422a6551bf1aeb68b46572f82e38517d0/src/TwigEnvironmentFactory.php#L252-L255

geerteltink commented 6 years ago

This code is never reached because the config is checked at the start as well. Remove it or leave it? https://github.com/zendframework/zend-expressive-twigrenderer/blob/81e7a4a422a6551bf1aeb68b46572f82e38517d0/src/TwigEnvironmentFactory.php#L252-L255

Fixed it by making mergeConfig a public static function in the TwigRendererFactory and removing it in the 2 other factories as it was the exact same code.