Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DX] Misleading error when the namespace of a controller is wrong #14073

Closed
javiereguiluz opened this issue Mar 26, 2015 · 9 comments
Closed

Comments

@javiereguiluz
Copy link
Member

The problem

The other day I was playing with the Symfony Demo application, in which the routing is simply defined as:

# app/config/routing.yml
app:
    resource: @AppBundle/Controller/
    type:     annotation

I had a BlogController.php file with @Route() annotations in src/AppBundle/Controller/BlogController.php file.

Then, I moved this controller to the src/AppBundle/Controller/Admin/ folder without updating the namespace of the PHP class. Then, the application displayed the following error message:

FileLoaderImportCircularReferenceException in FileLoader.php line 97:
Circular reference detected in "/Users/javier/sf/symfony-demo/app/config/routing_dev.yml" 
("/Users/javier/sf/symfony-demo/app/config/routing_dev.yml" >
"/Users/javier/sf/symfony-demo/app/config/routing.yml" >
"/Users/javier/sf/symfony-demo/src/AppBundle/Controller/" >
"/Users/javier/sf/symfony-demo/app/config/routing_dev.yml").

Sadly this error message is very misleading because CircularReferenceException message looks scary and the error keeps talking about the routing_dev.yml file, which has nothing to do with the actual error.

The solution

Symfony already includes some great error messages when you are warned about typos and PHP classes mislocations. I'd like to see a better error message when the namespace of a controller file is not correct.

@javiereguiluz javiereguiluz changed the title Misleading error when the namespace of a controller is wrong [DX] Misleading error when the namespace of a controller is wrong Mar 26, 2015
@stof
Copy link
Member

stof commented Mar 27, 2015

why does AppBundle/Controller try to load routing_dev.yml again when the namespace is wrong ? this does not make any sense

@javiereguiluz
Copy link
Member Author

@stof I know it's very strange. By the way, yesterday I was contacted by a Symfony developer which suffered the exact same problem and for the same reasons (moving a controller without updating its namespace).

@liutec
Copy link

liutec commented Apr 3, 2015

Possibly a problem with re-throwing exceptions in FileLoader

My steps to reproduce are:

  1. new symfony project (2.6.6 or 2.6.5) - no demo bundle, hard assets
  2. edit app/config/routing.yml and change @AppBundle/Controller/ to @AppBundleX/Controller/
  3. http://localhost/app_dev.php/app/example

I was unable to reproduce the problem following the steps above for Symfony 2.6.4

@liutec
Copy link

liutec commented Apr 3, 2015

I've found this commit 0ebcf63 to have generated the problem.
Specifically throw $e; instead of just returning from src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php onKernelException when exception thrown during handler.

@nicolas-grekas
Copy link
Member

Does #14192 enhance the situation? As stated in 0ebcf63#commitcomment-10558553, the issue is not in this commit, but in the error handling strategy of the route loader. A circular exception is just plain wrong.

@javiereguiluz
Copy link
Member Author

@nicolas-grekas it's better ... but it's not good enough yet :)

After applying your changes, I see 4 exceptions now. However, I see first the "wrong exceptions". The best exception in this case would be the number 2 (and possibly the number 1 too):

exceptions

@nicolas-grekas
Copy link
Member

So, HttpKernel is at the maximum that is legitimate to do: error reporting does not hide anything so the situation can be handled by reading the page.
If we need more, then we need to rethink the error handling part of the route loader.

@liutec
Copy link

liutec commented Apr 3, 2015

#14192 does the trick and saves a lot of debugging. Thx nicholas.

@stof
Copy link
Member

stof commented Apr 3, 2015

@nicolas-grekas the first exception in this case is wrong: the exception happened too early in the Symfony lifecycle to even give a chance to the kernel to throw the kernel.exception event

fabpot added a commit that referenced this issue Apr 3, 2015
…to bounced exceptions (nicolas-grekas)

This PR was merged into the 2.7 branch.

Discussion
----------

[HttpKernel] Embed the original exception as previous to bounced exceptions

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

Commits
-------

bb020f4 [HttpKernel] Embed the original exception as previous to bounced exceptions
@fabpot fabpot closed this as completed Apr 3, 2015
nicolas-grekas added a commit that referenced this issue Apr 3, 2015
…ounced exceptions (nicolas-grekas)

This PR was merged into the 2.3 branch.

Discussion
----------

[HttpKernel] Embed the original exception as previous to bounced exceptions

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

Commits
-------

[HttpKernel] Embed the original exception as previous to bounced exceptions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants