feature(pipeline): add support for registering middleware - #478
Conversation
- Added register middleware method - Added test to ensure middleware is correctly inserted into the pipeline
- Moved severity integrity logic into CallbackBridge - Added tests for severity integrity
- Add test to ensure middleware can override severity/unhandled
kattrali
changed the title
kattrali
left a comment
kattrali
left a comment
There was a problem hiding this comment.
Looks good so far. Could use a bit more tests around the interactions between CallbackBridge and custom middleware.
Tests
Added unit tests
Existing tests
- Custom middleware can modify metadata
- Custom middleware can modify severity
Additions
- Custom middleware can modify unhandled state and severity reason
- When a custom middleware sets the unhandled state, a callback cannot override it
- When a custom middleware sets the severity and severity is later changed by a callback then the severity reason is
userCallbackSetSeverity
| */ | ||
| public function registerMiddleware(callable $middleware) | ||
| { | ||
| if (is_callable($middleware)) { |
There was a problem hiding this comment.
Don't need to check this. The type annotation already raises an error.
- Add tests to ensure callbacks cannot override middleware-set severity reason and unhandled
| if (is_callable($middleware)) { | ||
| $this->pipeline->pipe($middleware); | ||
| } else { | ||
| syslog(LOG_WARNING, 'Middleware '.get_class($middleware).' could not be added to the pipeline'); |
There was a problem hiding this comment.
This code is not reachable, and should be deleted.
There was a problem hiding this comment.
You're correct, I was just verifying the earlier versions of PHP had the same behaviour
There was a problem hiding this comment.
They should do. Callable was introduced in php 5.4 as a typehint, and the behavior has remained the same since.
There was a problem hiding this comment.
I'm not sure what kind of error is thrown in earlier versions of PHP, in PHP 7 it's quite explicitly a TypeError, any idea for PHP 5.6?
There was a problem hiding this comment.
- Remove callable check and add test
…bugsnag-php into cawllec/add-register-middleware
- Remove erronous test
…bugsnag-php into cawllec/add-register-middleware
3 participants
Goal
This change adds a
registerMiddlewaremethod to theClientclass that allows middleware to be directly added to the pipeline. It also shifts the consistency checks onunhandledandseverityto theCallbackBridgemiddleware, allowing registered middleware to modify these elements.Design
Interface changes:
public function registerMiddleware(callable $middleware)Logic changes:
Wrapped given callback in
notifyin aCallbackBridge, complicating logic around calling it. Any feedback on that would be appreciated.Changeset
Added
registerMiddlewaremethodChanged
Moved severity consistency checks to the
CallbackBridgemiddlewareTests
Added unit tests
Discussion
I'm not especially happy with the callback logic changed in
notify, so any suggestions on how to neaten/improve on that would be appreciated.