DevinVinson / WordPress-Plugin-Boilerplate

[WordPress] A foundation for WordPress Plugin Development that aims to provide a clear and consistent guide for building your plugins.
http://wppb.io
7.67k stars 2.25k forks source link

remove_action, remove_filter #217

Open javorszky opened 10 years ago

javorszky commented 10 years ago

These two are missing from the codebase. Was it a conscious omission, or am I missing something and the LOADER->add_filter and LOADER->add_hook can deal with removing actions and filters?

tommcfarlin commented 10 years ago

That was an omission; however, this is something that should definitely be put into place. I'll look to resolve this when I have a chance.

Thanks!

jjeaton commented 10 years ago

Would remove_{action|filter} be general purpose, as-in you would use that to remove any action or filter added by any other code, or only a way to remove actions and filters that have been added using this abstraction?

I think it would only be possible/useful to have it work with hooks added with the abstraction because while you can do loader->run() on plugin load and add all your hooks there, removing hooks happens on an adhoc basis (removing hooks right after they have been added in loader->run() seems a bit silly)

So now I'm curious how we would implement this, and what the use case is for a remove_hook abstraction? The remove method just drops it from the actions or filters variables and then immediately calls the appropriate remove_ function?

javorszky commented 10 years ago

In all honesty I can't yet see why the add_hook abstraction is there in the first place versus what we used to do in 2.6 : add_action( 'hookname', array( $this or 'classname', 'functionname'), 10, 4 );.

I want to remove some of the things WordPress attaches to itself, or WooCommerce, or anything else. The problem is, currently I can only do that via a normal 'remove(action|filter). But then again, I could just use theadd(action|filter)` without going through the abstraction.

So ideally it would be a general use remove filter. I'm putting together a PR at the moment, although if it's general, then the $hook['component'] and $hook['callback'] properties are ... less than ideal to work with.

jjeaton commented 10 years ago

I would say if you're removing an action/filter added by anything outside of the plugin, then you should just use the normal:

remove_action()

Any abstraction for removing a plugin's own hooks, say if you are adding a hook to the_content but need to remove it and re-add it in certain instances, could be handled by the abstraction...

However...hmm...I don't think the current abstraction could handle that use case either. The only place the hooks actually get added is in loader->run() @tommcfarlin ?

tommcfarlin commented 10 years ago

All hooks are added by $loader->add_action( ... ) and $loader->add_filter( ... );

The reason all of this is abstracted into its own class is primarily because it introduces a separation of concerns and has a single place in which all hooks are registered. $loader->run() registers everything with WordPress when the plugin fires.

In short, it keeps a single class from having to maintain its business logic, its hook registration, and its hook logic. It's only aware of the $loader and the loader then does all the lifting when it comes to registering hooks with WordPress.

We could technically do a remove_action( ... ) outside the loader; however, if we're adding a hook with the loader, it makes sense that it can be removed with the loader.

Anything that's added via third-party plugins or outside the loader is up to the developer to handle that.

danielpataki commented 10 years ago

On my end I am adding hooks which I want other plugin/theme developers to be able to overwrite. For example: my plugin uses Advanced Custom Fields. By defining a constant we can remove the UI for ACF since it is usually not needed. However, I want other developers to be able to overwrite this.

Due to this we have the following:

private function define_admin_hooks() {
    $this->loader->add_action( 'plugins_loaded', $plugin_admin, 'disable_acf_menu' );
}

Is there any way a developer could remove this action from outside our plugin or should I define these elsewhere, circumventing the loader class?

Thanks for your input!

tommcfarlin commented 10 years ago

@danielpataki right now, there's no way to remove actions using the loader class. The idea behind it is to setup hooks for your plugin (though it should offer remove_action functionality and it eventually will).

To circumvent the loader, you can use the standard WordPress add_action and remove_action calls to remove whatever it is that you need to remove.

danielpataki commented 10 years ago

Hi Tom,

Thanks for weighing in super quickly. I had a look at the class and I thought so, but it was worth a quick question :)

My main concern with this is letting others remove my hooks from outside the class. If there is something I can do to help, let me know!

tommcfarlin commented 10 years ago

My main concern with this is letting others remove my hooks from outside the class. If there is something I can do to help, let me know!

They can still do that using the standard WordPress API. It's primarily setup the way that it is in the plugin to maintain a strong separation of concerns so the plugin is only aware of the loader and the loader is responsible for registering the hooks and what not.

It keeps the primary plugin class from getting too long and mixing it's responsibility with adding, removing, and so on :).

danielpataki commented 10 years ago

Oh really? Can you let me know how? Since the actions are registered with objects, as opposed to arrays pointing to static functions

add_action( 'hook', array( object , 'function' ) );
// instead of
add_action( 'hook', array( 'string', 'function' )  );

I've had trouble removing these. Perhaps I'm missing something, I'm sure my knowledge is a bit loose on this.

tommcfarlin commented 10 years ago

