[TwigBundle] Fixing regression in TwigEngine exception handling #21179
Conversation
| } else { | ||
| $templateName = $e->getTemplateName(); | ||
| } | ||
| $path = (string) $this->locator->locate($this->parser->parse($templateName)); |
There was a problem hiding this comment.
the variable $name should be kept
There was a problem hiding this comment.
But $name is a parameter that is already in use, and we even checking if $name instanceof TemplateReference. Should we overwrite this variable?
There was a problem hiding this comment.
if you want to rename it, you must rename also line 84 now
There was a problem hiding this comment.
True, sorry I missed that part, fixed.
|
👍 |
| $name = $e->getTemplateName(); | ||
| $path = (string) $this->locator->locate($this->parser->parse($name)); | ||
| if (method_exists($e, 'getSourceContext')) { | ||
| $templateName = $e->getSourceContext()->getName(); |
There was a problem hiding this comment.
@nicolas-grekas @attila901 maybe $path could be also get from the context with $e->getSourceContext()->getPath() in the else $path could be set the old way.
There was a problem hiding this comment.
Thank you @burci , I've fixed.
| if (method_exists($e, 'setSourceContext')) { | ||
| $e->setSourceContext(new \Twig_Source('', $name, $path)); | ||
| $e->setSourceContext(new \Twig_Source('', $templateName, $path)); |
There was a problem hiding this comment.
why not using $e->getSourceContext() directly here, instead of creating a new one with the same info ? Existence of the getter and setter go together (they have been introduced together)
There was a problem hiding this comment.
After this @attila901 will end up something like this:
if (method_exists($e, 'setSourceContext')) {
$e->setSourceContext($e->getSourceContext());
} else {
$templateName = $e->getTemplateName();
$path = (string) $this->locator->locate($this->parser->parse($templateName));
$e->setTemplateName($path);
}The $e->setSourceContext($e->getSourceContext()); part can be skiped and we are not using the $name variable at all. Maybe after introducing setSourceContext/getSourceContext it is always filled or we should get necessary info based on $name?
| if (method_exists($e, 'setSourceContext')) { | ||
| $e->setSourceContext(new \Twig_Source('', $name, $path)); | ||
| $e->setSourceContext($e->getSourceContext()); |
There was a problem hiding this comment.
even better: do nothing in this case, as it is a no-op now
|
Thank you @attila901. |
…dling (Bertalan Attila) This PR was merged into the 2.7 branch. Discussion ---------- [TwigBundle] Fixing regression in TwigEngine exception handling | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21176 | License | MIT Fixing regression after #20831 in TwigEngine exception handling. Commits ------- 390cb33 Fixing regression in TwigEngine exception handling.
Fixing regression after #20831 in TwigEngine exception handling.