Opened 6 years ago
Last modified 4 months ago
#46635 new enhancement
Improve identifying of non–trivial callbacks in hooks
Reported by: | Rarst | Owned by: | |
---|---|---|---|
Milestone: | Awaiting Review | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | has-patch has-unit-tests |
Focuses: | coding-standards | Cc: |
Description
The API for hooks provides comprehensive support for adding any kind of valid PHP callback.
However there are currently no easy way to identify non–trivial (non–string) callbacks in any other operations, such as has_*
or remove_*
.
Rather than discouraging use of complex callbacks, I would like to suggest API is improved to better support them.
In my experience the best take I had seen is to identify complex callbacks with a string representation in Brain Monkey testing library.
Tentatively I would imagine support for following expressions (up for discussion and refinement):
ClassName()
object instance (such as invokable object)ClassName->foo()
concrete method callbackClassName->*
any concrete method callbackclass()
verbatim, anonymous class instancefunction()
a closure instancefunction( $argument )
a closure instance with specific signature
E.g. remove_action( 'init', 'ClassName->*' )
should remove all method callbacks belonging to instances of that class.
Change History (30)
This ticket was mentioned in Slack in #core-coding-standards by rarst. View the logs.
6 years ago
#4
follow-up:
↓ 6
@
6 years ago
The comments on the recent coding standards proposal also have some discussion about this ticket.
In particular, the thread starting in comment-35602. I don't have time to dig into this closely, but at first glance it seems like generating a hash for a closure will be unreliable, since the signatures aren't unique, and change often in practice. Technically function names can change too, but in practice that's much less common.
#5
@
6 years ago
I thought some more about identifying closures. In one of my plugins I display their definition point (source path, line) for informational purposes.
So if identifying closure by signature alone is concern we could also try something path based, e.g. PluginFile.php/function()
#6
in reply to:
↑ 4
@
6 years ago
Yes, signatures are "fragile". Using the declaring file as identifier some-file.php/function()
, would be more stable: the fact that a closure is moved from a file to another can not be assumed to be more frequent than a function that changes name.
However, that would require Reflections which means that to store the serialization upfront would be a potentially big waste of resources having to serialize any closure, even if chances are high none will be removed. On the other hand, not storing upfront means that removing will be quite expensive having to loop the potential dozen of callbacks added to a given hook, and using Reflection anytime a closure is encountered.
For me, the best scenario is when the developer can pass an unique identifier for the closure, that will be fast, reliable, and very future proof, even *more* than current situation: the actual callback added can completely change (even not being a closure anymore), but as long as the identifier is the same, code that removes the callback don't need to change.
Being an additional optional argument this does not pose any backward compatibility issues.
Again, I see something like this:
<?php add_action( 'save_post', function ( $post_ID, $post ) { // ... }, 10, 2, 'my_plugin_save_post_routine' );
as the best solution: removing this closure will be easy, reliable, future proof and performant.
A WPCS rule could be added to encourage usage of this identifier when closures are used as callbacks, and in case core will ever use closures for hooks, it should follow that guideline.
#7
@
6 years ago
On the other hand, not storing upfront means that removing will be quite expensive having to loop the potential dozen of callbacks added to a given hook, and using Reflection anytime a closure is encountered.
I am fine with removal being more expensive operation. And to be clear I am suggesting we explore path as one of the ways of identifying, not the only way. So if broad function()
match is good enough then it can be just used without doing paths lookup.
For me, the best scenario is when the developer can pass an unique identifier for the closure
While this would have made sense if it was baked into API from the start, I feel it is a weak as an optional addition to a very established one. It simply doesn't help the case when developer doesn't add an identifier and we are back to square one. We do need to improve removal for current state of API.
#8
@
6 years ago
While this would have made sense if it was baked into API from the start, I feel it is a weak as an optional addition to a very established one. It simply doesn't help the case when developer doesn't add an identifier and we are back to square one. We do need to improve removal for current state of API.
Yes, what I've in mind is a way to override an identifier which, by default, would be auto-generated.
A fictional function that generates the string representation of a callback could be like this (I speak PHP better than english):
<?php function _wp_serialize_callback( $callback, $identifier = null ) { if ( ! is_callable( $callback, true ) ) { return ''; } // Dev-provided identifier, just use it if ( $identifier ) { // should be a string, but who knows return maybe_serialize( $identifier ); } // plain function (or static methds in the form "ClassName::methodName") if ( is_string( $callback ) ) { return $callback; } if ( $callback instanceof Closure ) { // $r = new ReflectionFunction($f); return 'function()@' . $r->getFileName(); return 'function()'; } // invokables if ( is_object( $callback ) ) { $class = get_class( $callback ); if ( false !== strpos( $class, '@' ) ) { // anonymous class $class = 'class'; } return $class . '()'; } // methods if ( is_array( $callback ) ) { // static methods can be rendered as strings if ( is_string( $callback[0] ) ) { return $callback[0] . '::' . $callback[1]; } $class = get_class( $callback[0] ); if ( false !== strpos( $class, '@' ) ) { // anonymous class $class = 'class'; } return $class . '->' . $callback[1]; } }
With such function in place, WP_Hook
could keep a map from callbacks IDs (created by _wp_filter_build_unique_id
) to callback string representations.
If a string representation is passed to remove_filter
, the callback ID can be resolved from that map, and hook can be removed.
Of course, the map could have multiple IDs for a single string representation, and in such case probably all matching hooks should be removed.
With such approach at least we have something, but in best case scenario (which imo should be recommended by WPCS) there's the possibility to really uniquely identify closures.
#9
@
6 years ago
For closures, what could be done is:
- store
"function()"
in the hook class - if exactly
"function()"
is passed toremove_filter
all closures added to that hook are removed - if something like
"function()@wp-content/plugins/foo/bar.php"
is passed, first all the callbacks with closures are retrieved (all those having"function()"
as identifier) then they are looped to find a match in the declaring file.
#10
follow-up:
↓ 12
@
6 years ago
The discussion about closures in hooks here and on https://make.wordpress.org/core/2019/03/26/coding-standards-updates-for-php-5-6/ has been ...fascinating :)
As there doesn't seem to be a good way to remove filters and actions where the callback is a closure for now, I think we should add a "Doing it wrong" warnings so all new plugin/theme developers are aware they are breaking the hooks API.
If/when closures become "easily removable", we can remove the Doing it wrong warnings too.
#11
follow-up:
↓ 14
@
6 years ago
Using a closure for a callback is far from doing it wrong. Only when the callback needs to potentially be unhooked is using a closure wrong.
Also, closures have been a feature of PHP for the last decade. I can't believe these conversations still take place.
#12
in reply to:
↑ 10
@
6 years ago
Replying to azaozz:
As there doesn't seem to be a good way to remove filters and actions where the callback is a closure for now, I think we should add a "Doing it wrong" warnings so all new plugin/theme developers are aware they are breaking the hooks API.
If/when closures become "easily removable", we can remove the Doing it wrong warnings too.
Are you going to forbid objects as well? Because there is no good way to remove them either, access to original instance is in no way guaranteed.
There is nothing breaking whatsoever about using advanced callbacks. If there was such a concern then Hooks API should never have supported advanced callbacks.
Really I get that from core perspective closures are "new" and questionable. From perspective of last nine years of PHP development, including WordPress space, they are fine.
#13
follow-up:
↓ 15
@
6 years ago
As there doesn't seem to be a good way to remove filters and actions where the callback is a closure for now, I think we should add a "Doing it wrong" warnings so all new plugin/theme developers are aware they are breaking the hooks API.
I really hope this is not what we will end up doing. This would basically mean that, now that we're moving Core away from PHP 5.2, we're forcing plugins & themes that have been doing sensible development for years back into PHP 5.2, for the simple reason that the hooks system is missing the concept of an identifier...
Why not think about something like a RemovableClosure( $id, $callable )
object instead or something along these lines. Let people decide whether they want to have them be removable or not. They are probably using a closure for a very good reason in the first place...
#14
in reply to:
↑ 11
@
6 years ago
Replying to johnbillion:
Using a closure for a callback is far from doing it wrong. Only when the callback needs to potentially be unhooked is using a closure wrong.
Exactly. Closures are great for all "inline" callbacks, except when they break an existing API :)
I really hope a good solution can be found for the hooks API.
#15
in reply to:
↑ 13
@
6 years ago
Replying to schlessera:
I really hope this is not what we will end up doing.
Yeah, I'd hate doing it too but as PW 5.2 officially "opens up" the possibility for using closures in hooks, thinking we need to ... do something to at least warn (new) developers that they are breaking the existing API without realizing it...
Let people decide whether they want to have them (hooks) be removable or not.
Having "enforced", non-removable, almost "hard-coded" actions and filters is something I think we should avoid at all costs!
The hooks API is old, but is very fast and simple to work with. It was designed to be "open", i.e. core or any plugin can add or remove any hook. Breaking this principle now would be pretty bad decision.
#16
@
6 years ago
Yeah, I'd hate doing it too but as PW 5.2 officially "opens up" the possibility for using closures in hooks
WordPress officially opened up possibility of using closures in hooks nine years ago #10493
#17
follow-up:
↓ 18
@
6 years ago
Here's another proposal on how this could be solved.
First, I'd create an object that let's us wrap the closure in an object together with an ID.
<?php final class RemovableClosure { private $id; private $callable; /** * Instantiate a RemovableClosure object. * * @param string $id Identifier of the closure. * @param callable $callable Callable logic for the closure. */ public function __construct( $id, callable $callable ) { $this->id = $id; $this->callable = $callable; } /** * Get the ID of the closure. * * @return mixed */ public function get_id() { return $this->id; } /** * Invoke the closure. * * @param mixed ...$arguments Arguments to pass to the closure. * @return mixed Result of invoking the closure. */ public function __invoke( ...$arguments ) { $callable = $this->callable; return $callable( ...$arguments ); } }
This is valid PHP 5.6 code, so it could be committed together with the minimum version bump if WordPress does indeed enforce that minimum before updating. If not, the __invoke()
method would have to be adapted.
Adding an action/filter and removing it again would be done as follows:
<?php add_action( 'some_action', new RemovableClosure( 'my_closure', function ( $some_arg ) { echo "{$some_arg} was passed."; } ) ); remove_action( 'some_action', 'my_closure' );
Internally, the only thing that would need to be changed is to add a condition to _wp_filter_build_unique_id()
function:
<?php function _wp_filter_build_unique_id($tag, $function, $priority) { // [ ... ] if ( $function instanceof RemovableClosure ) { return $function->get_id(); } // [ ... ] }
I haven't looked into the code in detail, so I might have missed a few nuances, but I think the basic principle should work.
And please, y'all, when building new functionality, please at least consider objects... PHP 7+ is built 100% around objects, there's only drawbacks and performance penalties to be had when still sticking to procedural code by default. This does not mean we should rewrite WordPress, but I rarely see anyone resorting to objects to solve new problems in here.
#18
in reply to:
↑ 17
@
6 years ago
I forgot to mention two things:
- Yes, the
RemovableClosure
is acallable
to PHP (due to the presence of__invoke()
), so it will just behave as a regular Closure, or any other callable. - We can still provide a convenience function, like
add_closure_action( $hook, $id, $closure, $priority, $num_args )
to hide the object instantiation. This can be done without BC break.
#19
follow-up:
↓ 20
@
6 years ago
@schlessera Your class solve the issue for closures, but not for objects, but they suffer from same issue.
Also, most of the good of closures comes from being concise, having to wrap them in an object defeats that.
Moreover, the ID of the closure need to be provided by user, and there's no assurance of unicity that _wp_filter_build_unique_id
has, and I would not replace the real unique id with an user-provided maybe unique id.
Considering that a similar approach would be needed also for dynamic methods, imo the overall result would be that making use of methods and closure becomes harder, not simpler for users.
Provide an additional function (or two, if a separate function has to be used for objects methods) would allow to keep the conciseness, but does not seem a great solution to me, because add_action
and add_filter
are still there serving same purpose, and they could be continued to be used (and no, please, let's not start to trigger _doing_it_wrong in that case).
For me, if WordPress would generate a string representation by itself and only optionally accept an identifier from the user, that would require basically no change in existing users code, no additional API, and the core would really need a few changes to accomodate this.
Imagine the function _wp_serialize_callback
from my previous comment (https://core.trac.wordpress.org/ticket/46635#comment:8) becomes a WP_Hook::serialize_hook_callback()
method.
After that, only a single line and an additional optional param would be added to WP_Hook::add_filter
:
<?php public function add_filter( $tag, $function_to_add, $priority, $accepted_args, $id = null ) { $idx = _wp_filter_build_unique_id( $tag, $function_to_add, $priority ); // next line is the only added, `$this->identifiers` is a new, private, property $this->identifiers[ $idx ] = $this->serialize_hook_callback( $function_to_add, $id ); // [ ...]
Finally WP_Hook::remove_filter
would require less than 10 more LOC
<?php public function remove_filter( $tag, $function_to_remove, $priority ) { /* If $function_to_remove is an "old" identifier, the `empty` check below ensures back compat and also avoid infinite loop when method is called recursively below */ if ( is_string( $function_to_remove ) && empty( $this->identifiers[ $function_to_remove ] ) ) { $function_keys = array_keys( $this->identifiers, $function_to_remove ); } if ( ! empty( $function_keys ) ) { $removed = 0; // recursively call itself, passing unique identifier foreach( $function_keys as $function_key ) { $removed += (int)$this->remove_filter( $tag, $function_key, $priority ); } return $removed > 0; } // code that removes hook using _wp_filter_build_unique_id is here, unchanged }
And that's it.
This is all the code needed to give users ability to identify (and remove) the great majority of callbacks, being them invokables, objects methods or closures, without additional API, and without the need for who add the callbacks to do anything different at all.
Also the unicity of _wp_filter_build_unique_id
would be kept and the whole thing would be totally backward compatible.
Only for closures, is up to who add them to provide additional reliability on removal, by providing an ad-hoc identifier.
A better by default reliability for closures removal, without user-provided ids, would require some use of reflection and some more code. I would be very fine in having that, but having at least this would be already definetively more than we have now.
#20
in reply to:
↑ 19
@
6 years ago
Replying to giuseppe.mazzapica:
Your class solve the issue for closures, but not for objects
if WordPress would generate a string representation by itself [...] that would require basically no change in existing users code
I disagree. Both closures and objects already work perfectly fine. The API has provided a contract and expectations, and plugins/themes have built their code around that. They are not easily removable, and developers know that, and probably use it just because of it.
All of a sudden turning non-removable code into removable code is the BC break that causes the users to need to change their existing plugin/theme code. You're all of sudden breaking the expectations, and the API behaves differently for a same set of input.
What I have provided is a way to explicitly add a removable form of closures, and it is a mechanism that does not break BC or produce unexpected changes. The exact notation can be modeled after whatever you want, and you can add smart prefixing if you think that is needed. But most of all, it does not change the existing behavior.
Developers have been able to write working plugins for 9 years now despite the fact that this was already in the "broken" state it is. Trying to fix that now by flipping the behavior of the existing API is what will break plugins.
#21
@
6 years ago
I'd like to reiterate that introducing a new opt-in behavior does not address what this issue is trying to address. That is convenience of removing advanced callbacks that are already being added (for many years now) with current API.
They are not easily removable, and developers know that, and probably use it just because of it.
All of a sudden turning non-removable code into removable code is the BC break that causes the users to need to change their existing plugin/theme code.
I disagree with this take. The API had never intended or advertised any callbacks as being unremovable explicitly or implicitly.
All of advanced callbacks, closures included, can currently be removed by API with access to original instance.
#22
@
6 years ago
All of advanced callbacks, closures included, can currently be removed by API with access to original instance.
This is key for me. If we made add_action
return a reference to the callback that was passed in, then that could be made available for other developers to use to remove it.
This doesn't solve the problem of existing code registering unremovable callbacks, but is that so much of a problem? If there are mechanisms in place to make it possible to remove hook and filter callbacks, then it is up to developers to follow guidelines (or rules) to use them.
#23
@
5 years ago
Noting that for the wp.hooks
JavaScript implementation (where anonymous functions are very common), we added a required namespace parameter explicitly so we could (find and) remove hooks. Is there any way we could add something like that to PHP hooks?
#24
@
5 years ago
we added a required namespace parameter explicitly so we could (find and) remove hooks. Is there any way we could add something like that to PHP hooks?
Since API had already been established for many years it is not viable to introduce new mandatory behavior for complex callbacks.
While new opt in behavior could be introduced, as above I strongly believe that would not be an adequate solution. We are not at API design stage here, we need solution that enhances long existing API in very wide use.
I am busy with work at the moment, but I still intend to circle back to this and take a stab on a patch following my proposal.
#25
@
5 years ago
Since API had already been established for many years it is not viable to introduce new mandatory behavior
...
While new opt in behavior could be introduced, as above I strongly believe that would not be an adequate solution.
Not sure I agree :)
Would that mean the API is "frozen in time" and shouldn't evolve (in a backwards-compatible way)?
Looking at the proposed solutions, seems the API can be enhanced to support removal of closures in a simple and fully backwards-compatible way. For now I like what @schlessera posted in comment 17 the best. Passing a (standardized) object when adding a filter or action is futureproof and very simple to use. Only I'd probably change the name to something related to WP hooks, perhaps new Add_WP_Action()
with an alias of Add_WP_Filter()
, or maybe something like WP_Callback()
or WP_Hook_Callback()
.
Then Core will also be able to use closures in hooks, and we can think of a "nice way" to get plugins to switch to it too.
#26
@
4 years ago
Just wanted to chime in here from our perspective (GiveWP). We're building a Service Container into our next version for automatic dependency injection. We wanted to have a way where we weren't having to instantiate every hookable class just in case the hook fires. This is an unnecessary memory expenditure.
The way we came up with to get around this issue was to use a Closure in the hook and instantiate the class if and when it's fired. This works nicely, but we ended up needing to include an ugly workaround to provide a way for folks to effectively "remove" the hook:
<?php public static function addAction( $tag, $class, $method = '__invoke', $priority = 10, $acceptedArgs = 1 ) { if ( ! method_exists( $class, $method ) ) { throw new InvalidArgumentException( "The method $method does not exist on $class" ); } add_action( $tag, static function () use ( $tag, $class, $method ) { // Provide a way of disabling the hook if ( apply_filters( "give_disable_hook-{$tag}", false ) || apply_filters( "give_disable_hook-{$tag}:{$class}@{$method}", false ) ) { return; } // instantiate from the service container $instance = give( $class ); call_user_func_array( [ $instance, $method ], func_get_args() ); }, $priority, $acceptedArgs ); }
Looking at @schlessera's solution, I believe that would work great for us. At first I wished that WP had a way of instantiating as needed itself, but I now realize that would be more than necessary. Simply having a way of registering a removable Closure would work fine.
I agree that the suggestion in comment 17 is the best and will work for all callables (which includes Closures and class methods).
Thanks for all the great discussion. Hope to see this move forward!
This ticket was mentioned in PR #569 on WordPress/wordpress-develop by gmazzap.
4 years ago
#27
- Keywords has-patch has-unit-tests added
Trac ticket: https://core.trac.wordpress.org/ticket/46635
---
## Premise
Now that WordPress is having PHP 5.6 as minimum version, and it is working toward plans to move forward the minimum version at a predictable pace, the usage of closures, or even _short closures_, is something that many developers will be willling to use, many already do.
However, it is "officially"<sup>1<sup> considered bad practice to use those because it is not possible to remove hook callbacks added that way.
In the linked ticket, Rarst proposed make it possible to remove hooks by a sort of predictable "string representation" of closures, and more generally for hooks callbacks that make use of object instances.
Rarst's proposal is inspired by what Brain Monkey, a testing tool for WordPress that I happen to write a few ago and still maintain.
What Brain Monkey does, and what Rarst proposes is however something that will make calculation and retrieval of "string representation" too expensive for something like hooks.
In this PR I will _not_ support all the possibilities listed by Rarst in the Trac ticket, but it I will support a basic subset (with some variations) but keeping an open door to the possibility to extend the support for other possibilities in a later iteraction.
## Scope
When I started working on this PR I had in mind the following priorities:
- do not break backward compatibility in *any* way, even supporting code that makes use of "internal" or "private" WordPress functions
- allow the check (
has_filter
/has_action
) and the removal of hook callbacks supporting all the possibility offered by PHP callable
## Implementation
Assuming a functions.php
that contain the following code:
{{{php
add_action('some_hook', function () { /* some code here */ });
add_filter('some_other_hook', [new SomeObject, 'a_method']);
}}}
If merged, this PR will make possible to check and remove the added hooks like so:
{{{php
Check
has_action('some_hook', 'function()@functions.php'); true
has_filter('some_other_hook', 'SomeObject->a_method'); true
Remove
remove_action('some_hook', 'function()@functions.php');
remove_action('some_other_hook', 'SomeObject->a_method');
}}}
Please note that:
- for closures, the file name to use after the
@
is the *basename*, not the full path. - in both cases namespace is relevant.
### Namespace
Let's assume our functions.php
contains:
{{{php
namespace My\Awesome\Plugin;
use Another\Name\Space\SomeObject;
add_action('some_hook', function () { /* some code here */ });
add_filter('some_other_hook', [new SomeObject, 'a_method']);
}}}
If merged, this PR will make possible to check and remove the added hooks like so:
{{{php
remove_action('some_hook', 'My\Awesome\Plugin\function()@functions.php');
remove_action('some_other_hook', 'Another\Name\Space\SomeObject->a_method');
}}}
Note how the namespace is the namespace where classes/closures are _defined_ not where they are _used_ even if for closures the great majority of times the namespace where they are defined is the same where they are used.
There're peculiar cases that it is worth to see in detail.
### Anonymous classes
PHP does not allow to retrieve the namespace of anonymous classes. In other words, anonymous classes are always in the root namespace. Similarly to what is done for anonymous functions (aka closures) the way anonymous classes can be identified is class()@filename.php
.
However, unlike functions, classes can extend other classes. This is captured by the code in this PR that requires the parent class to be included in the "string representation" of the class.
For example:
{{{php
$object1 = new class {
public function toArray() {
return [];
}
};
$object2 = new class extends ArrayObject {
public function toArray() {
return $this->getArrayCopy();
}
};
add_filter('some_filter', [$object1, 'toArray']);
add_filter('some_filter', [$object2, 'toArray']);
Remove filter using $object1
remove_filter('some_filter', 'class()filename.php');
Remove filter using $object2
remove_filter('some_filter', 'class()ArrayObject@filename.php');
}}}
Remember that namespace does not affect "callback id" for anonymous classes but does affect id generated for closures and named classes.
### Invokable objects
In PHP any object implementing a method named __invoke()
can be used as a callback.
Anonymous funcitons are behind the scenes nothing more than instances of a class (Closure
) that has a __invoke()
method. (see https://3v4l.org/OXpBe).
Whne using an invokable object, we can use the __invoke()
as hook callback both explicitly and implicitly. For example:
{{{php
class MyInvokable {
public function invoke() {
return true;
}
}
$invokable = new MyInvokable;
Following two lines do exact same thing:
add_filter('an_hook', $invokable);
add_filter('an_hook', [$invokable, 'invoke']);
}}}
For this reason the "string representation" used by this PR will use exact same representation for both. For example, *both* the two filters added in the snippet right above can be removed like this:
{{{php
remove_filter('an_hook', 'MyInvokable->invoke');
}}}
To see how things works together, we can see an anonymous class that extend another class and is also invokable:
{{{php
$invokable = new class extends AnotherClass {
public function invoke() {
return true;
}
};
Hook added like this:
add_filter('an_hook', $invokable);
Can be removed like this:
remove_filter('an_hook', 'class()AnotherClass->invoke');
}}}
## Backward compatility
This PR is 100% backward compatible.
Code that uses "static" callbacks, like funciton names or static methods are not affected at all.
For code that make use of object instances (and that ioncludes anonymus functions) all the current approaches continue to work.
For example, let's assume the following code:
{{{php
global $func, $object;
$func = function () { /* some code here */ };
$object = new SomeObject;
add_action('some_hook', $func);
add_filter('some_other_hook', [$object, 'a_method']);
}}}
Currently, we can remove these hooks, in some other file, like this:
{{{php
global $func, $object;
remove_action('some_hook', $func);
remove_filter('some_other_hook', [$object, 'a_method']);
}}}
If we have access to exact same instance that was used to add the hooks, we can alos remove the hooks. This PR continue to support this approach.
Even more, something that is not really documented is that we can use `spl_object_hash` to remove hooks that make use of objects. For example, following code has the exact same effect of the code right above:
{{{php
global $func, $object;
remove_action('some_hook', spl_object_hash($func));
remove_filter('some_other_hook', spl_object_hash($object) . 'a_method');
or alternatively:
remove_filter('some_other_hook', _wp_filter_build_unique_id(, [$object, 'a_method'], false);
}}}
This approach, even if can be considered quite "hackish", continues to be supported by this PR.
## A new approach: custom callback ID
As *additional* functionality, that works side-by-side with the workflow described above, this PR introduces a new parameter to add_action
and add_filter
that allow to define a custom ID for callbacks that make use of object instances.
For example, hooks added like this:
{{{php
class MyPlugin {
public function init() {
add_action('init', [$this, 'init'], 10, 2, 'my-plugin/init');
add_action('wp', [$this, 'wp'], 10, 2, 'my-plugin/wp');
add_action('pre_get_posts', [$this, 'pre_get_posts'], 10, 2, 'my-plugin/pre_get_posts');
}
}
}}}
Can be removed like this:
{{{php
remove_action('init', 'my-plugin/init');
remove_action('wp', 'my-plugin/wp');
remove_action('pre_get_posts', 'my-plugin/pre_get_posts');
}}}
The additional ID is entirely optional, but when it is used the only way to check or remove the hook will be the custom ID.
In the current implementation the custom ID is only supported for callbacks that make use of object instances, any custom ID used for callbacks represented by plain funciton names or static methods will be ignored and will end up in _doing_it_wrong_
being called (assuming that funcitons isalredy loaded).
## Tests
This PR includes small change to the WordPress unit tests suite to prevent existing tests to fail, however the new funcionality is not really tested in here.
The reason is that not being familiar with development practices at core I'm not sure how I should test things that require PHP 7+ (like anonymous classes) or closusres declared in a namespaced context.
Thsi is why to test my code I created a separate repository where I copied plugin.php
and class-wp-hook.php
from this PR and I tested them to ensure the new functionality works as expected and that backward compatibility is ensured.
## Gotchas
Even if this PR took backward compatibility as primary goal there's a sort-of edge case that will break if it woudl be merged.
Consider following code:
{{{php
add_action('say_hi', function() { echo "Hello"; });
add_action('say_hi', function() { echo "World"; });
do_action('say_hi');
}}}
This code will actually end in *"Hello World"* being printed, because _both_ closures are being added, even if the hook and priority are the same. With this PR merged the second closure will override the first, and executing the code only *"World"* is printed.
The reason is that both closures are "serialized" as function()@filename.php
and this identifier is identical for both (assuming both are placed in the same file).
If this look bad to you, please notice that this is something that *already* happens for "static" callbacks and even for object methods uisng same instance.
For example:
{{{php
function say_hi() {
echo "Hi";
}
add_action('say_hi', 'say_hi');
add_action('say_hi', 'say_hi');
do_action('say_hi');
}}}
The code above prints *"Hi"* once, not twice.
Or consider the following:
{{{php
public MyPlugin {
public function add_test_hook() {
add_action('test', [$this, 'test']);
}
public function test() {
static $counter = 1;
echo "Counter: $counter";
$counter++;
}
}
$plugin = new MyPlugin;
$plugin->add_test_hook();
$plugin->add_test_hook();
$plugin->add_test_hook();
do_action('test');
}}}
The code above will print *"Counter: 1"* even if add_test_hook
was called 3 times.
What I mean is that by using same hook, and same callback, and same priority WordPress _already_ add the hook only once so I believe that what this PR does is actually to add more consistence.
Of course, by defining closures in different files, or even just using a different priority will be enough to make them distinguable.
For example the following code:
{{{php
add_action('say_hi', function() { echo "Hello"; });
add_action('say_hi', function() { echo "World"; }, 11);
do_action('say_hi');
}}}
Will print *"Hello World"* as expected.
---
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.
<sup>1<sup> For "officially" I mean _for code to be written for core_.
#28
@
16 months ago
Linking ticket #59126 for visibility: PHP 8.1 first class callables should also be taken into account when fixing this.
#29
@
5 months ago
Hoping to bring this back to life, as I think we need a solution to be able to use PHP 8.1 first class callables in WordPress with a way to remove them.
I think there are two proposals in this ticket that are most interesting:
- from @schlessera, the proposal is to introduce a
RemovableClosure
class which takes a unique ID and a the callable. Only a very small change is needed to the Filter/Actions internals to pull the unique ID.
- from @gmazzap (github), the proposal is to add an optional 4th argument to add_action/add_filter with a unique id, which is used in-place of the callable ID. This is probably the most ergonomic, but requires more changes to the Filter/Actions API and internals.
I personally prefer the first proposal from @schlessera as I think it will be simpler to introduce and won't expand the Filters API. The only thing I might suggest there would be to use an interface Identifiable_Callback
or similar which just exposes the get_id(): string
function. This means 3rd parties can implement their own versions of RemovableClosure
should they want.
#30
@
4 months ago
I would also love to see this reach the finish line. I think your summary is good, @joehoyle. I an see issues in adding the 4th parameter that make backwards compatibility difficult, which leans me towards @schlessera's suggestion. I do acknowledge that the class-based route is a bit more complex, but I like the idea of encapsulating callbacks within an invokable class, which is something we often do anyway.
The only change I'd make to the class approach would be to make RemovableClass
both an interface (with more distinct properties/methods) and a final class. That way folks could return a custom class that implements the interface and handles the callback.
Hi, author of Brain Monkey here.
I honestly think this is a much needed thing, and I have experimented with this also using a different approach here: https://github.com/inpsyde/objects-hooks-remover
where I introduce a dedicated API:
remove_object_hook
remove_closure_hook
remove_class_hook
remove_instance_hook
remove_invokable_hook
The idea behind is similar, but I felt that a dedicated API would be a better fit for a production library (opposed to Brain Monkey that should never run in production).
Now that I think about it, in case of core merge the string identifier proposed by Andrey would probably a better fit, so that thousands of developers don't need to learn a new API.
The main issue with both my approaches is that quite expensive operations has to be made to properly identify objects, and especially closures.
But core could do something I could not do: to change how hooks are stored.
In fact, alongside
'function'
and'accepted_args'
, the methodWP_Hook::add_filter
could store a predictable string for the hook, in a form similar to the one used by Brain Monkey or proposed here by Andrey, this way, when checking for hook existence (WP_Hook::has_filter()
) or when removing hooks, it would be possible to compare the wanted callback, provided as string identifier, with this stored identifier, making the whole operation simpler and faster.This would not pose backward compatibility issues, because for plain functions and static methods the identier would be identical to what returned by
_wp_filter_build_unique_id
, and in case a non-string is passed toremove_filter
orhas_filter
,_wp_filter_build_unique_id
could continue to be used.Moreover, an additional parameter could be added to
add_action
andadd_filter
allowing a way to provide a predictable unique identifier for the callback.For example,
add_action
signature could become:and developers could do:
and then:
I'm using a colon as prefix to avoid conflicts with a real functions, because no function name can contain a colon, so that make clear that I want to use an identifier, being completely backward compatible.
In case no identifier is provided when adding the hook, WordPress could create an identifier using the "Brain Monkey" way.
Two things to consider: first should be misured the impact on memory of adding an additional string for each hook callback in an average WP installation (which does not include just core).
Second, to distinguish closure by parameters requires heavy use of Reflection so it is probably worth to measure performance and see if it make sense. In the worst case, having a single string identifier like
"function()"
would be better than nothing, expecially if developer would have a way to provide a custom identifier as proposed above. Even because my experience says that used parameters names are very often the same for all the claosures added to the same hook, so they don't add much in making a distinction.