We may be miscommunicating, but the way in which someone would remove a hook on your plugin would be:

  1. Get a reference to the instance of the plugin
  2. Remove an action using said reference

For example:

$plugin = new My_Plugin();
remove_action( 'hook', array( $plugin, 'function_name' ) );

Note that you may need to either create a singleton or a container (I prefer the latter) to maintain a reference to the plugin, and the function_name will have to be a public function; otherwise, WordPress can't access it.

danielpataki commented 10 years ago

Nope, that's what I was looking for and it is indeed something I tried, but it doesn't seem to work. I'm sure this is my OOP failing me, not the plugin so let me know if you don't have time for this.

I tried this using a hook which will definitely not have problems being removed from a theme's functions file: wp_footer. At the bottom of my plugin's main, outside of any class I did this:

add_action( 'wp_footer', 'test' );
function test() {
    echo 'testing';
}

In the theme's functions.php this could be removed using remove_action( 'wp_footer', 'test' ), no surprise there. I then added it within the define_admin_hooks() function in the main class like so (I added the function in the admin class as well of course)

$plugin_admin = new Pile_Gallery_Admin( $this->get_Pile_Gallery(), $this->get_version() );
$this->loader->add_action( 'wp_footer', $plugin_admin, 'test' );

In the theme's function file I tried these:

$plugin = new Pile_Gallery();
remove_action( 'wp_footer', array( $plugin, 'test' ));

$plugin = new Pile_Gallery_Admin( 'pile-gallery', '1.0.1' );
remove_action( 'wp_footer', array( $plugin, 'test' ));

The first thing I did was to use the actual instance by doing this:

function run_Pile_Gallery() {
    $plugin = new Pile_Gallery();
    $plugin->run();
        return $plugin;
}
$plugin = run_Pile_Gallery();

Sorry for being a but stupid :) If you're interested in the actual plugin it's here :)

afragen commented 10 years ago

@danielpataki try the following. Make sure function test is declared as public static function test() then change to remove_action( 'wp_footer', 'Pile_Gallery::test' );

I've never been able to use add_action or add_filter from a class object without it being called as a static function.

danielpataki commented 10 years ago

Thanks @afragen but no luck! The problem is not really the type of method (static or not) but how it is registered in the first place.

@tommcfarlin do you think it is necessary to use the object instance to register the hooks? Since objects are used WordPress uses some sort of method to turn them into dynamic strings (I'll look in the core if I can). So the end result is something like this being used as the hooked function: 000000007a6eb68900007f4d65036704disable_acf_menu. The string at the beginning is different on each page load.

This means that any change of targeting it with 'Plugin_Name::function_name' or any other method other than providing the actual instance shouldn't work. However, providing the actual instance doesn't seem to do the trick either.

I have a feeling there is a solution to this, but even if there is, is it necessary to reference the object? Isn't a static name using the plugin's naming convention enough? For example: add_action( 'pile_gallery_remove_acf_menu' ). There is a low chance of this conflicting.

Overall I personally like referencing the instance better, but it doesn't seem to work :)

danielpataki commented 10 years ago

Ok, after a long-long loop, going through the core files I figured out the issue. When adding and removing hooks WordPress uses the _wp_filter_build_unique_id function which generates a unique ID for each plugin. If the hooked function is an object it creates a unique string ID.

This ID is different for each instance of an object so you must remove the hook with the same instance you registered it, not the same object.

I tested this using the following method: In the define_admin_hooks function I did this:

    private function define_admin_hooks() {
        global $plugin_admin;
        $plugin_admin = new Pile_Gallery_Pro_Admin( $this->get_Pile_Gallery_Pro(), $this->get_version() );
        $this->loader->add_action( 'wp_footer', $plugin_admin, 'test' );

Since I globalized the $plugin_admin it is now available in the theme's functions file so this works:

global $plugin_admin;
remove_action( 'wp_footer', array( $plugin_admin, 'test' ) );

This is obviously an ugly solution, but it works. @tommcfarlin any ideas how this could be done better?

Hope I could help, at least I personally learned something about how hooks work under the hood :)

tommcfarlin commented 10 years ago

This ID is different for each instance of an object so you must remove the hook with the same instance you registered it, not the same object.

Exactly this! Nice work.

I need add a container class of sorts that will allow you to gain public access to a given instance of a plugin.

For example, using global is kind of a bad practice but if you were to have a class like:

class Plugin_Container {

  private static $instances;

  private static $instance = null;

  private function __construct() {
    self::$instances = array();
  }

  public static function get_instance() {

    if ( null === self::get_instance() ) {
        self::$instance = new self;
    }  

    return self::$instance;

  }

  public add_instance( $key, $instance ) {

    if ( ! in_array( self::$instances, $instance ) ) {
      self::$instances[ $key ] = $instance;
    }

  }

  public function retrieve_instance( $key ) {
    return self::$instances[ $key ];
  }

}

The above code is basically pseudo-code and it needs to be more defensive but the idea behind is that you can do something like this:

$plugin = new Plugin_Name();
$container = Plugin_Container::get_instance();

$container->add_instance( 'my_plugin', $plugin );
$container->retrieve_instance( 'my_plugin' );

From there, you'll have access to a reference to your plugin and can then do whatever it is that you need to do in order to unhook your actions.

mAAdhaTTah commented 10 years ago

Minor typo in the pseudocode, should be self::$instances for the first reference in the add_instance function.

My real question, though, is how is the container pattern different from the singleton pattern and what adv/disadv do each have?

tommcfarlin commented 10 years ago

Minor typo in the pseudocode, should be self::$instances for the first reference in the add_instance function.

Fixed - thanks for the heads up :).

