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.65k stars 2.25k forks source link

Drawbacks to using custom add_action and add_filter methods #396

Open yaronguez opened 8 years ago

yaronguez commented 8 years ago

I've been using this boilerplate for years now...thanks for your hard work! One thing that's always bothered me is the user of custom add_action and add_filter methods. I understand the benefit of loading all of the plugins hooks at one point but the implementation breaks WordPress convention and some fantastic IDE integrations like PHPStorm which allows developers to quickly navigate to the callback functions passed into add_action and add_filter as well as to find invocation points of the hook (https://confluence.jetbrains.com/display/PhpStorm/WordPress+Development+using+PhpStorm). Once you've used this type of integration, you just can't go back.

An easy and more "WordPress-y" solution would be to execute do_action('[plugin_name]_load_hooks') in the run method. The various define_[foo]_hooks() methods currently in class-plugin-name.php could be enqueued with standard add_action('[plugin_name]_load_hooks', array($this, 'define_[foo]_hooks')) instead. In other words, why build a system for building an array of functions to call when WordPress already offers you one?

I'll create a pull request when I get a chance but I figured I'd open the discussion here first.

G-Rath commented 7 years ago

@yaronguez please have a look at this! https://youtrack.jetbrains.com/issue/WI-37749

It's an issue that hopefully should help with this, that came about because of this exact thing.

it's about adding support for custom add_action and add_filter methods, that PHPStorm will pickup on and treat like the native versions.

As I detailed in my comment on the issue, it might be nice/easier if the loader altered it's add_action and add_filter methods so that their signature match the native ones (i.e take a callable as the second parameter, instead of two separate parameters).

It shouldn't be too much trouble, since the loader actually combines the two parameters into a callable when it's calling add_action and add_filter. I've modified my version of the loader to do this and it's working just fine.

yaronguez commented 7 years ago

Thanks for the update! I still see no added value at all from using a custom loader class rather than calls to native add_action and add_filter calls. It just adds complexity and moves away from standards. All of my class now have their own load_hooks method which register their respective hooks. I then instantiate these classes in a master Plugin_Hooks class which in turn calls the respective load_hooks method on each instance as the last step of the root class' run method. Very straightforward. If you prefer, you can register the hooks from the Plugin_Hooks class instead of each class' load_hooks method but I find it a bit easier to navigate through a code base if each class' hooks are registered within the class itself.

On Mon, Aug 28, 2017 at 7:39 PM, Gareth Jones notifications@github.com wrote:

@yaronguez https://github.com/yaronguez please have a look at this! https://youtrack.jetbrains.com/issue/WI-37749

It's an issue that hopefully should help with this, that came about because of this exact thing.

it's about adding support for custom add_action and add_filter methods, that PHPStorm will pickup on and treat like the native versions.

As I detailed in my comment on the issue, it might be nice/easier if the loader altered it's add_action and add_filter methods so that their signature match the native ones (i.e take a callable as the second parameter, instead of two separate parameters).

It shouldn't be too much trouble, since the loader actually combines the two parameters into a callable when it's calling add_action and add_filter. I've modified my version of the loader to do this and it's working just fine.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DevinVinson/WordPress-Plugin-Boilerplate/issues/396#issuecomment-325538152, or mute the thread https://github.com/notifications/unsubscribe-auth/AAXdw1g7wj1dbzchAx_EZQac2mdUM61yks5sc3nMgaJpZM4J2gnN .

smileBeda commented 3 years ago

@yaronguez - I am trying to understand the reasoning here.

By what I known, It has nothing to do with "custom" loader that you can't navigate to the call-back with IDE. It is due to the object->method passed, versus the direct function in same or globally instantiated file.

Also the other argument I've seen in the issues of this repo, being "cannot remove actions or filters when using custom loader" is not due to the custom loader at all. It is simply due to the object->method passed in array, which simply requires you to either remove the hook from the Class instance, from the class itself if static method, or, directly from the $wp_filter global like I explain here.

While I initially also thought this loader is just redundant - since I added the remove logic seen in above comment, the loader became a useful thing. What is truly bugging me however is, have a loader or not: the problem with IDE and remove action or filter will stay, as far I know, because literally we can not call action and filter "natively" in a class, because it requires the object to be passed in array along with method, unless of course you define your callbacks globally, outside a class (like say in the plugin main file as a "normal" function). If we do not pass array($object, 'callback'), then the method simply wont be found, if its inside a class.

Thus, I do not understand why the several moves against having a loader. While it might be "redundant" for small projects not having a remove logic for filters and actions, it surely wouldn't alleviate the core problem mentioned to be caused by that loader by removing it. The problems would stay. And on top creating the "remove" actions and filters logic would require more code, like shown in above comment.

I am quite sure though, I miss something fundamental, and since I forked this project in a maintained copy at https://github.com/TukuToi/better-wp-plugin-boilerplate, I would truly like to understand your reasoning in removing the loader, because I do not like things being there "just because it is cool".

I'd appreciate your feedback and knowledge sharing, if you have time and are interested :)