Skip to content
This repository was archived by the owner on Oct 1, 2024. It is now read-only.

Conversation

@gfaza
Copy link

@gfaza gfaza commented Apr 3, 2017

attn @sfgeorge,

abandoned original idea of passing the exception into the notification. as it turns out, the notification is meant to mimic an Exception, so made more sense to pass the error code instead. this way we'd simply use the getCode method that the Exception interface expects.

@gfaza gfaza force-pushed the feature/DTPORTAL-2738-implementing_Airbrake_for_CRBN branch from 47ad49b to 7e7e860 Compare April 3, 2017 17:34
return;
}
$exc = new Errors\Fatal($error['message'], $error['file'], $error['line']);
$exc = new Errors\Fatal($error['message'], $error['file'], $error['line'], $trace = array(), $error['type']);
Copy link

Choose a reason for hiding this comment

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

Thanks for doing this, man.

Could you also update onError() such that each of the Base objects that it creates is given the $code as the 5th argument to the constructor?

Copy link
Author

@gfaza gfaza Apr 3, 2017

Choose a reason for hiding this comment

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

'tis done

@gfaza gfaza force-pushed the feature/DTPORTAL-2738-implementing_Airbrake_for_CRBN branch from 7e7e860 to a3b93de Compare April 3, 2017 21:16
Copy link

@sfgeorge sfgeorge left a comment

Choose a reason for hiding this comment

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

This looks good, thank you mate! 👍

I suggest we merge PR #2 first to make the test suite happy, just so that we don't get a false-positive and make it look like this PR broke something that it didn't

@sfgeorge
Copy link

sfgeorge commented Apr 4, 2017

@ajayanty Can you merge this after merging #2?

@ajayanty ajayanty merged commit fd61842 into cloudvox:support/v0.0.7.x Apr 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants