Fix the Twig loader infrastructure #6936
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
the changes include - inlining of the parent class - moving the responsibility of reading directories and caching files from the ContaoFilesystemLoaderWarmer to the ContaoFilesystemLoader# - making the ContaoFilesystemLoaderWarmer an optional cache warmer - removal of the TemplateHierarchyInterface - outsourcing of the auto refresh event into its own class - optimizations
|
I will test it in the next hour. |
|
While I encountered the error once in my That being said, I think the changes make sense to me. |
|
Thank you @m-vo. |
|
Will this be backported to 4.13? |
|
Thank you @m-vo Unfortunately, I have not yet been able to reproduce the error in my installations. So I can't help with testing at the moment. |
m-vo
added a commit
to m-vo/contao
that referenced
this pull request
Feb 28, 2024
This was referenced Feb 28, 2024
leofeyer
pushed a commit
that referenced
this pull request
Mar 5, 2024
leofeyer
pushed a commit
that referenced
this pull request
Mar 20, 2024
Description ----------- Followup to #6936 Not sure if this is the correct thing to do, but it restores the behavior of 5.3.0 and fixes madeyourday/contao-rocksolid-custom-elements#172 Before Contao 5.3.1 a template in a theme folder was registered as `@Contao_Global/my_theme/foo.html.twig` as well as `@Contao_Theme_my_theme/foo.html.twig`. I think we should restore this behavior for backwards compatibility. The current behavior would also be confusing for people that use folders like `content_element` in the theme configuration for export purposes. @m-vo Are there any downsides in doing this? Commits ------- 7ba109e Register theme templates in the global namespace too 105c6cd Revert FilesystemLoader changes e85811e Do not filter out templates in theme directories
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes a problem where templates would not be recognized when clearing the cache in production.
Spoiler alert: 🐰🕳️
The original problem
In the existing implementation, we had a mandatory cache warmer, the
ContaoFilesystemLoaderWarmer, that gathered the Contao template paths, added them to our loader and made it persist its cache. Symfony'scache:clearis a command, so when running it on an empty/nuked cache, it first builds the container and thus executing the mandatory cache warmers. After that the command finally clears the cache and runs the cache warmers gain. However, now, only the optional ones, reasoning the others had already run when the container was built. So our custom cache, living in the cache pool, gets killed (and not rebuild). 🤦♂️ It was only a side-effect, that it still worked in dev mode (auto-refresh logic).So yeah, this is a design issue due to mixed up responsibilities. It resulted from the attempt to build a loader similar to Twig's own. But it turned out, that ours is quite different, because it a) must know about individual template files and b) must allow a refresh at runtime.
The fix
The responsibility to find and memorize templates is now moved to the
ContaoFilesystemLoaderin its entirety. That means, there are no more functions to build, clear or persist the inheritance chains. They will get created (and cached) on demand or, optionally, when calling a newwarmUp()method. This makes so much more sense as it encapsulates the state and inner workings and just provides the hierarchy every time someone requests it.The
ContaoFilesystemLoaderWarmeris now an optional cache warmer and basically only callsContaoFilesystemLoader#warmUp(). I also extracted the listener that automatically refreshed the template hierarchy in dev mode to its own class because this was also a mix of responsibilities.As the
ContaoFilesystemLoaderis the only thing ever, that knows about the template hierarchy, I also dropped theTemplateHierarchyInterface. This was an attempt to separate concerns but keeping it would now add even more confusion than before. If you need the template hierarchy, inject theContaoFilesystemLoader. Btw.: there are quite some files changed in this PR, however, most of the changes outside the loader/cache warmer files are just switchingTemplateHierarchyInterfacewithContaoFilesystemLoader.Moving things into the
ContaoFilesystemLoaderclass also made it obvious, that extending from Twig's filesystem loader is abadunbelievably stupid idea (also see Do not extend from the original Twig filesystem loader #6871). I therefore removed the parent class, inlined the calls and refactored a lot of code. Without theparent::calls, there is now also less filesystem access, because - surprise - our loader already knows about all the files when creating the hierarchy.4.13
This PR is targeted against 5.3 because it was much simpler to do (and still a lot of pain). If things look good, we should probably backport it to 4.13 as well (maybe dropping the tests).
/cc @bytehead, @fritzmg, @contaoacademy
Please help testing this PR in your projects to ensure all the template hierarchy related features still work. This includes rendering legacy templates with Twig, extending and horizontally reusing things and so on...