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] generate conflict-free variable names#59059
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 commentedDec 2, 2024
| Q | A |
|---|---|
| Branch? | 7.2 |
| Bug fix? | yes |
| New feature? | no |
| Deprecations? | no |
| Issues | Fix#58706 (comment),FixEasyCorp/EasyAdminBundle#6605,Fixtwigphp/Twig#4480 |
| License | MIT |
src/Symfony/Bridge/Twig/NodeVisitor/TranslationDefaultDomainNodeVisitor.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
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.
thank you!
| private Scope $scope; | ||
| private int $nestingLevel = 0; | ||
| private int $nameCounter = 0; |
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.
Having a global name counter will not produce reproducible builds (as it depends on the order of compilation of templates). And it does not even prevent conflicts in case the parent template was already cached from a previous compilation (and so not taking into account by the current counter).
There's a good reason why I previously suggested hashing the template name.
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 changed the implementation back to what I initially had in#58706. But my question from#58706 (comment) still stands. How do we handle cases where there is no template name? Is using the counter (as I did for now) the way to go?
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.
AFAICT, this case cannot happen in a node visitor for traversed nodes. The parser sets the source context in the node during parsing, which sets it recursively in the whole tree. And from that point, setting a child node in a node of the tree will inject the source context in the child node.
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.
What would you suggest? Remove the counter and do nothing? Or throw a runtime exception when that happens?
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 suggest throwing a LogicException to be able to become aware of this unsupported state, in case it turns out it is not an impossible state. For now, I'm sure we would never reach this exception in practice during a template compilation.
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.
okay, I have updated the code accordingly
src/Symfony/Bridge/Twig/NodeVisitor/TranslationDefaultDomainNodeVisitor.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
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.
These changes fixed the reported issue for me. Thanks!
cc1802d intosymfony:7.2Uh oh!
There was an error while loading.Please reload this page.
…luz)This PR was squashed before being merged into the 4.x branch.Discussion----------Temporary ignore errors in Symfony 7.2 branchEasyAdmin won't work with Symfony 7.2 until this bugfix:symfony/symfony#59059 is not released in Symfony 7.2.1.So, ignore errors (temporarily) in Symfony 7.2.Commits-------5731774 Temporary ignore errors in Symfony 7.2 branch
…ith dynamic expressions (stof)This PR was merged into the 6.4 branch.Discussion----------[TwigBridge] Add tests covering `trans_default_domain` with dynamic expressions| Q | A| ------------- | ---| Branch? | 6.4| Bug fix? | no| New feature? | no| Deprecations? | no| Issues || License | MITThis adds tests covering the usage of `{% trans_default_domain %}` with dynamic expressions (which we broken in 7.2.0 and fixed in#59059, which those tests would have caught).I'm adding those tests in 6.4 to increase test coverage on all versions, given that the test does not require changes anyway when upmerging.#SymfonyHackdayCommits-------6262a62 Add tests covering `trans_default_domain` with dynamic expressions