About Design and Architecture of the Plugin Code and Files

[smileBeda]

This was originally the introduction Discussion, however since the thread discusses design and architecture the introduction thread has been moved to #15

[LoonSongSoftware]

Thanks for offering this discussion and for continuing (I hope) the fork of WordPress-Plugin-Boilerplate.

I've invested some time reviewing the code and have a few design-type questions. I'm trying to understand to what degree some of the choices are just "because that's the way it's been for a long time" vs. those that have strong rationales behind them. (I'm a long-term programmer, but have only been doing WordPress stuff--among many other things--for the last couple of years. So I'm not so arrogant as to think that I know the best way to accomplish something in WordPress.)

Anyway, here goes:

• In the Plugin_name class, why are both define_admin_hooks() and define_public_hooks() called in the constructor? Why not put an is_admin() test in place around those calls to see what kind of page is being rendered (and define only the relevant hooks)? Unless it affects ajax calls or something, it would seem to be a way to eliminate some unnecessary code execution (like setting public actions/filters when rendering an admin page).
• Similar question about wrapping some of the load_dependencies() files/classes in is_admin()? Why read the full public/class-plugin-name-public.php file, for example, when rendering an admin page? I thought the design principle was to have admin-only code in admin/, public-only code in public/, and mixed code in include/.
• Why are these functions (define_xxx_hooks) in the Plugin_Name class anyway? Why not put them inside the respective admin (Plugin_Name_Admin) and public (Plugin_Name_Public) classes? Moving them into those classes would help keep the core functionality of those classes together in the same file (which, I would think, would improve readability, maintainability, and "portability" of those classes).

Thanks for you help in clarifying this stuff.

Scott M.

[smileBeda]

Hi @LoonSongSoftware and welcome!

Related to the 3 points you outline, let me reply one by one and hope I can shed some light in these doubts. I am pretty sure about 1 and 2, but #3 I am not entirely sure I got it right.

1. This is exactly what one would do depending on the use case. I do that usually in my plugins but it is not always possible - for example due to AJAX (where is_admin will be true but you will not necessarily be in the admin) or the customiser where is_admin is false, but you are in the admin. Those are just a few of the cases where a is_admin check is not always possible and thus, has been omitted to the leave it to the programmer to further enrich the code with their use-case requirements.

2. Similar answer here. is_admin is more often than not an inadequate check. Much more effective are capability checks or, if is_admin is used, usually you want to use it together with an AJAX check too depending on what you do. Of course, in certain cases it makes perfect sense to load something only if is_admin (or whatever other check) but as a base skeleton, not knowing what you'll do with the plugin, I think it is best to not include such checks. Similarly for example the main plugin file will initiate the main class globally and not Hooked to any hook. There is a comment however stating that depending on what you do you should hook the main plugin function to a hook. The same idea applies to the is_admin checks. Bz default the plugin doesnt check this, because it cannot know where you will need that check (sometimes it is possible to wrap the entire define_admin_hooks or define_public_hooks in such check, and sometimes, you have to granularly control single hooks inside it)

3. I think this is because we bootstrap the entire plugin here. Note there may (and will usually) be more files not just the Plugin_Name_Public or Plugin_Name_Admin classes, but perhaps a ShortCodes Class (in its file) or whatever else. With the current setup you will know that immediately reading the main plugin class file what is loaded, to what hooks it is loaded and probably even what it does. If we put those into the single files, all we would see are the includes of the single files and initiate of the single classes but not what is in those classes. So I think the organization is arranged like this to see in one place what happens where. I say I think because ... I am actually not the original creator of this boilerplate and my programming path begun 8 years ago as a supporter - so I am totally self-schooled. Thus I can only think what is or was the intention here and it makes sense to me, but I am happy to stand corrected.

Thanks for offering this discussion and for continuing (I hope) the fork of WordPress-Plugin-Boilerplate.

Thanks for initiating a discussion ;)
I very much intend to continue the development of the Boilerplate also with the help of hopefully a strong community around it. Other than the original Boilerplate (which dodged each and every PR or suggestion) I intend to let the community have a word and contribute. With this already great things came into the plugin like ShortCode support and unhooking actions and filters. Thus you are very welcome to discuss, suggest changes and push code changes yourself as well. This is intended to be a community effort, where the community does have a say.

Thanks for your input and let me know if any doubts come up or something is not clear!

[LoonSongSoftware]

