Howdy Reader,
this is my take on a full code review; enjoy. :)
PHPDoc
As this is a package that we want to open source sooner or later, I suggest to implement our own PHPDoc best practices.
For Wonolog, this would mean to add missing @since annotations, document constants (const as well as define) with a short description and @since and @var annotations, provide a short description for require etc., and document parameters and return type of all functions.
Let's also get rid of @inheritdoc; yes, please?! :) We all use an IDE that serves us well here.
Name References
Referencing the package is not consistent. The file-level C-style comment should use just "Wonolog" like everywhere else, and not "Inpsyde wonolog".
Also, when using namespaces, we usually also use these for the @package annotation, which currently is just wonolog no matter for what file/domain.
Code
Inpsyde\Wonolog\Data\Error
This should be a final class, right?
Inpsyde\Wonolog\Data\FailedLogin
The constructor should cast $username to a string.
The get_level() method is too verbose (and contains error-prone redundand data). I would suggest it like so:
public function level() {
$attempts = $this->count_attempts( 300 );
switch ( TRUE ) {
case ( $attempts > 2 && $attempts <= 100 ):
return Logger::NOTICE;
case ( $attempts <= 590 ) :
return Logger::WARNING;
case ( $attempts <= 990 ) :
return Logger::ERROR;
case ( $attempts > 990 ) :
return Logger::CRITICAL;
}
return 0;
}The order is now important, of course. This could go into a comment.
Also, the 0 return value should be a constant as well.
If sniff_ip() didn't only return a values array, but provide ip and from keys, it could directly be used in context():
public function context() {
return $this->sniff_ip() + [ 'username' => $this->username ];
}Is PHP_SAPI always set, no matter what? Otherwise, checking defined( 'PHP_SAPI' ) cannot hurt, right?
Inpsyde\Wonolog\Data\Log
The $message and $context variables in the named constructor from_wp_error() can be inlined as the according methods will be called only once anyway.
For what particular strange use case is the named constructor from_array() designed?! 🤔 Let's please just require an associative array, and be done with six lines of code here!? No kidding. :)
Also, the constructor should cast $message and $channel to strings, and $level to int.
Inpsyde\Wonolog\Data\LogDataTrait
The constructor should cast $message and $channel to strings.
Inpsyde\Wonolog\Data\NullLog
The return value, -1, of level() could be turned into a class constant (of the NullLog itself, or a better fitting class) in order to allow it to be referenced and managed consistently throughout all things.
Inpsyde\Wonolog\Data\VariableLevelLog
To be honest, I don't understand how this class should be used. I don't see any reason for making the with_level_callback() not a regular named constructor, but a PSR-7-like modifier. I'd rather have something like this:
public static function instance_with_level_callback(
callable $level_callback,
$message,
$channel,
array $context = []
) {
$instance = new static( $message, $channel, $context );
$instance->level_callback = $level_callback;
return $instance;
}Probably no big deal, but iff the level() method gets called more often, there's unnecessary computation overhead. This could be circumvented by having a default callback that just returns Logger::DEBUG (oh, BTW, why hard-coded DEBUG, and not provide ways to adapt this?). A possible implementation is this:
public function __construct(
$message,
$channel,
array $context = [],
callable $level_callback
) {
parent::__construct( $message, $channel, $context );
$this->level_callback = $level_callback ?: [ $this, 'default_level' ];
}
public static function instance_with_level_callback(
callable $level_callback,
$message,
$channel,
array $context = []
) {
return new static( $message, $channel, $context, $level_callback );
}
public function level() {
return $this->level_callback();
}
private function default_level() {
// TODO: Make customizable (via constructors) or use `Logger::DEBUG` here...?
return $this->default_level;
}Inpsyde\Wonolog\Data\WpErrorChannel
The result of apply_filters() inside for_error() should be cast to string.
Also, why only allow to use the filter if there was a channel provided for the named constructor? That would also make very much sense in general, IMO. The channel() method would allow for this, too, but that's maybe too late, meaning too much unnecessary things got computed already.
Use Yoda conditions for stripos() in maybe_http_channel() and maybe_security_channel().
Inpsyde\Wonolog\HookListeners\*
What's the reason to have the update() method on the HookListenerInterface instead of the ActionListenerInterface? All (two) implementations of the former (i.e., CronDebugListener and WpDieHandlerListener) only return a NullLog() anyway. I'd rather have an empty HookListenerInterface, and move the update() method to the ActionListenerInterface. If someone for whatever reason needs to use both, they can easily implement both interfaces. Done.
Inpsyde\Wonolog\HookListeners\CronDebugListener
It would be nice to be able to filter the priority for the add_action() calls in filter().
Also, '-' . PHP_INT_MAX both looks strange and actually is not the lowest possible value (this would be (int) ( PHP_INT_MAX + 1 )). :)
Inpsyde\Wonolog\HookListeners\DbErrorListener
The problem with using (only!) the shutdown action hook is that you can only log regularly ended requests. Potential DB errors on redirect requests, or AJAX calls etc. that get explicitly terminated won't get logged at all, right? To capture wp_die() calls, one could, for example, also use the wp_die_ajax_handler, wp_die_xmlrpc_handler and wp_die_handler filters. Of course, this wouldn't take care of exit(); and die(); calls, though.
Inpsyde\Wonolog\HookListeners\HttpApiListener
In isError() (why is this camelCase, BTW?), one could simplify the second part in the condition to just this:
isset( $response[ 'response' ][ 'code' ] )
&& is_numeric( $response[ 'response' ][ 'code' ] )
&& 200 !== (int) $response[ 'response' ][ 'code' ]
Use Yoda condition in isCron().
In logHttpError(), why is it okay to directly access $response[ 'response' ][ 'message' ] and $response[ 'response' ][ 'code' ] without checking first? From the code, I can't see the array always having these elements/keys. Okay, for $response[ 'response' ][ 'code' ] it is checked in isError(). But $response[ 'response' ][ 'message' ] just might not be there.
Also, the formatting of logCron() and logHttpError() is different. When renaming the parameters accordingly, one could easily use compact() here to create the log context.
Inpsyde\Wonolog\HookListeners\MailerListener
Like mentioned yesterday, the on_mal_failed() method should return a NullLog() after the if statement.
Also, why is the class not final?
Inpsyde\Wonolog\HookListeners\WpDieHandlerListener
Why isn't this class hooked to wp_die_xmlrpc_handler, too?
The closure in isDbError() should be a regular private method, in my opinion.
Inpsyde\Wonolog\Channels
The string in logger() is missing something, or I don't get it:
%s is not a registered channel. Use "" to register custom channels
Inpsyde\Wonolog\FrontController
Like mentioned yesterday, it would be nice to allow for customizing the priority for the add_action() calls in setup(). Also, using 9999 for the number of arguments looks quite odd. If Core would only allow to pass null. :/ Until then, PHP_INT_MAX better carries the message "all of 'em", in my opinion. Even though it's very unlikely to pass more than, say, ten arguments to a function. :D
I would move all the hook listener registering stuff to the setup_hook_listener() method. So one doesn't have to create and pass around a local registry. Let's do this directly in setup_hook_listener(), and also fire the action and flush the registry.
The listen_hook() method would have to be adapted when moving update() from HookListenerInterface to ActionListenerInterface, like mentioned before.
Inpsyde\Wonolog\HookListenersRegistry
I'm not sure that identifying a listener instance by means of its class name is the right thing to do. What if someone already registered some listener, and now someone else (maybe you? :) ) would like to register a listener instance of the same class, but with a different configuration...? This is currently not possible, because you can register only a single instance of a specific listener class.
Also, there's currently no way to learn of listeners that did not get registered (because something was wrong with them). Should we integrate something that takes care of that? For example by means of a simple skipped_listeners() method that accesses a new class property, which gets filled in the listeners() method...?
What's the reason for this:
unset( $this->factories, $this->listeners );
$this->factories = [];
$this->listeners = [];
Is there some garbage collection magic happening, compared to when just setting the properties to an empty array?
Inpsyde\Wonolog\LogActionSubscriber
The namespace import of Inpsyde\Wonolog\Data\Debug is not needed (anymore?).
In update(), ! ( $log->level() > 0 ) should be just 1 > $log->level().
Inpsyde\Wonolog\PhpErrorController
The on_exception() method is missing a type hint. Also, the formatting is not consistent with on_error(). Inlining variables that only occur once is fine for me.
Action and Filter Hooks
In my opinion, any action or filter hook that is (to be) used in more than one unit should not be hard-coded throughout the codebase, but instead be defined as class constant. Good examples are the wonolog.loaded action hook that is currently being used in FrontController, HookListenersRegistry and LogActionSubscriber, and the wonolog.log action hook that is being used in six classes. These should be defined as FrontController::ACTION_LOADED and FrontController::ACTION_LOG.
Code Style
In the Inpsyde\Wonolog\HookListeners\HttpApiListener and Inpsyde\Wonolog\HookListeners\WpDieHandlerListener classes, there are function names in lowerCamelCase, which is odd, in my opinion.
Cheers,
Thorsten
React
Typescript
Django
Laravel
Facebook
Microsoft
Google
Alibaba
D3
Tencent