Example custom listener coupled to global WP function

[XedinUnknown]

The Problem

The custom listener example includes a MyFilesListener class. This class is coupled to the global WordPress function current_filter(). This is not good for several obvious reasons. Alas, it does not seem possible to avoid this, because ActionListenerInterface#update() does not provide any context which could be used to determine which hook is being handled.

Suggested Solution

HookListenersRegistry#listen_hook(), the method which appears to invoke the hook callback, looks like it receives the hook name as the first $hook parameter. If this value could be passed to HookListenersRegistry#hook_callback(), and then subsequently passed to the #update() method, this would give listeners enough context to distinguish between hooks without relying on global context.

This would not break BC, because it would add a parameter to a method. Listener implementations that currently do not support this will simply get an extra argument, which will not be handled.

[gmazzap]

This class is coupled to the global WordPress function current_filter(). This is not good for several obvious reasons.

Surely it's me, but that reasons are not that obvious to me. Wonolog will be always used in WordPress context.

Outside WP context it makes no sense (being just a set of WordPress-specific "sugars" on top of Monolog, outside WP one could use Monolog directly) and would not work (there're calls to WP functions like add_action that can't be avoided).

Being used in WP context, IMO we can consider WordPress as our framework. To me that means that WP is almost the library language, and just like one doesn't write libraries for PHP that are decoupled from PHP, I don't see the urgent need to write libraries for WordPress that are decoupled from WordPress.

If one of the reasons you have in mind is testability in isolation from WordPress, that's a solved issue: Brain Monkey not only allow testability for that, it does it without even requiring one to do anything, by defining current_filter out of the box.

That said, per se, I have nothing against passing the hook to the ActionListenerInterface::update() method (or the similar FilterListenerInterface::filter() method) but that would actually break backward compatibility.

Adding the additional parameter to the interface, even providing a default for that, would be a backward incompatible change, due to signature mismatch (https://3v4l.org/Dhqvi).

Surely we could pass $hook parameter to ActionListenerInterface::update() and FilterListenerInterface::filter() without specifying that in the interfaces, but I am really against that.

Otherwise concrete listeners would have to rely on func_num_args() + func_get_arg() to obtain the hook, not being able to type $hook in the parameter list (plus probably some is_string() would be needed to check what is received is at least of the correct type), and all of that looks very ugly to me.

The only backward compatible way I can think of to handle this is to add 2 new interfaces, something like:

interface MultipleFilterListenerInterface extends HookListenerInterface {
    public function filter(string $hook, array $args);
}

interface MultipleActionListenerInterface extends HookListenerInterface {
    public function update(string $hook, array $args);
}

and then add some logic in HookListenersRegistry::hook_callback() to call the proper method based on the interface implemented by the concrete listener.

(Note that methods are named in the same way of existing interfaces, on purpose, to avoid a concrete class to implement both old and new interface, as that would make impossible, or at least ambiguos, to know which method to call in that case)

Adding these two new interfaces would be totally backward compatible, as the interfaces are new, old interfaces are there unchanged, and concrete classes using old interfaces would continue to work.

Even if doable without much effort, after all is just about adding 2 new interfaces for a total of 6 lines of code, I am still not excited about the idea of doing it, and would prefer to update existing interfaces in next major version.

The reason is that if in next version we decide to pass the hook to update() / filter(), and it looks reasonable to me, these two new interfaces would be not needed anymore and we would need to deprecate them. And the idea to add 2 new interfaces that we know already would be deprecated in next version is not something that I like that much.

Even because:

• as said above, I don't see big issues in using current_filter
• the provided example is just an example, nothing shipped with Wonolog. If one, for any reason, want really to avoid current_filter call, could create a listener object for each listened hook, so that there's no hook to check.

That said, this is not my package, and if there will be some consensus to the idea of adding these 2 new interfaces (or to other solutions that would keep backward compatibility) I will not oppose the merging of such change.

[XedinUnknown]

Thanks for your feedback, @gmazzap!

My reason was going to be that relying on globals is bad on its own, and opens the implementation to many more problems than testability, because it fails to limit the actions of the class to what is made explicitly available to that class, and couples it, breaking DIP. There is an unlimited number of cases that may be affected negatively by this, which is why there are principles that help us avoid this kind of problem without even knowing exactly what the cases are.

I completely agree that new interfaces would quickly become redundant, and they add unnecessary complexity, besides making entities responsible for the same things. I am very pro adding this to the roadmap of the next BC-breaking release - which would be 2.0 in this case. I hope to receive more feedback, and would be happy to submit a PR.

[gmazzap]

When we move to next major version there are quite a lot of things that can be improved in the package.

E.g. the idea of determining the behavior based on the interface an object implements is not something that I like very much.

Moreover, in next major version I would really like to move min PHP version to 7.0 at very least. Which would open to a quite big amount of possible improvements.

Note that currently Monolog 2 is in beta version, and it requires PHP 7.1+. We could consider to use that as min PHP version as well, and move the dependency to next major version of Monolog, which seems a wise choice (pairing Wonolog major version with Monolog major version looks very reasonable and user firendly to me).

If @XedinUnknown or anyone else, want to start thinking about what we could do for version 2, feel very welcome.

I guess we can start with finding some consensus in the min PHP version, decide if we should target Monolog v2, and also about the fact that this package does not follow Inpsyde code style, so we probably can consider moving to it.

When that is done, we can re-think some architectural decisions, and also reconsider various part of the library, taking as priority the feedback of people who have used Wonolog in real world (and looking at packagist.org stats, that also means people outside Inpsyde).

So, honestly, I would surely put this issue in the list of things we can consider for next version, but I would not start from here for v2, but I would look at a wider scope first.

[Chrico]

We will re-consider this in version 2.x of the package.

[widoz]

I created a PR for fixing this. I also had a talk with @Chrico about my doubts on passing the $hook parameter to the ActionListenerInterface::update and FilterListenerInterface::filter and he proposed an alternative approach which if I recall it correctly (sorry my wonolog knowledge is very limited) allow to call the method directly eg.

public function listen_to() {
    return [ 
    'phpmailer_init' => [$this, 'on_mailer_init'],
        'wp_mail_failed' => [$this, 'on_mail_failed']
    ];
}

Additionally, I don't know if would make sense to pass the hook to the context too when calling update or filter.

[gmazzap]

This is going to be tacked in v2. Clòosing for now, we can discuss the implementation for v2. Due to breaking chanegs is not an option for v1.