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

[TwigBundle] Fixed caching of templates in src/Resources/<BundleName>/views on cache warmup #28376

Merged
merged 1 commit into from Sep 27, 2018

Conversation

Projects
None yet
5 participants
@yceruto
Copy link
Contributor

commented Sep 6, 2018

Q A
Branch? 2.8
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Same as #27764, but in this case the convention is wrong.

Corrected according to:

if (is_dir($dir = $container->getParameter('kernel.root_dir').'/Resources/'.$name.'/views')) {

@yceruto

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2018

This iterator suffers from tachycardia having to duplicate the paths registration and convention :)

Note that we're still missing custom paths registered in a third party bundles through twig.loader.native_filesystem service, e.g.:

$container->getDefinition('twig.loader.native_filesystem')
    ->addMethodCall('addPath', array('/foo/templates/path', 'Foo'));

The templates inside it, will not be cached on cache warmup.

I wondering if can we iterate over the Twig loader filesystem instead?

@stof

This comment has been minimized.

Copy link
Member

commented Sep 6, 2018

We should recommend third-party bundles to add their paths through adding them in the bundle config (with PrependExtensionInterface on their DI extension).

Adding paths directly on the filesystem loader is quite fragile, because Symfony is injecting the paths in multiple places, and twig.loader.native_filesystem might even not be used at all (when symfony/templating is installed, we use a different loader).
Thus, compiler passes might run after the TwigBundle one, so reading method calls added on this services (with some complex logic to detect the one not added by TwigBundle itself) will not even ensure that this works (it would depend on the order of passes)

@nicolas-grekas nicolas-grekas added this to the 2.8 milestone Sep 6, 2018

@yceruto

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2018

We should recommend third-party bundles to add their paths through adding them in the bundle config (with PrependExtensionInterface on their DI extension).

I've a use case where that recommendation wouldn't work. For example: a shared third-party bundle aiming to override templates (company design & themes standardization) from others third-party bundles (@Twig/Exception/*, @SonataAdmin/*, @EasyAdmin/*, etc), without losing the possibility of override templates at project level (for minor tweaks).

This shared bundle will work for our 4.0+ projects only (without symfony/templating). Using Twig paths config (with PrependExtensionInterface on their DI extension) will add the new paths on the top, making impossible the overriding at project level.

Therefore, reading method calls added on this services (twig.loader.native_filesystem), with some logic to insert the new path just before the last one per bundle namespace, was the solution so far.

This shared bundle would also define a new templates/ directory instead of the traditional Resources/views/ (yes, I had to register it manually), but I had to undo these changes because these templates aren't cached on cache warmup, as this TemplateIterator, is not aware of dynamic paths added on twig.loader.native_filesystem service (I could have updated the iterator definition, but I thought it was too much).

Adding paths directly on the filesystem loader is quite fragile, because Symfony is injecting the paths in multiple places, and twig.loader.native_filesystem might even not be used at all (when symfony/templating is installed, we use a different loader).

I think it is possible to detect that situation and inject the right loader, as well as the method calls are copied during this transition:

$loader->setMethodCalls(array_merge($twigLoader->getMethodCalls(), $loader->getMethodCalls()));

Still I think that the current logic behind this iterator is quite fragile too and prone to errors, by using the Twig loader we can remove all this logic and rely only on the registered paths. Thus, removing the need to keep updated the iterator paths everywhere where a new path is added to the loader.

This is just an idea, meanwhile we can merge this as it is.

@stof

This comment has been minimized.

Copy link
Member

commented Sep 6, 2018

I think it is possible to detect that situation and inject the right loader, as well as the method calls are copied during this transition:

but your own compiler pass might be running *after that transition. That's precisely why I'm saying it is fragile to do it

@yceruto

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2018

but your own compiler pass might be running *after that transition. That's precisely why I'm saying it is fragile to do it

Can't I use (in my own compiler pass) the twig.loader.filesystem service if templating service is activated, otherwise twig.loader.native_filesystem? I've setting a priority to ensure it's executed after ExtensionPass.

About the TemplateIterator, we can to inject twig.loader.filesystem always, as it'll an alias of twig.loader.native_filesystem when templating doesn't exist.

@yceruto yceruto force-pushed the yceruto:fix_template_cache branch from e53c98a to 83a75f4 Sep 8, 2018

@nicolas-grekas nicolas-grekas requested a review from stof Sep 18, 2018

@yceruto

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

This is ready on my side. I'll move the discussion about the TemplateIterator issues in a separate issue.

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 27, 2018

Could be seen as a BC break. I would only do the change in master with a note in the UPGRADE file. WDYT?

@yceruto

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2018

Could be seen as a BC break. I would only do the change in master with a note in the UPGRADE file. WDYT?

Are you referring to the proposal "iterate over the Twig loader filesystem instead"? I agree, I'll do it soon.

However, we still have to fix this bug on 2.8 (as is).

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 27, 2018

I was referring to the current patch. People might have stored their templates in the current supported directory

@yceruto

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2018

AFAIK this path $this->rootDir.'/'.$bundle->getName().'/views' was never supported nor expected to work: src/FooBundle/views, also it's not added to the loader to render a template. How would it be a BC break?

@yceruto

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2018

Even if ppl registering it (manually in Twig paths) they still can render templates there and Twig paths are taken into account on this TemplateIterator, so all these templates will be cached on warmup.

This fix the cache warmup of the overriding convention templates in src/Resources/<BundleName>/views as defined here:

if (is_dir($dir = $container->getParameter('kernel.root_dir').'/Resources/'.$name.'/views')) {

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 27, 2018

Thank you @yceruto.

@fabpot fabpot merged commit 83a75f4 into symfony:2.8 Sep 27, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Sep 27, 2018

bug #28376 [TwigBundle] Fixed caching of templates in src/Resources/<…
…BundleName>/views on cache warmup (yceruto)

This PR was merged into the 2.8 branch.

Discussion
----------

[TwigBundle] Fixed caching of templates in src/Resources/<BundleName>/views on cache warmup

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Same as #27764, but in this case the convention is wrong.

Corrected according to:
https://github.com/symfony/symfony/blob/992a174470fd557e1cddccd3a35447209602aea3/src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php#L165

Commits
-------

83a75f4 Caching missed templates on cache warmup

@yceruto yceruto deleted the yceruto:fix_template_cache branch Sep 27, 2018

This was referenced Sep 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.