Global request logging WIP #18
Conversation
There was a problem hiding this comment.
@kyranb I really appreciate this PR. Thanks a bunch.
Besides the comment in the review then I am not really sure I understand why you need two listeners? Is the $requestSendingEvent->request the actual instance that is being sent, because if so then adding the middleware to that one should be just fine I quess?
| */ | ||
| public function handle(ResponseReceived $event) | ||
| { | ||
| $event->response->withMiddleware((new LoggingMiddleware( |
There was a problem hiding this comment.
Question: Can we just use app(LoggingMiddleware::class) instead?
There was a problem hiding this comment.
True, I can't see why note. That would make sense to resolve it rather than instantiating it again
| */ | ||
| public function handle(RequestSending $event) | ||
| { | ||
| return $event->request->withMiddleware((new LoggingMiddleware( |
There was a problem hiding this comment.
Question: Can we just use app(LoggingMiddleware::class) instead?
| ResponseReceived::class => [ | ||
| LogResponseReceived::class, | ||
| ], | ||
| ]; |
There was a problem hiding this comment.
Can we test this using a simple unit test that uses assertListening? 🤔
Event::assertListening(
RequestSending::class,
LogRequestSending::class
);
Event::assertListening(
ResponseReceived::class,
LogRequestSending::class
);
See:
|
@kyranb Did you see me previous comment by any chance? 👍 |
|
@bilfeldt Sorry, yes I did but I haven't had a chance to dive back into this yet. I think you're correct, we only need to log the the sent request 👍 |
Hey Bilfeldt,
After your work on Footprints, I wanted to try and return the favour.
Description
This PR is a WIP. Haven't had the chance to test properly yet.
Does this close any currently open issues?
Aimes to fix #2
Type of change
Please delete options that are not relevant.
...
Checklist