Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
fabpot merged 7 commits intosymfony:2.3fromjaviereguiluz:fix_14806
Mar 3, 2016

Conversation

@javiereguiluz
Copy link
Member

QA
Bug fix?no
New feature?no
BC breaks?no
Deprecations?no
Tests pass?no
Fixed tickets#14806
LicenseMIT
Doc PR-

Before

template_error_before

After

template_error_after

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

@Tobion
Copy link
Contributor

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

@javiereguiluz
Copy link
MemberAuthor

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

Copy link
Contributor

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 context

Copy link
MemberAuthor

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?

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Maybe the exception's message what we catchhere should be thrown, andthis should be the$previous

@linaori
Copy link
Contributor

@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
Copy link
MemberAuthor

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

wrong_template

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

Copy link
Member

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?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

worse_error_message

Copy link
Member

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?

Copy link
MemberAuthor

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
Copy link
Member

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
Copy link
MemberAuthor

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:

error_before

If we apply this patch, this other message is displayed:

error_after


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)

old_error_before

If we apply this patch, this other message is displayed:

old_error_after


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!!!)

namespace_before

If we apply this patch, this other message is displayed:

namespace_after


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
Copy link
Member

Thank you@javiereguiluz.

@fabpotfabpot merged commit0134d76 intosymfony:2.3Mar 3, 2016
fabpot added a commit that referenced this pull requestMar 3, 2016
…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 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
Copy link
Member

Thanks@javiereguiluz for the very detailed analysis. It looks like it is indeed always better to use the new code.

nicolas-grekas added a commit that referenced this pull requestMar 3, 2016
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
@fabpotfabpot mentioned this pull requestMar 13, 2016
This was referencedMar 25, 2016
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@javiereguiluz@Tobion@linaori@fabpot@paradajozsef@carsonbot@rvanginneken

[8]ページ先頭

©2009-2025 Movatter.jp