Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Improved the error message when a template is not found#17434
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
Tobion commentedJan 19, 2016
I'd prefer a real sentence like |
javiereguiluz commentedJan 19, 2016
@Tobion I prefer a real sentence too. I did this to use the same error message as the original FilesystemLoader of Twig library (seehttps://github.com/twigphp/Twig/blob/1.x/lib/Twig/Loader/Filesystem.php#L209). Anyway, if others agree with you, I'll do the change. Thanks! |
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.
parseName() became private intwig master and as I see, this is the problem in travis build too:
Fatal error: Call to private method Twig_Loader_Filesystem::parseName() from contextThere 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 see. In your opinion, what would be the easiest way to overcome this issue then?
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'm not sure what would be a good solution here. One could be to get the exception message from$previous. \Twig_Loader_Filesystem throws exception if there areno registered paths for namespace or whenUnable to find template with the given paths. These exception messages already filled with the$paths and$name, what you'd like to extract here. TwigBundle's FilesystemLoader catch these exceptions andsaves it in a$previous variable. What could be done is just to check, if$previous is not null and get the exception message, ohterwise throw the exception with there original message.
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.
linaori commentedJan 19, 2016
@javiereguiluz is it possible to use the Alias of bundles instead of the full path? I'm mentioning this because of my directory structure. If I were to look into a bundle it would become pretty deep:
And this is with PSR-4 with only 1 vendor bundle. My suggestion would be to make an exception to /app and alias the rest with the bundle name:
It's quite clear in which application/project you are working with the given error so that information is redundant. Given you hardly ever use vendor templates, the last case would hardly ever be useful but cause a lot of clutter. The AppBundle shouldn't contain templates according to the best-practices but I think enough people still use multiple bundles in their application and know how to find it with the alias. |
javiereguiluz commentedJan 27, 2016
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.
At this point, why not just rethrow the previous exception if it is aTwig_Error_loader exception?throw $previous would do the same, no?
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.
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.
Not sure I understand. Instead of storing$errorMessage = $e->getMessage();, why not store the exception itself and throw it here?
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 are right. I tried to maintain the existing code ... but it doesn't make sense. Updated. Thanks!
fabpot commentedMar 2, 2016
Now that the patch is much simpler, I don't get why we would need to change this. Basically, you are hiding one exception that might occur and the exception you put front and center is anyway available in the linked exceptions. So, I don't get it. Are you saying that people don't look at linked exceptions? |
javiereguiluz commentedMar 3, 2016
Let me explain why I think we should change this: If your application uses the modern template syntax and put this in your controller: return$this->render('wrong_template.html.twig'); Symfony will show you this message: If we apply this patch, this other message is displayed: If your application uses the old template syntax and put this in your controller: return$this->render('AppBundle:Default:wrong_template.html.twig'); Symfony will show you this message: (the four chained exceptions don't provide any information) If we apply this patch, this other message is displayed: And if your application uses the namespace template syntax and put this in your controller: return$this->render('@App/Default/wrong_template.html.twig'); Symfony will show you this message: (everything is very misleading! The second exception says that AppBundle doesn't exist!!!) If we apply this patch, this other message is displayed: So, it seems that the new error messages are more useful and always consistent, regardless of the template syntax used. But let's ping@nicolas-grekas to check if he sees something wrong in this proposed exception change. |
fabpot commentedMar 3, 2016
Thank you@javiereguiluz. |
…vanginneken, javiereguiluz)This PR was merged into the 2.3 branch.Discussion----------Improved the error message when a template is not found| Q | A| ------------- | ---| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | no| Fixed tickets |#14806| License | MIT| Doc PR | -### Before### AfterThis seems to work in the browser ... but I can't make tests pass. Could anybody please help me? Thanks!Commits-------0134d76 Simplified everything19bfa2e Added a test88b913b Fixed the problem in an easier way35f082f Fixed a syntax issue968476b Improved the error message when a template is not founde9d951a [CodingStandards] Conformed to coding standardsd3fe07b [TwigBundle] fixed Include file locations in "Template could not be found" exception
fabpot commentedMar 3, 2016
Thanks@javiereguiluz for the very detailed analysis. It looks like it is indeed always better to use the new code. |
This PR was merged into the 2.3 branch.Discussion----------[TwigBundle] Fix failing test on appveyor| Q | A| ------------- | ---| Branch | 2.3| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | no| Fixed tickets |#17434| License | MIT| Doc PR | -Commits-------d5bbc3c [TwigBundle] Fix failing test on appveyor








Before
After
This seems to work in the browser ... but I can't make tests pass. Could anybody please help me? Thanks!