Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[TwigBridge] use reproducible variable names in the default domain node visitor#58706
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
xabbuh commentedOct 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if a counter can replace any hashing, then yes, that'd be nicer
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I updated the implementation to rely on a counter instead
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.
@nicolas-grekas using a counter for the nesting level inside the current template doesnot solve the issue that this PR tries to fix, meaning that my comment in#57609 (comment) isnot 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
FTR, this was fixed in#58706
5557736 intosymfony:7.2Uh oh!
There was an error while loading.Please reload this page.
Thank you@xabbuh |
| if (class_exists(Nodes::class)) { | ||
| $name = new LocalVariable(null, $node->getTemplateLine()); |
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.
@xabbuh changing this to a local variable is likely the root cause ofEasyCorp/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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@nicolas-grekas using a counter for the nesting level inside the current template doesnot solve the issue that this PR tries to fix, meaning that my comment in#57609 (comment) isnot 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),FixEasyCorp/EasyAdminBundle#6605,Fixtwigphp/Twig#4480| License | MITCommits-------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 |Fixsymfony/symfony#58706 (comment),FixEasyCorp/EasyAdminBundle#6605,Fixtwigphp/Twig#4480| License | MITCommits-------1a38acac24 generate conflict-free variable names