[TwigBundle] Inject project root path into twig filesystem loader #20727
Conversation
4rthem
commented
Dec 2, 2016
- [TwigBundle] added project root path to filesystem loader
| Q | A |
|---|---|
| Branch? | 3.2 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #20726 |
| License | MIT |
| Doc PR | - |
| { | ||
| parent::__construct(array()); | ||
| parent::__construct(array(), realpath($rootPath)); |
There was a problem hiding this comment.
it should pass the rootPath without altering it. Thus, not all filesystems support realpath
| @@ -52,6 +52,7 @@ | ||
| <service id="twig.loader.filesystem" class="Symfony\Bundle\TwigBundle\Loader\FilesystemLoader" public="false"> | ||
| <argument type="service" id="templating.locator" /> | ||
| <argument type="service" id="templating.name_parser" /> | ||
| <argument>%kernel.root_dir%/..</argument> |
There was a problem hiding this comment.
this should use the same value than for twig.loader.native_filesystem instead, which is configured in a compiler pass and deals with passing the path without using /.. (which would not play well with systems not supporting realpath)
There was a problem hiding this comment.
Sorry, I didn't take time to look at the new code in 3.2...
f3eea6b to
b999708
Compare
| @@ -52,6 +52,7 @@ | ||
| <service id="twig.loader.filesystem" class="Symfony\Bundle\TwigBundle\Loader\FilesystemLoader" public="false"> | ||
| <argument type="service" id="templating.locator" /> | ||
| <argument type="service" id="templating.name_parser" /> | ||
| <argument></argument> <!-- project's root dir --> |
There was a problem hiding this comment.
- <argument></argument> <!-- project's root dir -->
+ <argument /> <!-- project's root dir -->(for consistency)
EDIT: Just saw the twig.loader.native_filesystem definition didn't use this notation, but it's the most common one in symfony core.
| @@ -64,6 +64,11 @@ public function process(ContainerBuilder $container) | ||
| $container->getDefinition('twig.extension.debug')->addTag('twig.extension'); | ||
| } | ||
|
|
||
| if ($container->has('twig.loader.filesystem')) { |
There was a problem hiding this comment.
I think this check is not useful. The service should exist and is loaded by the bundle extension.
We do not have such check here for instance.
b999708 to
7037a44
Compare
| @@ -64,6 +64,9 @@ public function process(ContainerBuilder $container) | ||
| $container->getDefinition('twig.extension.debug')->addTag('twig.extension'); | ||
| } | ||
|
|
||
| $loader = $container->getDefinition('twig.loader.filesystem'); | ||
| $loader->replaceArgument(2, $this->getComposerRootDir($container->getParameter('kernel.root_dir'))); |
There was a problem hiding this comment.
getComposerRootDir should only be executed once and reused below
| */ | ||
| public function __construct(FileLocatorInterface $locator, TemplateNameParserInterface $parser) | ||
| public function __construct(FileLocatorInterface $locator, TemplateNameParserInterface $parser, $rootPath = '') |
There was a problem hiding this comment.
the default value for $rootPath is null in the parent class and the child should use the same approach.
There was a problem hiding this comment.
My aim was to avoid the getcwd because this is the issue in a Symfony context (twigphp/Twig#2281).
But OK, the job is done by the compiler pass.
| * @param TemplateNameParserInterface $parser A TemplateNameParserInterface instance | ||
| * @param FileLocatorInterface $locator A FileLocatorInterface instance | ||
| * @param TemplateNameParserInterface $parser A TemplateNameParserInterface instance | ||
| * @param string $rootPath The project's root dir |
There was a problem hiding this comment.
Should use the same as https://github.com/twigphp/Twig/blob/1.x/lib/Twig/Loader/Filesystem.php#L30
7037a44 to
1094773
Compare
|
Thank you @4rthem. |
… loader (4rthem) This PR was merged into the 3.2 branch. Discussion ---------- [TwigBundle] Inject project root path into twig filesystem loader * [TwigBundle] added project root path to filesystem loader | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20726 | License | MIT | Doc PR | - Commits ------- 1094773 inject project root path into twig filesystem loader