Skip to content

[TwigBundle] Use kernel.build_dir to store the templates known at build time #54384

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: 7.3
Choose a base branch
from
Open

[TwigBundle] Use kernel.build_dir to store the templates known at build time #54384

wants to merge 1 commit into from

Conversation

Okhoshi
Copy link
Contributor

@Okhoshi Okhoshi commented Mar 23, 2024 •
edited
Loading

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
Issues none
License MIT

Follow up to #50391, set up the Twig TemplateCacheWarmer to use the kernel.build_dir instead of kernel.cache_dir. A new argument is added to its constructor to specify the directory to use for the cache.

@Okhoshi Okhoshi requested a review from yceruto as a code owner March 23, 2024 21:49
@carsonbot carsonbot added this to the 7.1 milestone Mar 23, 2024
@Okhoshi Okhoshi force-pushed the twig-cache-build-dir branch 6 times, most recently from fb68b3c to 0bc3774 Compare March 24, 2024 14:59
@Okhoshi Okhoshi force-pushed the twig-cache-build-dir branch from 0bc3774 to c5b4ab0 Compare March 25, 2024 06:31
@Okhoshi Okhoshi force-pushed the twig-cache-build-dir branch from c5b4ab0 to 95b994c Compare March 25, 2024 08:04
GromNaN
GromNaN requested changes Mar 26, 2024
View reviewed changes
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good idea.

@Okhoshi Okhoshi force-pushed the twig-cache-build-dir branch 2 times, most recently from 0dbdd9b to 40e485c Compare March 27, 2024 10:41
@stof
Copy link
Member

stof commented Mar 27, 2024

shouldn't this dual cache setup be disabled when autoReload is enabled ? Otherwise, any template cached in the readonly cache won't be reloaded (as the cache never writes the new content) which will break the DX

@Okhoshi
Copy link
Contributor Author

Okhoshi commented Mar 27, 2024

shouldn't this dual cache setup be disabled when autoReload is enabled ? Otherwise, any template cached in the readonly cache won't be reloaded (as the cache never writes the new content) which will break the DX

I don't think it needs to. Looking at the Environment::loadTemplate method, it skips the cache::load call if autoReload is enabled, and calls cache::write directly. As the ChainCache propagates the write to all caches, from that moment, only the Runtime cache will be used, with the updated copy of the template.
Or did I miss something ?

@Okhoshi Okhoshi force-pushed the twig-cache-build-dir branch from 40e485c to 4289c16 Compare March 27, 2024 12:34
@Okhoshi Okhoshi requested a review from GromNaN March 27, 2024 13:36
@Okhoshi Okhoshi force-pushed the twig-cache-build-dir branch from 5155bf6 to 3483575 Compare December 7, 2024 12:13
GromNaN
GromNaN reviewed Mar 31, 2025
View reviewed changes
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're close to the result we wanted. Do you want to continue @Okhoshi?

@Okhoshi Okhoshi force-pushed the twig-cache-build-dir branch 2 times, most recently from 5c7c0b8 to bf25c25 Compare April 1, 2025 07:29
@Okhoshi Okhoshi requested a review from GromNaN April 1, 2025 07:31
@Okhoshi Okhoshi force-pushed the twig-cache-build-dir branch from bf25c25 to 70757d5 Compare April 1, 2025 08:15
GromNaN
GromNaN reviewed Apr 1, 2025
View reviewed changes
Copy link
Member

@GromNaN GromNaN left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To summarize the configuration:

  • cache === false: No cache
  • cache is string path: Filesystem cache using the configured path, warmed by cache:warmup.
  • cache === true && auto_reload === true: (debug mode) Filesystem cache in the cache dir, warmed by cache:warmup
  • cache === true && auto_reload === false: (prod mode) Read-only filesystem cache in the build dir warmed by cache:warmup command, and filesystem cache in %kernel.cache_dir%/twig for dynamic templates.

It would be perfect to add service definition tests for each configuration, using micro-kernels in the tests.

@Okhoshi Okhoshi force-pushed the twig-cache-build-dir branch from 70757d5 to b55820e Compare April 1, 2025 15:41
@Okhoshi Okhoshi requested a review from GromNaN April 8, 2025 09:46
@Okhoshi Okhoshi force-pushed the twig-cache-build-dir branch from b55820e to 1a38f61 Compare April 8, 2025 11:36
GromNaN
GromNaN requested changes Apr 8, 2025
View reviewed changes
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it on a project and got some functional feedback.

@Okhoshi Okhoshi force-pushed the twig-cache-build-dir branch from 1a38f61 to a0bbca5 Compare April 9, 2025 20:46
@GromNaN GromNaN mentioned this pull request Apr 10, 2025
Closed
@GromNaN
Copy link
Member

GromNaN commented Apr 11, 2025

Tests are failing.

Symfony\Component\DependencyInjection\Exception\ParameterNotFoundException: You have requested a non-existent parameter "kernel.build_dir". Did you mean this: "kernel.cache_dir"?

@Okhoshi
[TwigBundle] Use kernel.build_dir to store the templates known at b…
0f841d2 
…uild time

Signed-off-by: Quentin Devos <4972091+Okhoshi@users.noreply.github.com>
@Okhoshi Okhoshi force-pushed the twig-cache-build-dir branch from a0bbca5 to 0f841d2 Compare April 11, 2025 15:04
@Okhoshi
Copy link
Contributor Author

Okhoshi commented Apr 11, 2025

Tests are failing.

Symfony\Component\DependencyInjection\Exception\ParameterNotFoundException: You have requested a non-existent parameter "kernel.build_dir". Did you mean this: "kernel.cache_dir"?

Fixed, I didn't realise the tests were using a smaller kernel.

@GromNaN GromNaN requested a review from stof April 18, 2025 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dkarlovi dkarlovi dkarlovi left review comments

@GromNaN GromNaN GromNaN approved these changes

@yceruto yceruto Awaiting requested review from yceruto yceruto is a code owner

@derrabus derrabus Awaiting requested review from derrabus

@stof stof Awaiting requested review from stof

@fabpot fabpot Awaiting requested review from fabpot

@nicolas-grekas nicolas-grekas Awaiting requested review from nicolas-grekas

@OskarStark OskarStark Awaiting requested review from OskarStark

Assignees
No one assigned
Labels
Feature ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Needs Review TwigBundle
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.