Skip to content

Navigation Menu

Sign in

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[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
Merged
merged 1 commit into from
May 10, 2025
Merged

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

merged 1 commit into from
May 10, 2025
+210 −28

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.

src/Symfony/Bundle/TwigBundle/Cache/ChainCache.php Outdated Show resolved Hide resolved
src/Symfony/Bundle/TwigBundle/Cache/ChainCache.php Outdated Show resolved Hide resolved
src/Symfony/Bundle/TwigBundle/Cache/ChainCache.php Outdated Show resolved Hide resolved
src/Symfony/Bundle/TwigBundle/CacheWarmer/TemplateCacheWarmer.php Outdated Show resolved Hide resolved
src/Symfony/Bundle/TwigBundle/CacheWarmer/TemplateCacheWarmer.php Outdated Show resolved Hide resolved
@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
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.

src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php Outdated Show resolved Hide resolved
src/Symfony/Bundle/TwigBundle/CacheWarmer/TemplateCacheWarmer.php Outdated Show resolved Hide resolved
src/Symfony/Bundle/TwigBundle/Resources/config/twig.php Show resolved Hide resolved
@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
@Okhoshi
Copy link
Contributor Author

Okhoshi commented May 8, 2025

@GromNaN @fabpot @nicolas-grekas @OskarStark Could we still get this merged in 7.3 ?

@fabpot
Copy link
Member

fabpot commented May 10, 2025

Thank you @Okhoshi.

@fabpot fabpot merged commit 532e408 into symfony:7.3 May 10, 2025
7 of 11 checks passed
@fabpot fabpot mentioned this pull request May 10, 2025
Closed
@Okhoshi Okhoshi deleted the twig-cache-build-dir branch May 10, 2025 08:41
@Okhoshi Okhoshi mentioned this pull request May 10, 2025
Merged
@fabpot fabpot mentioned this pull request May 10, 2025
Merged
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request May 12, 2025
@javiereguiluz
minor #20951 [Twig] [TwigBundle] Describe the new behaviour of twig.c…
f6e6a8c 
…ache config (Okhoshi)

This PR was merged into the 7.3 branch.

Discussion
----------

[Twig] [TwigBundle] Describe the new behaviour of twig.cache config

Update the description of the `twig.cache` config to follow the changes implemented in symfony/symfony#54384.

Fixes #20948

cc `@alexandre`-daubois

Commits
-------

8c5f22b [TwigBundle] Describe the new behaviour of twig.cache config
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

@fabpot fabpot fabpot 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

@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: Reviewed TwigBundle
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.
Morty Proxy This is a proxified and sanitized view of the page, visit original site.