-
-
Notifications
You must be signed in to change notification settings - Fork 398
Fix InteractsWithTwigComponents not being able to render blocks when variable scope is limited #3211
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
base: 2.x
Are you sure you want to change the base?
Conversation
… when variable scope is limited in tests
…variable scope is limited
|
This probably means that the template cache creates new entries whenever the block content changes. I assume that in most cases, the block content will only change when the Test code changes. Is this tolerable? Alternatively, I could imagine fixing this by calling |
|
This is problematic to me, as using outerScope really does change the runtime code and makes it in my eyes too different of what would happen in production. Also at best it would not bring added value for most users.. at worst it could be causing problems with outerScope or any change we do around its implementation. I'm pretty sure we can find a way to help you in that very specific need without impacting every other tests out there .. -- Update: comment originally written for the PR with your other implementation idea, but still valid (not for the outerscope runtime, but the cache one indeed is a big change, has even the lines in the template would change, its hash, etc..) |
| foreach (array_keys($blocks) as $blockName) { | ||
| $template .= \sprintf('{%% block %1$s %%}{{ blocks.%1$s|raw }}{%% endblock %%}', $blockName); | ||
| foreach ($blocks as $blockName => $blockContent) { | ||
| $template .= \sprintf('{%% block %1$s %%}%2$s{%% endblock %%}', $blockName, $blockContent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a good idea to change implementation here.
This will create a new template everytime, risking that two successive calls for the same component end up with different cache files. (what would be the case elsewhen, and something TwigComponent or LiveComponent sometimes uses).
But also because it does change the way it words. Current is runtime block-rendering, when your suggestion is changing that into a pre-compiled static template.
I can understand your desire to make things work the way you'd like, but for the vast majority of users out there, this change will not bring new or value, but would change the way their tests are done...
What about adding new methods in the trait.. could be specialized in testing/working with Components?
What do you feel about looking at what can be done here and how?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So i'm 👎 this one
When using Blocks, the helper for rendering Twig Components in tests (
InteractsWithTwigComponents::renderTwigComponent) would create a template like this:{% component "My:Component" with data %}{% block myBlock %}{{ blocks.myBlock|raw }}{% endblock %}{% endcomponent %}Passing the variables
dataandblocksfrom outside, whereblockscontains the block content indexed by block name.If the block however was declared inside a
{% with … only %}scope, this would not work, because inside the block, you wouldn't have access toblocks.This PR fixes the issue by inlining the block content when creating the template.
This probably means that the template cache creates new entries whenever the block content changes. I don't know if this is tolerable.