[wp-trac] [WordPress Trac] #46635: Improve identifying of non–trivial callbacks in hooks

[WordPress Trac]

#46635: Improve identifying of non–trivial callbacks in hooks
-------------------------+-------------------------------
 Reporter:  Rarst        |       Owner:  (none)
     Type:  enhancement  |      Status:  new
 Priority:  normal       |   Milestone:  Awaiting Review
Component:  General      |     Version:
 Severity:  normal       |  Resolution:
 Keywords:               |     Focuses:  coding-standards
-------------------------+-------------------------------

Comment (by giuseppe.mazzapica):

 @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
 <?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
 <?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.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/46635#comment:19>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform