hyyan / woo-poly-integration

Looking for maintainers! - Wordpress WooCommerce Polylang Integration
https://wordpress.org/plugins-wp/woo-poly-integration/
MIT License
183 stars 66 forks source link

Removing filters/actions added by the plugin classes is not user-friendly #101

Closed decarvalhoaa closed 7 years ago

decarvalhoaa commented 7 years ago

This plugin has several classes that are instantiate like (example) new Cart(); instead of $cart = new Cart();.

Whenever a plugin creates a new MyClass();, it should assign it to a uniquely named variable. That way, the instance of the class is accessible. This is important to allow other plugins and themes to remove the filters and actions this plugin sets.

So if a class is instantiated like $myclass = new MyClass();, then you could do this:

global $myclass;
remove_action( 'wp_footer', array( $myclass, 'my_action' ) );

This works because plugins are included in the global namespace, so implicit variable declarations in the main body of a plugin are global variables. However, this plugin uses it own namespace, therefore may not be the desired solution. A better solution perhaps, is to add a static function to all plugin classes to retrieve the instance (see below for code snip).

TL;TR If the plugin doesn't save the identifier of the new class somewhere like so $myclass = new MyClass();, then technically, that's a bug. One of the general principles of Object Oriented Programming is that objects which are not being referenced by some variable somewhere are subject to cleanup or elimination.

Now, PHP in particular doesn't do this like Java would, because PHP is sorta a half-arsed OOP implementation. The instance variables are just strings with unique object names in them, sort of thing. They only work because of the way the variable function name interaction works with the -> operator. So just doing new class() can indeed work perfectly, just stupidly. :)

Read more at http://wordpress.stackexchange.com/questions/36013/remove-action-or-remove-filter-with-external-classes END TL;TR

We could add the following to every class:

namespace Hyyan\WPI;

class WooPoly_Class {
    // The single instance of the class.
    private static $_instance = null;

    // Ensures only one instance of SPH_Poly_Integration is loaded or can be loaded.
    public static function instance () {
        if ( is_null( self::$_instance ) )
            self::$_instance = new self();
        return self::$_instance;
    }
}

This would allow the following to retrieve the class instance:

$woopoly_instance = Hyyan\WPI\WooPoly_Class::instance();
remove_filter( 'woocommerce_cart_item_product', array( $instance, 'translateCartItemProduct' ) );

Read more at http://wordpress.stackexchange.com/questions/36013/remove-action-or-remove-filter-with-external-classes

Jon007 commented 7 years ago

Hi, fyi while attempting to do something similar on a different plugin a few weeks ago, I found some quite detailed discussion about why remove_filter does not work and should not be used, for example from here and elsewhere: https://magp.ie/2013/11/12/wp-plugins-how-to-remove-a-filter/ (WP Codex)[https://codex.wordpress.org/Function_Reference/remove_filter] also warns that "No warning will be given on removal failure." However add_filter with higher priority worked great! So I'm not sure what priority level should be given to this, if there is a use case that really makes it necessary to change.

I just noticed original issue was posted here: https://wordpress.org/support/topic/how-to-remove-a-filter/ where user had built a custom filter:

// disable the hyyan filter which causes some problems
function remove_hyyan_filter(){
    global $wp_filter;
    write_log($wp_filter["woocommerce_cart_item_product"][10]);
    if(isset($wp_filter["woocommerce_cart_item_product"][10])){
        foreach($wp_filter["woocommerce_cart_item_product"][10] as $key => $hook){
            if($hook["function"][1] == "translateCartItemProduct") unset( $wp_filter["woocommerce_cart_item_product"]->callbacks[10][$key]);
        }
    }
}
add_action( 'wp', 'remove_hyyan_filter' );

I also notice the built in WordPress function remove_all_filters which is also simpler to use if someone needs to really take over a filter.. .. so not clear if there is really a need for this..

hyyan commented 7 years ago

There is no use case that really makes it necessary to change the current behaviour, so closing