-
Notifications
You must be signed in to change notification settings - Fork 6
Add support for an optional onError callback #17
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
Changes from all commits
895c084
a3b9214
5f8a29b
725959b
3f960b6
1932dda
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,20 +14,29 @@ | |
| use Psr\Http\Message\ServerRequestInterface; | ||
| use Psr\Http\Server\MiddlewareInterface; | ||
| use Psr\Http\Server\RequestHandlerInterface; | ||
| use Psr\Log\LoggerInterface; | ||
| use Throwable; | ||
|
|
||
| class ErrorHandler implements MiddlewareInterface | ||
| { | ||
| /** @var FormatterInterface[] */ | ||
| private $formatters = []; | ||
|
|
||
| /** @var LoggerInterface|null */ | ||
| private $logger = null; | ||
|
|
||
| /** @var callable|null */ | ||
| private $logCallback = null; | ||
|
|
||
| /** | ||
| * Configure the error formatters | ||
| * | ||
| * @param FormatterInterface[] $formatters | ||
| */ | ||
| public function __construct(?array $formatters = null) | ||
| public function __construct(?array $formatters = null, ?LoggerInterface $logger = null) | ||
| { | ||
| $this->logger = $logger; | ||
|
|
||
| if (empty($formatters)) { | ||
| $formatters = [ | ||
| new PlainFormatter(), | ||
|
|
@@ -54,11 +63,33 @@ public function addFormatters(FormatterInterface ...$formatters): self | |
| return $this; | ||
| } | ||
|
|
||
| /** | ||
| * Set a custom log callback | ||
| */ | ||
| public function logCallback(callable $callback): self | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, but then it wouldn't be about logging anymore, but a hook for when an error happens, right? Because inside the callback you can put anything, there is no LoggerInterface enforcement anymore so we should change the name. How does this look like? $errorHandler->onError(function( Throwable $error, ServerRequestInterface $request) use($logger, $other): void {
$logger->error('Whoops!');
// $other stuff...
});
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks good to me. Maybe an |
||
| { | ||
| $this->logCallback = $callback; | ||
|
|
||
| return $this; | ||
| } | ||
|
|
||
| public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface | ||
| { | ||
| try { | ||
| return $handler->handle($request); | ||
| } catch (Throwable $error) { | ||
| if ($this->logger) { | ||
| if ($this->logCallback) { | ||
| ($this->logCallback)($this->logger, $error, $request); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the logger is callback, I'd do |
||
| } else { | ||
| $this->logger->critical('Uncaught exception', [ | ||
| 'message' => $error->getMessage(), | ||
| 'file' => $error->getFile(), | ||
| 'line' => $error->getLine(), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably I'd include some info about the request: url, method, and uuid (https://github.com/middlewares/uuid).
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, funny that I added it and then forgot to put it back |
||
| ]); | ||
| } | ||
| } | ||
|
|
||
| foreach ($this->formatters as $formatter) { | ||
| if ($formatter->isValid($error, $request)) { | ||
| return $formatter->handle($error, $request); | ||
|
|
||
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 think the logger shouldn't be passed in the constructor because it's an optional feature. It should be a method (for example,
$middleware->logger(...)).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.
Dependencies belong in constructors... how else can we have dependency inversion?
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.
Yes, I agree. But in this case it's an optional feature. People may want to log the errors or not, so I see this as a configuration, not a core dependency of the middleware.
That being said, maybe you're right in your comment about the premise of this feature and it would be better to simply create a specific middleware for this.
@filisko ?
Uh oh!
There was an error while loading. Please reload this page.
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.
A constructor parameter with a default value of
nullhas no cost. (With PHP named parameters, it doesn't even cost writingnullto fill it in.) A method that has to accept a parameter to update the object means the object is no longerreadonly. Thus, a method for an optional is far more expensive to create, maintain, and operate than a constructor parameter.