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

[6.x] Backport #33323 to 6.x #37235

Merged
merged 2 commits into from
May 3, 2021
Merged

[6.x] Backport #33323 to 6.x #37235

merged 2 commits into from
May 3, 2021

Conversation

rodrigopedra
Copy link
Contributor

@rodrigopedra rodrigopedra commented May 3, 2021

This PR is a backport from PR #33323 to the 6.x branch

The goal is to allow to report a reportable exception with the default handler by simply returning false from the report method.

The backport suggestion was made by issue #37213 where the issue's author noted if an error happens when rendering a blade view, and no dev dependencies are installed (e.g. in production), the error does not get logged to the log file.

I tested in a local installation and now it does.

One difference from PR #33323 is that in Laravel 6.x, a value returned from a reportable exception's report method is returned:

if (Reflector::isCallable($reportCallable = [$e, 'report'])) {
    return $this->container->call($reportCallable);
}

So to avoid breaking changes for developers expecting such behavior, I still return the returned value, unless it is false

if (Reflector::isCallable($reportCallable = [$e, 'report'])) {
    if (($response = $this->container->call($reportCallable)) !== false) {
        return $response;
    }
}

If the developer is manually calling the Exception handler report method and expecting false, this would be a breaking change.

But as the docblock states the report method return value should be void, I guess it is very unlikely someone is using the report handler this way.

Let me know if you prefer to change it to be the same as #33323 and I will change the PR to it.


Closes #37213

@GrahamCampbell
Copy link
Member

GrahamCampbell commented May 3, 2021

👎 6.x is closed for new features.

@rodrigopedra
Copy link
Contributor Author

I understand that. Sent this PR based on this comment by @taylorotwell

#37213 (comment)

@taylorotwell taylorotwell merged commit eaea47a into laravel:6.x May 3, 2021
@rodrigopedra rodrigopedra deleted the 6.x branch May 3, 2021 19:07
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

Successfully merging this pull request may close these issues.

3 participants