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] Enhance the twig not found exception#27535
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] Enhance the twig not found exception#27535
Uh oh!
There was an error while loading.Please reload this page.
Conversation
de2c952 tob7af45bCompare| try { | ||
| return parent::findTemplate($template, $throw); | ||
| } catch (LoaderError $e) { | ||
| if ($e instanceof \Twig_Error_Loader && preg_match('/^([^:]*?)(?:Bundle)?:([^:]*+):(.+\.[^\.]+\.[^\.]+)$/', $template, $m) && 0 !== strpos($template, '@')) { |
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.
theinstanceof will always be true.Twig\Error\LoaderError is a class alias ofTwig_Error_Loader.
| try { | ||
| return parent::findTemplate($template, $throw); | ||
| } catch (LoaderError $e) { | ||
| if ($e instanceof \Twig_Error_Loader && preg_match('/^([^:]*?)(?:Bundle)?:([^:]*+):(.+\.[^\.]+\.[^\.]+)$/', $template, $m) && 0 !== strpos($template, '@')) { |
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 switch the regex match in thestrpos. Checking the first char is faster.
Btw, you could optimize it even more:
if ('' !==$template &&'@' !==$template[0] &&preg_match(...)) {
Using strpos to check only the first char is not the fastest way.
c832758 to82d1db4Compare| class NativeFilesystemLoaderTest extends TestCase | ||
| { | ||
| const DS = DIRECTORY_SEPARATOR; |
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 remove this const and use the PHP one instead.
| try { | ||
| return parent::findTemplate($template, $throw); | ||
| } catch (LoaderError $e) { | ||
| if ('' === $template || '@' === $template[0] || !preg_match('/^([^:]*?)(?:Bundle)?:([^:]*+):(.+\.[^\.]+\.[^\.]+)$/', $template, $m)) { |
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.
Can we use named patterns to make the code using$m more easily understandable?
8358ca7 toa3b665cComparebehnoushnorouzi commentedJun 8, 2018 • 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.
Thank you for the review it's done. Status: needs review |
| use Twig\Error\LoaderError; | ||
| use Twig\Loader\FilesystemLoader; | ||
| class NativeFilesystemLoader extends FilesystemLoader |
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 may want to add yourself as the author of this class :)
a3b665c toa5a4d48CompareLewiscowles1986 commentedJun 9, 2018
Should this emit a notice or warning if used in production? It seems like a development pattern, and can be switched out for the extended class. Once the suggestion is implemented it seems like switching to the standard class might be more helpful. |
nicolas-grekas commentedJun 9, 2018 • 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.
@Lewiscowles1986 good idea, we could use the new class only in debug mode. Could be done in |
stof commentedJun 14, 2018
We don't even need to do it in a compiler pass. The debug parameter can be accessed in the DI extension (the |
a5e0738 to554b0b1CompareEnhance the twig not found exception
554b0b1 to32988b4Comparebehnoushnorouzi commentedJun 14, 2018
Thank you for the review it's done. |
nicolas-grekas commentedJun 19, 2018
Thank you@behnoushnorouzi. |
…noushnorouzi)This PR was merged into the 4.2-dev branch.Discussion----------[TwigBundle] Enhance the twig not found exception| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#27468| License | MIT| Doc PR | -Improve error message to make it clear to developers what mistake they have made.Commits-------32988b4 Enhance the twig not found exception
Uh oh!
There was an error while loading.Please reload this page.
Improve error message to make it clear to developers what mistake they have made.