[TwigBridge] use reproducible variable names in the default domain node visitor #58706
Conversation
xabbuh
commented
Oct 29, 2024
| Q | A |
|---|---|
| Branch? | 7.2 |
| Bug fix? | no |
| New feature? | yes |
| Deprecations? | no |
| Issues | Fix #57609 (comment) |
| License | MIT |
| return new SetNode(false, new Nodes([$name]), new Nodes([$node->getNode('expr')]), $node->getTemplateLine()); | ||
| } | ||
|
|
||
| $var = '__internal_trans_default_domain'; | ||
|
|
||
| if (null !== $templateName = $node->getTemplateName()) { |
There was a problem hiding this comment.
I wonder what we do when there is no template name? We could use an internal counter in this visitor and append it to the prefix instead maybe?
There was a problem hiding this comment.
if a counter can replace any hashing, then yes, that'd be nicer
45183e0 to
500d72f
Compare
| return new SetNode(false, new Nodes([$name]), new Nodes([$node->getNode('expr')]), $node->getTemplateLine()); | ||
| } | ||
|
|
||
| $var = '__internal_trans_default_domain_'.$this->nestingLevel; |
There was a problem hiding this comment.
I updated the implementation to rely on a counter instead
There was a problem hiding this comment.
@nicolas-grekas using a counter for the nesting level inside the current template does not solve the issue that this PR tries to fix, meaning that my comment in #57609 (comment) is not solved and that Symfony 7.2 still breaks the case where both a parent template and its child template use {% trans_default_domain %}
There was a problem hiding this comment.
FTR, this was fixed in #58706
|
Thank you @xabbuh |
| if (class_exists(Nodes::class)) { | ||
| $name = new LocalVariable(null, $node->getTemplateLine()); |
There was a problem hiding this comment.
@xabbuh changing this to a local variable is likely the root cause of EasyCorp/EasyAdminBundle#6605 because a local variable will not be available in blocks (it won't stay available in the Twig child scopes that are totally separate PHP scopes in the compiled template).
To me, we should keep assigning a context variable, as done before your PR.
| return new SetNode(false, new Nodes([$name]), new Nodes([$node->getNode('expr')]), $node->getTemplateLine()); | ||
| } | ||
|
|
||
| $var = '__internal_trans_default_domain_'.$this->nestingLevel; |
There was a problem hiding this comment.
@nicolas-grekas using a counter for the nesting level inside the current template does not solve the issue that this PR tries to fix, meaning that my comment in #57609 (comment) is not solved and that Symfony 7.2 still breaks the case where both a parent template and its child template use {% trans_default_domain %}
This PR was merged into the 7.2 branch. Discussion ---------- [TwigBridge] generate conflict-free variable names | Q | A | ------------- | --- | Branch? | 7.2 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #58706 (comment), Fix EasyCorp/EasyAdminBundle#6605, Fix twigphp/Twig#4480 | License | MIT Commits ------- 1a38aca generate conflict-free variable names
This PR was merged into the 7.2 branch. Discussion ---------- [TwigBridge] generate conflict-free variable names | Q | A | ------------- | --- | Branch? | 7.2 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix symfony/symfony#58706 (comment), Fix EasyCorp/EasyAdminBundle#6605, Fix twigphp/Twig#4480 | License | MIT Commits ------- 1a38acac24 generate conflict-free variable names