Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[TwigBridge][TwigBundle][HttpKernel] prefer getSourceContext() over getSource()#20440
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 commentedNov 7, 2016
| Q | A |
|---|---|
| Branch? | 2.7 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #20435 |
| License | MIT |
| Doc PR | n/a |
| $loader =$this->environment->getLoader(); | ||
| if ($loaderinstanceof \Twig_ExistsLoaderInterface) { | ||
| if ($loaderinstanceof \Twig_ExistsLoaderInterface ||method_exists($loader,'exists')) { |
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.
In Twig 2.0, theexists() method will be present in theTwig_LoaderInterface.
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.
This is not needed asTwig_ExistsLoaderInterface still exist in 2.0. It will be removed in 3.0 only.
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 call toexists() will never be made though if the loader does not implementTwig_ExistsLoaderInterface, but is just an instance ofTwig_LoaderInterface.
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.
indeed
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.
And this seems to happen. Otherwise, I don't see how#20435 would ever reach thegetSource() call.
| $loader->getSourceContext($template); | ||
| }else { | ||
| $loader->getSource($template); | ||
| } |
FrancescoBorziNov 7, 2016 • 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.
I would suggest:... } else if (method_exists($loader, 'getSource')) { ... instead of line150
| try { | ||
| $loader->getSource($template); | ||
| if (method_exists($loader,'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.
👍 as HttpKernel does not constrain Twig version.
xabbuh commentedNov 7, 2016
fabbot failures are unrelated. |
fabpot commentedNov 7, 2016
Thank you@xabbuh. |
…xt() over getSource() (xabbuh)This PR was merged into the 2.7 branch.Discussion----------[TwigBridge][TwigBundle][HttpKernel] prefer getSourceContext() over getSource()| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#20435| License | MIT| Doc PR | n/aCommits-------adbc529 prefer getSourceContext() over getSource()
This PR was merged into the 1.x branch.Discussion----------add machine-readable version constantsThis will make it easier to implement version depending features (seesymfony/symfony#20440 (comment) for an example).Commits-------bf07db4 add machine-readable version constants
FrancescoBorzi commentedNov 7, 2016
good job@xabbuh |