Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[TwigBundle] Fixing regression in TwigEngine exception handling#21179
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
[TwigBundle] Fixing regression in TwigEngine exception handling#21179
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| }else { | ||
| $templateName =$e->getTemplateName(); | ||
| } | ||
| $path = (string)$this->locator->locate($this->parser->parse($templateName)); |
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.
the variable $name should be kept
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.
But $name is a parameter that is already in use, and we even checking if$name instanceof TemplateReference. Should we overwrite this variable?
nicolas-grekasJan 6, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
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.
if you want to rename it, you must rename also line 84 now
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.
True, sorry I missed that part, fixed.
nicolas-grekas commentedJan 6, 2017
👍 |
| $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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 introducingsetSourceContext/getSourceContext it is always filled or we should get necessary info based on$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've fixed.
| $path = (string)$this->locator->locate($this->parser->parse($name)); | ||
| if (method_exists($e,'setSourceContext')) { | ||
| $e->setSourceContext(new \Twig_Source('',$name,$path)); | ||
| $e->setSourceContext($e->getSourceContext()); |
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.
even better: do nothing in this case, as it is a no-op now
nicolas-grekas commentedJan 10, 2017
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 | MITFixing regression after#20831 in TwigEngine exception handling.Commits-------390cb33 Fixing regression in TwigEngine exception handling.
Uh oh!
There was an error while loading.Please reload this page.
Fixing regression after#20831 in TwigEngine exception handling.