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
Use new exception in PluginsArchiver & add previous exceptions to backtrace in fatal error report #13758
Conversation
…wn exception may not have compatible constructor.
} | ||
|
||
throw $exception; | ||
throw new PluginsArchiverException($e->getMessage() . " - in plugin $pluginName", $e->getCode(), $e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose that should work. Although wondering if anything may trigger like an NoAccessException
which should maybe show login page etc. I don't think it's an actual issue though but maybe could be. Not sure if we handle some other exceptions in any specific way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could use reflection to change the $message
property of the exception. Not sure how future proof that is, but I suppose the tests will tell us quickly if it breaks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could work as well I suppose. I reckon it'd be just like https://stackoverflow.com/a/29542873
Although I reckon the current approach is fine as well, just not sure if any specific exceptions would be treated differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tsteur Does that mean this PR is ok to merge or do you want to think about it some more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reckon it should be fine 👍
Refs #13466
Changes: