laravel / ideas

Issues board used for Laravel internals discussions.
939 stars 28 forks source link

Don't wrap view exceptions in ErrorException #956

Open ajcastro opened 6 years ago

ajcastro commented 6 years ago

https://github.com/laravel/framework/issues/22784

I think this is an unexpected behavior. I encountered this scenario where I have thrown my custom exception during blade compilation and this custom exception should have its own rendering display page. But since it was thrown during compilation the rendering doesnt work as expected because it was being wrapped by ErrorException because of this line of code https://github.com/laravel/framework/blob/a32b087d8d3fd1085316cb6523b167ca10fb19b0/src/Illuminate/View/Engines/CompilerEngine.php#L77. I tried to comment/disable that code temporarily then I get my expected display page for the custom exception.

My render code in App\Exceptions\Handler::render()

public function render($request, Exception $exception)
    {
       // dd($exception);
        if ($exception instanceof NoDueStartException) {
            return response()->view('errors.no_due_start', [], 422);
        }

        return parent::render($request, $exception);
    }

Sample Screenshot

image

So currently, my workaround is this

public function render($request, Exception $exception)
    {
        if (
            $exception instanceof \ErrorException &&
            $exception->getPrevious()->getPrevious() instanceof NoDueStartException
        ) {
            return response()->view('errors.no_due_start', [], 422);
        }

        return parent::render($request, $exception);
    }

I am using laravel 5.4 on this.

sisve commented 6 years ago

It looks like your blade directive is causing an exception during the generation of php code. Could you add the directive to this issue so we have something to reproduce the problem with?

ajcastro commented 6 years ago

Yes the exception is thrown during view compilation. I created a simpler use-case so I can paste a shorter code here.

Blade file index.blade.php:

{{ request()->user()->exception() }}

App\User model:

public function exception()
{
    throw new \App\Exceptions\MyCustomException();
}

App\Exceptions\MyCustomException:

class MyCustomException extends \Exception
{
}

App\Exceptions\Handler::render() method:

public function render($request, Exception $exception)
{
    dd($exception);
}

Output of dd($exception):

dd

As you can see, the exception is wrapped with ErrorException caused by the code in CompilerEngine line 77.

sisve commented 6 years ago

{{ request()->user()->exception() }} wouldn't call exception() during compilation time, but rather when executing the view. So, it's all exceptions thrown from the views that are the problem, not just at compile time. You could probably reproduce this by just adding a <?php throw new Exception(); ?> in your view.

ajcastro commented 6 years ago

@sisve Im sorry for the misunderstanding, but that's actually what I mean. But supposedly, the App\Exceptions\Handler should be able to handle the desired render output right?

sisve commented 6 years ago

At least we've pinpointed what you're describing and why it happens. Could you update the issue title to remove the reference to the compilation since this happens even when the views are executed? I believe a more clear description would be something like "Don't wrap view exceptions in ErrorException".

ajcastro commented 6 years ago

@sisve Yeah thanks a lot for the help!

tbruckmaier commented 5 years ago

For anyone stumbling upon this, you can get the original exception in your App\Exceptions\Handler::render() quite easily:

    public function render($request, Exception $exception)
    {
        $originalException = $exception;

        while ($exception instanceof \ErrorException) {
            $exception = $exception->getPrevious();
        }

        // any custom handler code

        return parent::render($request, $originalException);
    }
willsmanley commented 4 years ago

Are ErrorExceptions thrown at any point in the framework besides when we choose throw them ourselves?

I want to show $e->getMessage() for the ErrorException class only, but I am afraid that it reveals sensitive debugging information.

I like to use it in controllers like throw new ErrorException('Custom message to display to user');

But I don't want a user accidentally seeing a message that should be for internal use only...