fix(contrib-auto-laravel): fix log level type issue #306
Conversation
…ger expects int leading to incorrect types
|
Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one. |
| @@ -37,7 +37,9 @@ public function recordLog(MessageLogged $log): void | |||
| $underlyingLogger = $this->logger->getLogger(); | |||
|
|
|||
| /** @phan-suppress-next-line PhanUndeclaredMethod */ | |||
| if (method_exists($underlyingLogger, 'isHandling') && !$underlyingLogger->isHandling($log->level)) { | |||
| if (method_exists($underlyingLogger, 'isHandling') && !$underlyingLogger->isHandling( | |||
| $underlyingLogger->toMonologLevel($log->level)) | |||
There was a problem hiding this comment.
this seems like it would now only work with monolog as the underlying logger, but there could be others, right? is toMonologLevel a part of any common interface between loggers?
There was a problem hiding this comment.
@brettmc
I'm not quite sure how many of the third-party libraries in the industry that comply with \Psr\Log\LoggerInterface support the method method_exists($underlyingLogger, 'isHandling').
- Suppose there are many. Then I wonder if adding
method_exists($underlyingLogger, 'toMonologLevel')is too specific. - Suppose there are few, and it is more for supporting
Monolog. Then I think it is acceptable to addmethod_exists($underlyingLogger, 'toMonologLevel').
Of course, there is another way, which is to add an adapter to solve this problem, but the extension might be a bit cumbersome.
So, I'd like to know your thoughts.
There was a problem hiding this comment.
Also, there is another method, that is, I define an interface of ShouldHandle and bring it in when doing new LogWatcher($this->instrumentation). It's probably like this:
<?php
declare(strict_types=1);
use Illuminate\Log\Events\MessageLogged;
use Illuminate\Log\LogManager;
interface ShouldHandle
{
public function shouldHandle(LogManager $logger, MessageLogged $log): bool;
}There was a problem hiding this comment.
@brettmc
Another processing method. Because method_exists($underlyingLogger, 'isHandling') it is too special. This method is not supported in \Psr\Log\LoggerInterface.
As an official extension package, for this special case, I suggest that users can consider overriding the recordLog method of LogWatcher.
And for this package, we only need to add a shoudRecordLog method.
For this, what we may have to do:
- Roll back Restrict trace events to correct log level #264
- Add a
shoudRecordLogmethod
Of course, there are some other additions. That is, it can be retrieved more conveniently through some configurations. Currently, our register entry has some limitations. But this may take some time to continuously improve.
|
@flc1125 - will you add some relevant tests to this PR as well please? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #306 +/- ##
=========================================
Coverage 80.33% 80.33%
Complexity 1026 1026
=========================================
Files 98 98
Lines 4114 4114
=========================================
Hits 3305 3305
Misses 809 809
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
No problem. Give me some time. |
@flc1125 this link appears to be broken for me. Though if I hover over it, I see the following: I can't actually see that review though. Have you seen an example of this failure? (Do you have a stacktrace to hand?) |
This is an example of an error I encountered:
|
|
@bobstrecansky @brettmc I think I already know where the problem is. The occurrence of this problem mainly lies in the version of
I want to know if our maintenance policy has any requirements for the version of Laravel? This will facilitate me in dealing with this problem.
|
Our policy is to try to support the versions that are actively supported |
@brettmc OK, just checked official maintenance info this pr should be closed. thanks everyone for the help. 
|
fix: #264 (comment)