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 default path on cache warmup #27764

Merged
merged 1 commit into from Sep 5, 2018

Conversation

Projects
None yet
5 participants
@yceruto
Copy link
Contributor

commented Jun 28, 2018

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

Reproducer

$ git clone https://github.com/symfony/demo && cd demo
$ bin/console cache:clear
$ find var/cache/dev/twig -printf "%y\n" | awk '/f/{f++}/d/{d++}END{printf "%d directories, %d files\n", d, f}'
...

Before:

...
131 directories, 167 files

Twig paths config:

$container->getDefinition('twig.template_iterator')->replaceArgument(2, $config['paths']);

%kernel.root_dir%/Resources/views:
$this->templates = $this->findTemplatesInDirectory($this->rootDir.'/Resources/views');

After:

...
141 directories, 193 files

Adding %twig.default_path%.

@yceruto yceruto changed the title [TwigBundle] Fix missed templates in default path on cache warmup [TwigBundle] Fixed caching of templates in default path on cache warmup Jun 28, 2018

@yceruto yceruto force-pushed the yceruto:templates_cache branch from 16bc18f to 0a4c079 Jun 28, 2018

@nicolas-grekas nicolas-grekas modified the milestones: 4.0, 3.4 Jun 29, 2018

@nicolas-grekas nicolas-grekas requested a review from stof Jul 9, 2018

@yceruto

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2018

Any update about this?

@stof do you still think this solution is wrong? what is your suggestion? thanks.

@fabpot

This comment has been minimized.

Copy link
Member

commented Jul 19, 2018

@stof How can we move forward on this?

@yceruto

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2018

Can we move forward by providing some facts?

About the issue, this is the bug reproducer (see README.md for instructions).

As for the fix: adds the missing default Twig path in TemplateIterator to find templates in this directory (to cache them later) seemed like a fast merge 2 months ago.

that's the wrong fix, because that's not the template name.

It is the template name according to the test case. There is no TwigBundle configured here, I just reuse an existing path. Also (as we know) for Twig, the template class for bundles/TwigBundle/layout.html.twig is the same as @Twig/layout.html.twig (if this is the correct template name you expect), both refer to the same filename templates/bundles/TwigBundle/layout.html.twig hence same cache key and same cache file (taking the symfony-demo project as example).

I hope this helps to unlock the status of this PR.

@fabpot

fabpot approved these changes Sep 4, 2018

@stof

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

@yceruto but if you warmup the cache using bundles/TwigBundle/layout.html.twig, _self in it will refer to bundles/TwigBundle/layout.html.twig, not to @Twig/layout.html.twig (and also many error messages), which might create confusion (as the expected usage of this template is through the namespaced notation).
I actually think that the fact that our bundle-override path for template is actually a subpath of the main namespace is a drawback of the new structure (one we haven't identified earlier unfortunately) as it means these templates have 2 different names

@stof

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

an alternative might be to exclude %twig.default_path%/bundles from the locator for the main namespace (as that's a convention handled by the bundle anyway). These templates would then be located using their actual expected name.

@yceruto yceruto force-pushed the yceruto:templates_cache branch from 0a4c079 to 2518755 Sep 4, 2018

@yceruto yceruto requested review from dunglas, lyrixx, sroze and xabbuh as code owners Sep 4, 2018

@yceruto yceruto force-pushed the yceruto:templates_cache branch from 2518755 to 450632a Sep 4, 2018

@yceruto yceruto force-pushed the yceruto:templates_cache branch 2 times, most recently from b00c63d to 245c860 Sep 4, 2018

@yceruto

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2018

@stof thanks for clarifying, you're right about _self inside templates. I made the changes accordingly your last suggestion.

Sorry for the firing of reviews on rebasing.

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

@stof Can you review one last time before I merge? Thank you.

@stof

stof approved these changes Sep 5, 2018

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

Thank you @yceruto.

@fabpot fabpot merged commit 245c860 into symfony:3.4 Sep 5, 2018

2 of 3 checks passed

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

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

bug #27764 [TwigBundle] Fixed caching of templates in default path on…
… cache warmup (yceruto)

This PR was merged into the 3.4 branch.

Discussion
----------

[TwigBundle] Fixed caching of templates in default path on cache warmup

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

**Reproducer**
```bash
$ git clone https://github.com/symfony/demo && cd demo
$ bin/console cache:clear
$ find var/cache/dev/twig -printf "%y\n" | awk '/f/{f++}/d/{d++}END{printf "%d directories, %d files\n", d, f}'
...
```
**Before:**
```bash
...
131 directories, 167 files
```
Twig `paths` config:
https://github.com/symfony/symfony/blob/44ce4dd62536037f9cea4db0146541b4ab867d34/src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php#L104
`%kernel.root_dir%/Resources/views`:
https://github.com/symfony/symfony/blob/2b01d594818cf872bc481b3a1fe0210da29fab69/src/Symfony/Bundle/TwigBundle/TemplateIterator.php#L50
**After:**
```bash
...
141 directories, 193 files
```
Adding `%twig.default_path%`.

Commits
-------

245c860 Fixed caching of templates in default path on cache warmup

@yceruto yceruto deleted the yceruto:templates_cache branch Sep 5, 2018

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

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.