Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[VarDumper] Fix source links with latests Twig versions#20175
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
695c807 tocf93b79Comparefabpot commentedOct 17, 2016
Closing in favor of#20224 |
cdaa6dc to6401a33Compare| $templateInfo =$template->getDebugInfo(); | ||
| if (isset($templateInfo[$f['line']])) { | ||
| if (method_exists($template,'getSourceContext')) { | ||
| $templateName =$template->getSourceContext()->getPath(); |
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.
you should overwrite it only when the path is not null (not all loaders can give you a path, as not all of them are based on a file)
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.
fixed
| $templateInfo =$template->getDebugInfo(); | ||
| if (isset($templateInfo[$f['line']])) { | ||
| $templateName =$template->getTemplateName(); | ||
| $templateSrc =method_exists($template,'getSourceContext') ?$template->getSourceContext()->getCode() : (method_exists($template,'getSource') ?$template->getSource() :false); |
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 would use the empty string rather thanfalse, to have a consistent variable type (getCode() will return the empty string in non-debug mode anyway)
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.
fixed
| twig source | ||
| "; | ||
| returnnewTwig_Source(" foo bar\n twig source\n\n",'foo.twig','foo.twig'); |
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.
Please also add tests for other cases:
- Twig_Source with an empty code string (i.e. a non-debug compilation)
- Twig_Source without a path (i.e. a loader not based on the filesystem, or a loader not updated to use new features)
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.
Bof, that's only a caster, adding a test for that would be quite a bit of work for a minor thing to me...
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.
well, it would avoid issues when refactoring it, reintroducing the bugs I spotted during the review
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.
tests added
d71fcd9 to888e4bdCompare…icolas-grekas)This PR was merged into the 2.8 branch.Discussion----------[VarDumper] Fix source links with latests Twig versions| Q | A| ------------- | ---| Branch? | 2.8| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Sibling to#20173Commits-------f3b09d9 [VarDumper] Fix source links with latests Twig versions
Sibling to#20173