My real question, though, is how is the container pattern different from the singleton pattern and what adv/disadv do each have?

The above code is a singleton and I'm calling it a container and was intended to be just an example to show how to gain access to an instance of the plugin. Technically, the container is meant to hold multiple instances of a plugin which is kind of outside the scope of what's intended for the Boilerplate (though the use case in this issue is a valid one).

Technically, a container object can be instantiated elsewhere in the application, then the plugin is instantiated, executed, and run; however, since WordPress activates plugins automatically, the traditional ways to go about doing this aren't don't necessarily work the same way.

danielpataki commented 10 years ago

Hi @tommcfarlin,

Thanks so much for the details, I'll get to work on it! I definitely do not want to use globals, it was just a proof of concept thing :)

What I don't quite understand is why this would be different than the following. Instead of this being done in the main plugin file:

function run_Plugin_Name() {
    $plugin = new Plugin_Name();
    $plugin->run();
}
run_Plugin_Name();

If I replace it with this:

function run_Plugin_Name() {
    $plugin = new Plugin_Name();
    $plugin->run();
    return $plugin;
}
$plugin = run_Plugin_Name();

Wouldn't this be essentially equivalent?

Also, there is one more problem. The actions are actually added by the Plugin_Name_Admin and Plugin_Name_Public classes so we would need access to an instance of those. Perhaps instantiating them in the constructor and adding them to a publicly available variable would be an alternative solution? Something like this (just typing up what is needed):

class Plugin_Name {
    public function __construct() {
        $this->plugin_name_admin = new Plugin_Name_Admin( $this->get_Plugin_Name(), $this->get_version() );
    }

    private function define_admin_hooks() {
        $this->loader->add_action( 'admin_enqueue_scripts', $this->plugin_name_admin, 'enqueue_styles' );
    }
}

Then, if we initialize our plugin like this:

function run_Plugin_Name() {
    $plugin = new Plugin_Name();
    $plugin->run();
    return $plugin;
}
$Plugin_Name = run_Plugin_Name();

We can unhook an action like this from a theme's functions file:

remove_action( 'admin_enqueue_scripts', array( $Plugin_Name->plugin_name_admin, 'enqueue_styles' ) );

As I mentioned before I'm really no good at OOP PHP. I understand the mechanics but the structuring and logistics are not my strong suite yet so I might be off with this one.

Let me know what you think!

tommcfarlin commented 10 years ago

Wouldn't this be essentially equivalent?

There are a number of different ways to get a reference to an instance of the object. I've provided a few, but what you've proposed works as well; however, it still leaves that variable in a state that you can't access it in a theme.

Perhaps instantiating them in the constructor and adding them to a publicly available variable would be an alternative solution?

Instantiating them in the constructor rather than passing an instance into the constructor via dependency injection creates a dependency that I'm trying to avoid. Rather than instantiate it there, instantiate it externally and pass it in.

From there, you have an instance referring to the classes you need and the plugin has them, as well.

Alternatively, you could write some wrapper functions in the core plugin file to give you access to what you need on those two other classes, as well.

danielpataki commented 10 years ago

Thanks for your input, I'll choose a method and just move on :)

Daniel

smileBeda commented 3 years ago

This is the easiest and shortest, without changing current plugin behaviour.

global $wp_filter;
$tag = 'wp_footer';//The tag to which your function is hooked
$pri = 10;//The Priority of the hook
$class = 'Tkt_Tree_View_Admin';//The class declaring the function
$func = 'echo_footer';//The function
$fob = $wp_filter[$tag];//The global wp_filter object of the tag

foreach ($fob->callbacks as $priority => $filters) {
    if( $priority == $pri ){//if the priority matches
        foreach( $filters as $filter ){
            if( $filter['function'][1] == $func && is_object( $filter['function'][0] ) && $filter['function'][0] instanceof $class){//if function matches, and if function object is of class
                $fob->remove_filter( $tag, array(  $filter['function'][0], $func ), $pri );//then remove the filter.
            }
        }
    }
}

Hope this helps.


This ( a tad more extended ) is now added as a core feature to the loader class of the boilerplate, in the new hard fork here https://github.com/TukuToi/better-wp-plugin-boilerplate > develop branch.

The new method can be used in any external instance like so:

$ = new Tkt_Tree_View_Loader();
$plugin_hooks->remove_action( 'wp_footer', 'Tkt_Tree_View_Admin', 'echo_footer' );