Thanks for the quick reply. 1: I get it. I didn’t mean to imply that is_admin() was the only (or best) way to go. I was more asking about the boilerplate’s default of loading both/everything with every page display. 2: Ditto. Thanks. 3: Your explanation makes sense (and I recognize the history of the code). I can see the benefit of seeing all of the WordPress hooks in the base plugin class. Personally, though, I prefer to keep them encapsulated within the relevant classes on the theory that it improves the “object”-ness of the classes—expose a streamlined public interface and keep the execution guts in a single (class) file. Both approaches have their advantages and disadvantages. Thanks again!

[smileBeda]

Hey @LoonSongSoftware - great.

Honestly, I do probably not have the same experience in PHP programming like you do, so maybe I am all wrong. I had/have similar doubts when I started using the Boilerplate and the explanations of the original makers where rather confusing, so I think they just tried to explain status quo with whatever seemed nice.

I for me now am used to this setup (and I guess many users are too, since several plugins are made with it and it helps as well to recon the same patter over several different plugins.

I would like another opinion here, because I am all open to change this and make it more professional. If the general take is that hooks should be encapsulated more we can certainly do that.
I am also happy to accept PRs, so if you have suggestions in form of code feel free to push

I am asking @nylen here, he is the lead dev at ClassicPress and has a great experience in things WordPress and PHP for his opinion.
Perhaps he has some valuable input about the points you mentioned that will help to improve the boilerplate even more.

[nylen]

I think this is pretty subjective and comes mostly down to personal preference. Maybe I should, but I don't usually use classes very much in plugins, unless I really am creating a whole bunch of some kind of object and passing them around. A Plugin_Name is an object, so is a Plugin_Name_Admin_Page probably, but is a Plugin_Name_Public really an object? We probably won't ever create more than one of these, so what's the difference between that and some bare functions that don't live in a class at all? Again, personal preference.

As far as the specific points raised, I'm not sure I have much to add that hasn't already been said above:

(1-2) Not having the is_admin() checks by default makes sense because it could mislead developers (and really the performance impact should be negligible in case someone doesn't change it, unless a whole lot of code needs to be loaded). Maybe this would be something to document though: what common cases should use is_admin() and what other checks you might want in certain situations instead.

There are also vulnerabilities that come up from time to time because is_admin() is used as a capabilities check - might also be worth documenting that this is doing it wrong!

(3) Where to place the hooks: I could argue this one either way. Keeping the hook initialization all in one place makes it easier to see a summary of which hooks are used, but moving the hook initialization next to the actual code that gets run is probably better encapsulation. Personally I usually put the hook initialization next to the code that gets called, and I don't bother with a check (i.e. always call add_filter for an admin hook, because I know that it'll only be called when it's needed and the call to add_filter is super cheap).

[smileBeda]

Thanks @nylen - you touch a point that has been made by others before in relation to this boilerplate elsewhere:
just using classes does not really make a code object oriented, and often that's total overkill, specially in the plugin (but also theme) case.

Again I think (and definitely know it to be the case for my part of the story) originally it was created like that to kinda "show" how to use classes and be consistent, but I feel it is misleading and making folks think they need to use classes for all and everything, while really that is not the case.

I will see if I can find time to create a Wiki for this plugin explaining the single parts and their possible usage and also include a word on classes, and why the plugin is structured the way it is.

[LoonSongSoftware]

Since this is a “discussion” area, I’ll respond. I agree in general with @nylen; mostly, I was looking to understand the philosophy of the boilerplate, so I could figure out which comments would be considered improvements vs. those that would be entirely personal preferences. On the subject of “single-use classes,” I find them useful for a couple of reasons: 1. They provide a structure to encapsulate pretty much all of the functionality related to a section of code, so users (or those with short memories) only need to focus on the ‘public’ members for an otherwise-working class. Personally, I’d rather manage five classes, each with 10 public functions and 10 private ones rather than a monolithic file (or files) with 100 functions. But I recognize that that’s a personal preference. 2. By splitting up the code into classes, it becomes relatively easy to turn features on and off to improve efficiency and site responsiveness. If it’s clear from the WordPress environment that one or more big chunks of code aren’t needed, why not skip the ‘includes’ of the related files and execution of the class constructors? Especially when the constructor is resource (CPU, DB access, filesystem) intensive). I don’t think classes should be used “for all and everything,” but I do tend to rely on them to break down complex code into its component parts for the reasons given above. I think a wiki would be a great idea, and I’d be happy to help. Scott

[smileBeda]

I think the wiki is really a good idea, yes.
I have started here https://github.com/TukuToi/better-wp-plugin-boilerplate/wiki
WIP.

You are very welcome to help there :D

The start I made is merely what came to mind now quickly, perhaps we should adapt/change this, of course, open to edits and discussion.

PS I also moved the opening comment of this thread to a new thread since this one here is nicely about plugin architecture and thus we can streamline tings...