Improved the error message when a template is not found #17434

javiereguiluz · 2016-01-19T09:08:53Z

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	no
Fixed tickets	#14806
License	MIT
Doc PR	-

Before

After

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

…ound" exception

Tobion · 2016-01-19T09:34:27Z

I'd prefer a real sentence like Unable to find template "..." in any of these locations: ...

javiereguiluz · 2016-01-19T09:37:59Z

@Tobion I prefer a real sentence too. I did this to use the same error message as the original FilesystemLoader of Twig library (see https://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!

paradajozsef · 2016-01-19T10:06:40Z

src/Symfony/Bundle/TwigBundle/Loader/FilesystemLoader.php

@@ -89,7 +89,14 @@ protected function findTemplate($template, $throw = true)
        }

        if (false === $file || null === $file) {
-            throw new \Twig_Error_Loader(sprintf('Unable to find template "%s".', $logicalName), -1, null, $previous);
+            try {
+                list($namespace, $name) = $this->parseName($logicalName);


parseName() became private in twig master and as I see, this is the problem in travis build too:

Fatal error: Call to private method Twig_Loader_Filesystem::parseName() from context

I see. In your opinion, what would be the easiest way to overcome this issue then?

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 are no registered paths for namespace or when Unable 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 and saves 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.

Maybe the exception's message what we catch here should be thrown, and this should be the $previous

linaori · 2016-01-19T13:40:58Z

@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:

/home/ivanderberg/projects/my_project/www/app/Resources/views/
/home/ivanderberg/projects/my_project/www/src/Hostnet/Bundle/AppBundle/Resources/views/
/home/ivanderberg/projects/my_project/www/vendor/hostnet/some-bundle/src/Resources/views/

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:

/app/Resources/views/
@AppBundle/Resources/views/
@SomeBundle/Resources/views/

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 · 2016-01-27T16:53:09Z

In 88b913b I'm trying to solve the problem just by reusing the error message that Twig provides us. The result is the same:

@iltar I prefer to not show the namespaces. Although is more verbose, is easier to understand which real paths Symfony is looking into.

Now I'll try to add some test.

fabpot · 2016-01-27T17:00:12Z

src/Symfony/Bundle/TwigBundle/Loader/FilesystemLoader.php

@@ -89,7 +90,7 @@ protected function findTemplate($template, $throw = true)
        }

        if (false === $file || null === $file) {
-            throw new \Twig_Error_Loader(sprintf('Unable to find template "%s".', $logicalName), -1, null, $previous);
+            throw new \Twig_Error_Loader($errorMessage, -1, null, $previous);


At this point, why not just rethrow the previous exception if it is a Twig_Error_loader exception? throw $previous would do the same, no?

The problem is that there are two $previous. The first one is "the good one" ... but we always override it. If I use throw $previous the error message is worse:

Not sure I understand. Instead of storing $errorMessage = $e->getMessage();, why not store the exception itself and throw it here?

You are right. I tried to maintain the existing code ... but it doesn't make sense. Updated. Thanks!

fabpot · 2016-03-02T12:55:28Z

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 · 2016-03-03T14:24:50Z

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 · 2016-03-03T14:31:33Z

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 ![template_error_before](https://cloud.githubusercontent.com/assets/73419/12414237/7db29212-be94-11e5-85b4-c6444aa853f8.png) ### After ![template_error_after](https://cloud.githubusercontent.com/assets/73419/12414242/80ed1eca-be94-11e5-992e-a0596a1e95ca.png) This seems to work in the browser ... but I can't make tests pass. Could anybody please help me? Thanks! Commits ------- 0134d76 Simplified everything 19bfa2e Added a test 88b913b Fixed the problem in an easier way 35f082f Fixed a syntax issue 968476b Improved the error message when a template is not found e9d951a [CodingStandards] Conformed to coding standards d3fe07b [TwigBundle] fixed Include file locations in "Template could not be found" exception

fabpot · 2016-03-03T14:31:42Z

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

rvanginneken and others added 3 commits January 19, 2016 10:04

[TwigBundle] fixed Include file locations in "Template could not be f…

d3fe07b

…ound" exception

[CodingStandards] Conformed to coding standards

e9d951a

Improved the error message when a template is not found

968476b

carsonbot added the Status: Needs Review label Jan 19, 2016

javiereguiluz mentioned this pull request Jan 19, 2016

[TwigBundle] fixed Include file locations in "Template could not be f… #16202

Closed

Fixed a syntax issue

35f082f

paradajozsef reviewed Jan 19, 2016
View reviewed changes

Fixed the problem in an easier way

88b913b

fabpot reviewed Jan 27, 2016
View reviewed changes

Added a test

19bfa2e

javiereguiluz added Enhancement TwigBundle labels Feb 11, 2016

Simplified everything

0134d76

fabpot merged commit 0134d76 into symfony:2.3 Mar 3, 2016

nicolas-grekas mentioned this pull request Mar 3, 2016

[TwigBundle] Fix failing test on appveyor #18002

Merged

fabpot mentioned this pull request Mar 13, 2016

Release v2.3.39 #18153

Merged

This was referenced Mar 25, 2016

Release v2.7.11 #18315

Merged

Release v2.8.4 #18327

Closed

Release v2.8.4 #18329

Merged

Release v3.0.4 #18363

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved the error message when a template is not found #17434

Improved the error message when a template is not found #17434

javiereguiluz commented Jan 19, 2016

Tobion commented Jan 19, 2016

javiereguiluz commented Jan 19, 2016

paradajozsef Jan 19, 2016

javiereguiluz Jan 19, 2016

paradajozsef Jan 19, 2016

paradajozsef Jan 19, 2016

linaori commented Jan 19, 2016

javiereguiluz commented Jan 27, 2016

fabpot Jan 27, 2016

javiereguiluz Jan 27, 2016

fabpot Feb 29, 2016

javiereguiluz Mar 1, 2016

fabpot commented Mar 2, 2016

javiereguiluz commented Mar 3, 2016

fabpot commented Mar 3, 2016

fabpot commented Mar 3, 2016

Improved the error message when a template is not found #17434

Improved the error message when a template is not found #17434

Conversation

javiereguiluz commented Jan 19, 2016

Before

After

Tobion commented Jan 19, 2016

javiereguiluz commented Jan 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linaori commented Jan 19, 2016

javiereguiluz commented Jan 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Mar 2, 2016

javiereguiluz commented Mar 3, 2016

fabpot commented Mar 3, 2016

fabpot commented Mar 3, 2016