WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 2 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:
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 callback
  • ClassName->* any concrete method callback
  • class() verbatim, anonymous class instance
  • function() a closure instance
  • function( $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 (25)

This ticket was mentioned in Slack in #core-coding-standards by rarst. View the logs.


6 months ago

#2 @netweb
6 months ago

  • Focuses coding-standards added

#3 @giuseppe.mazzapica
6 months ago

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 method WP_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 to remove_filter or has_filter, _wp_filter_build_unique_id could continue to be used.

Moreover, an additional parameter could be added to add_action and add_filter allowing a way to provide a predictable unique identifier for the callback.

For example, add_action signature could become:

<?php
add_action(
  string $tag,
  callable $function_to_add,
  int $priority = 10,
  int $accepted_args = 1,
  string $indetifier = null
)

and developers could do:

<?php
add_action( 'save_post', function ( $post_ID, $post ) {}, 42, 2, 'my_plugin_save_post_cb' );

and then:

<?php
remove_action( 'save_post', ':my_plugin_save_post_cb', 42 );

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.

#4 follow-up: @iandunn
6 months 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 @Rarst
6 months 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 @giuseppe.mazzapica
6 months 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.

Last edited 6 months ago by giuseppe.mazzapica (previous) (diff)

#7 @Rarst
6 months 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 @giuseppe.mazzapica
6 months 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.

Last edited 6 months ago by giuseppe.mazzapica (previous) (diff)

#9 @giuseppe.mazzapica
6 months ago

For closures, what could be done is:

  • store "function()" in the hook class
  • if exactly "function()" is passed to remove_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: @azaozz
6 months 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: @johnbillion
6 months 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 @Rarst
6 months 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: @schlessera
6 months 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 @azaozz
6 months 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 @azaozz
6 months 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 @Rarst
6 months 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: @schlessera
6 months 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.

Last edited 6 months ago by schlessera (previous) (diff)

#18 in reply to: ↑ 17 @schlessera
6 months ago

I forgot to mention two things:

  • Yes, the RemovableClosure is a callable 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.
Last edited 6 months ago by schlessera (previous) (diff)

#19 follow-up: @giuseppe.mazzapica
6 months 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 @schlessera
6 months 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 @Rarst
6 months 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 @pabline
4 months 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 @adamsilverstein
2 months 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 @Rarst
2 months 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 @azaozz
2 months 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.

Last edited 2 months ago by azaozz (previous) (diff)
Note: See TracTickets for help on using tickets.