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

ViewExceptions will never be logged #36109

Closed
imjoehaines opened this issue Feb 1, 2021 · 4 comments · Fixed by #36110
Closed

ViewExceptions will never be logged #36109

imjoehaines opened this issue Feb 1, 2021 · 4 comments · Fixed by #36110
Labels

Comments

@imjoehaines
Copy link

  • Laravel Version: 6.20.15 / 8.25.0 / 9.x (8076d86)
  • PHP Version: N/A
  • Database Driver & Version: N/A

Description:

The ViewException class introduced in #36032 will never be logged because its report method always returns null. Any return value other than false will suppress logging, so no exceptions thrown in views will be logged

ViewException also doesn't return the wrapped exception's report value, so wrapped exceptions cannot change the return value

Steps To Reproduce:

  1. Create a new Laravel project
  2. Remove Ignition & Collision (composer remove facade/ignition nunomaduro/collision) [6.x & 8.x only]
  3. Throw an exception in a view
  4. The log file is empty
@driesvints
Copy link
Member

The return value of the report method is always void so I don't see the issue here?

@imjoehaines
Copy link
Author

The return value of the report method is always void so I don't see the issue here?

The return value can be false, which signals that the standard error reporting should happen:

If your exception contains custom reporting logic that is only necessary when certain conditions are met, you may need to instruct Laravel to sometimes report the exception using the default exception handling configuration. To accomplish this, you may return false from the exception's report method:
https://laravel.com/docs/8.x/errors#renderable-exceptions

The handler explicitly checks for false before aborting here: https://github.com/laravel/framework/blob/8.x/src/Illuminate/Foundation/Exceptions/Handler.php#L216-L220

@driesvints
Copy link
Member

Ah it seems that our docblocks are incorrect then. Thanks for letting us know. I'll get that also fixed.

@markkimsal
Copy link

ViewExceptions will still never be logged in Laravel 6 because the default Exception/Handler does not check nor react to return values of exception report() methods. The notion that:

The return value can be false, which signals that the standard error reporting should happen:

Is nowhere to be found in src/Illuminate/Foundation/Exceptions/Handler.php up to v6.20.22

    public function report(Exception $e)
    {

        if ($this->shouldntReport($e)) {
            return;
        }

        if (Reflector::isCallable($reportCallable = [$e, 'report'])) {
            return $this->container->call($reportCallable);            // does not check for return value === false 
        }

        // ViewExceptions will never reach here because they container a report method.
        try {
            $logger = $this->container->make(LoggerInterface::class);
        } catch (Exception $ex) {
            throw $e;
        }

return $this->container->call($reportCallable);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants