Renderable exceptions inside views

[ThibaudDauce]

• Laravel Version: 8.17.0
• PHP Version: 7.4.11

Description:

An exception with a render method thrown inside a view is wrapped inside a ErrorException so the render method is not executed by the Laravel's exception handler.

A simple repro repository https://github.com/ThibaudDauce/ignition_bug

See routes/web.php:

Route::get('/', function () {
    // If I throw the exception here, I'm redirected to the /other page.
    // throw new App\Exceptions\TestRenderableException;    

    // If I throw the exception inside the view, the exception page is shown.
    // It's because Laravel wraps all exceptions thrown inside a view within an ErrorException? 
    return view('welcome');
});

Route::get('/other', function () {
    return 'Other page';
});

My TestRenderableException:

<?php

namespace App\Exceptions;

use Exception;

class TestRenderableException extends Exception
{
    public function render()
    {
        return redirect('/other');
    }
}

I think there is the same problem with the report function.

I first though it was an issue with Ignition https://github.com/facade/ignition/issues/339 but it doesn't works even after removing Ignition with composer remove facade/ignition. (the exception is wrapped inside an ErrorException by Laravel instead of a ViewException for Ignition, but it's the same problem)

[taylorotwell]

Hmm, I don't totally recall why that wrapping is done. It doesn't seem like it would always be entirely necessary but maybe it is in certain situations?

[ThibaudDauce]

Maybe keeping the wrapping but forwarding the method calls?

[ThibaudDauce]

@driesvints Do you need more info from my side or is it ok?

[kafkiansky]

@ThibaudDauce, do you have any use cases when thrown an exception inside views is useful? it's very bad idea, views should not throws exceptions, they are needed exclusively for displaying data, that's all.

[ThibaudDauce]

Yes I may call a method from a model that throws an exception. The render and report methods are really useful in this case because in my controllers I can always add a try {} catch {} but it's different inside the view…

[kafkiansky]

@ThibaudDauce, you should not call model's method inside the view, there shouldn't be any logic, only the data output.

[driesvints]

I've discovered that this was implemented by @taylorotwell in 2013 here: bccc3fb

If I look at the commit correctly is seems to be done to include the view path in the exception message. @taylorotwell could that be correct?

[ThibaudDauce]

Maybe we can create a ViewException extends ErrorException which has the two methods render and report and returns null if the inside exception has no render or report (the exception handler checks if method_exists('render') and that the return is not null).

if (method_exists($e, 'render') && $response = $e->render($request)) {
     return Router::toResponse($request, $response);
} elseif ($e instanceof Responsable) {
    return $e->toResponse($request);
}

If the exception inside has a render method, it forwards the call?

[ThibaudDauce]

Something like:

public function render($request)
{
    if (!$this->getPrevious() || !method_exists($this->getPrevious(), 'render')) {
        return null;
    }

    return $this->getPrevious()->render($request);
}

[taylorotwell]

Seems like a possible solution. This probably hasn't come up because it may be kinda rare to trigger renderable exception in views? What is your use case where this is happening?

[ThibaudDauce]

Accounts on my website are linked with customers (relation HasMany), an account without an attached customer throws an exception. I want to redirect to the home page with a notification explaining why the account is not valid.

There are other solutions in my case:

• add an other middleware to all the auth routes to check that an account has a least one customer attached but it adds an SQL request to all the auth routes for a very rare case
• add a check to refuse the login but the last attached customer could be removed during a session (it could also crash when impersonating or other login methods…)

My solution with an exception has the advantage to be lazy with no SQL request until necessary. It also do not require adding more code since I'll throw the exception anyway in case of a "problem" (missing middleware or just to please the IDE and avoiding returning null with $account->customers->first())

I think the middleware could be a solution for the render despite adding the SQL request to all the routes, but I run into this issue so I reported it to know if it's wanted by the framework or if it's something fixable.

The report could be a little bit trickier since developers may asume to receive a custom notification when a particular exception is thrown and not notice that it doesn't happen when this exception is thrown inside a view.

[vdauchy]

@ThibaudDauce Maybe by eager loading your relation you may come arround that issue by throwing the exception earlier.
You may question the automatic throwing of renderable exceptions on relations fetching inside your model too (just a point of view).

[driesvints]

@ThibaudDauce I managed to reproduce this in another app where I was having the exact same issue. But for me I saw the original stacktrack with the exact point where the error was happening when I removed facade/ignition and just used the whoops handler. So to me it does seems related to Ignition? Can you try reproducing this without Ignition?

[ThibaudDauce]

Hi, my repository https://github.com/ThibaudDauce/ignition_bug reproduces the bug without Ignition (the first commit is with Ignition but I tried without in ThibaudDauce/ignition_bug@2611d75 when they told me it was a Laravel problem in my issue https://github.com/facade/ignition/issues/339)

[ThibaudDauce]

The problem is not not sawing the original stacktrace, the problem is not calling the report and render method on the original exception.

[driesvints]

I've finally found time to deep dive into this and managed to send in a PR that should fix this: #36032

I'll report the other issue I discovered from above (that indeed didn't had anything to do with this one) to the Ignition issue tracker.

@ThibaudDauce thanks for reporting this one.