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

Use new exception in PluginsArchiver & add previous exceptions to backtrace in fatal error report #13758

Merged
merged 5 commits into from Nov 27, 2018

Conversation

diosmosis
Copy link
Member

@diosmosis diosmosis commented Nov 24, 2018

Refs #13466

Changes:

  • When wrapping thrown exception, use custom exception class since thrown exception may not have compatible constructor.
  • Add previous exceptions to fatal error info's backtrace property.
  • Recongize previous exceptions in API responses as well (so they will appear in core:archive output).

@diosmosis diosmosis added the Needs Review PRs that need a code review label Nov 24, 2018
@diosmosis diosmosis added this to the 3.8.0 milestone Nov 24, 2018
}

throw $exception;
throw new PluginsArchiverException($e->getMessage() . " - in plugin $pluginName", $e->getCode(), $e);
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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 👍

@diosmosis diosmosis merged commit 307b8b3 into 3.x-dev Nov 27, 2018
@diosmosis diosmosis deleted the segment-exception branch November 27, 2018 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants