[TwigBridge] generate conflict-free variable names #59059
Conversation
xabbuh
commented
Dec 2, 2024
| 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 |
src/Symfony/Bridge/Twig/NodeVisitor/TranslationDefaultDomainNodeVisitor.php
Show resolved
Hide resolved
| private Scope $scope; | ||
| private int $nestingLevel = 0; | ||
| private int $nameCounter = 0; |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
okay, I have updated the code accordingly
src/Symfony/Bridge/Twig/NodeVisitor/TranslationDefaultDomainNodeVisitor.php
Outdated
Show resolved
Hide resolved
…luz) This PR was squashed before being merged into the 4.x branch. Discussion ---------- Temporary ignore errors in Symfony 7.2 branch EasyAdmin won't work with Symfony 7.2 until this bug fix: 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 | MIT
This 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.
#SymfonyHackday
Commits
-------
6262a62 Add tests covering `trans_default_domain` with dynamic expressions