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

[8.x] Allow reporting reportable exceptions with the default logger #33323

Merged
merged 3 commits into from
Jun 24, 2020

Conversation

netpok
Copy link
Contributor

@netpok netpok commented Jun 24, 2020

Currently there is no way to report a reportable exception with the default handler except manually accessing the logger from the report method (and then the default report context is not available).

This pull request allows this by simply returning false from the report method.

Example:

class MyException extends Exception {
//...
    public function report()
    {
        if ($this->shouldReport()) {
            return false; // this will result in using the original log channels
        }

        $this->doSomethingElse();
    }
//...
}

Furthermore this PR also fixes the following problems in the test:

  • Previously the mockery call count was not defined which defaults to 0 or more, resulting in no calls were actually checked
  • It adds a check that a reportable is not passed to the default handler (except of course intentionally)
  • It adds a missing trait so tests are no longer resulting in "This test did not perform any assertions" warnings.

@taylorotwell taylorotwell merged commit b9cccd7 into laravel:master Jun 24, 2020
@mfn
Copy link
Contributor

mfn commented Jun 24, 2020

@netpok good change but this leaves things in a bit inconsistent state

phpdoc still says void: in the method you changed, the contract and it must also be made sure that the laravel/laravel stub handler has the proper phpdocs

Do you think you can take a second look here?

@GrahamCampbell
Copy link
Member

I'd vote for this to be reverted. The use case is not convincing. Just override the whole report method in your app if you want custom stuff.

@netpok
Copy link
Contributor Author

netpok commented Jun 24, 2020

@mfn The contract and the phpdoc does not need to change, since it only checks the return value of the report method of the exception. The report method on the exception handler does not return anything (just as before).

I will look into the stub tomorrow.

@netpok netpok deleted the feature/report-reportable branch June 24, 2020 20:09
@netpok netpok restored the feature/report-reportable branch June 24, 2020 20:12
taylorotwell pushed a commit that referenced this pull request May 3, 2021
* Backport #33323 to 6.x

* StyleCI fix
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.

4 participants