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

Add meta data interface and trait #574

Closed
wants to merge 1 commit into from
Closed

Conversation

migo315
Copy link

@migo315 migo315 commented Mar 9, 2020

Goal

Provide a very simple way to extend an exception with additional meta data everywhere without further dependencies.

Design

This feature is very pragmatic and straightforward. Similar to PSR Logger, we provide a MetaDataAwareInterface and a MetaDataAwareTrait. The developer can (but must not) extend his Exceptions with the Interface / Trait to add some additional informations:

class UnrecoverableProcessException extends Exception implements MetaDataAwareInterface
{
    use MetaDataAwareTrait;
}

Later just pass your informations:

$exception = new UnrecoverableProcessException('I am an Exception');
$exception->addMetaData(['myTab' => [
    'foo' => 'bar',
]]);

throw $exception;

This implementation does not need any services or heavy structures.

Changeset

Added

  • MetaDataAwareInterface
  • MetaDataAwareTrait

Changed

  • Client add MetaData if Exception implements interface

Linked issues

Original PR: bugsnag/bugsnag-symfony#83

Review

For the submitter, initial self-review:

  • Commented on code changes inline explain the reasoning behind the approach
  • [] Reviewed the test cases added for completeness and possible points for discussion
  • A changelog entry was added for the goal of this pull request
  • Check the scope of the changeset - is everything in the diff required for the pull request?
  • This pull request is ready for:
    • Initial review of the intended approach, not yet feature complete
    • Structural review of the classes, functions, and properties modified
    • Final review

For the pull request reviewer(s), this changeset has been reviewed for:

  • Consistency across platforms for structures or concepts added or modified
  • Consistency between the changeset and the goal stated above
  • Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
  • Usage friction - is the proposed change in usage cumbersome or complicated?
  • Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
  • Concurrency concerns - if components are accessed asynchronously, what issues will arise
  • Thoroughness of added tests and any missing edge cases
  • Idiomatic use of the language

@@ -275,6 +276,10 @@ public function notifyException($throwable, callable $callback = null)
{
$report = Report::fromPHPThrowable($this->config, $throwable);

if ($throwable instanceof MetaDataAwareInterface) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be implemented by a middleware, rather than being baked into the client like this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not that familiar with this SDK. But the middleware doesn't get any exceptions, does it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh, that is correct. Maybe we should change the report object to keep the actual exception around, so people can grab it (by means of a method on the report object that returns the exception or null).

@@ -275,6 +276,10 @@ public function notifyException($throwable, callable $callback = null)
{
$report = Report::fromPHPThrowable($this->config, $throwable);

if ($throwable instanceof MetaDataAwareInterface) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh, that is correct. Maybe we should change the report object to keep the actual exception around, so people can grab it (by means of a method on the report object that returns the exception or null).

@johnkiely1
Copy link
Member

johnkiely1 commented Apr 29, 2020

This has now been fixed via bugsnag-php release V3.21.0
https://github.com/bugsnag/bugsnag-php/releases/tag/v3.21.0.
running composer update bugsnag/bugsnag will pull in latest version

We have added support for the getting the original error on which you can attach additional metadata.

@johnkiely1 johnkiely1 closed this Apr 29, 2020
@johnkiely1 johnkiely1 added feature request Request for a new feature released This feature/bug fix has been released labels Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request for a new feature released This feature/bug fix has been released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants