tkt-maintenance

[joyously]

https://github.com/TukuToi/tkt-maintenance/blob/18c552f0089296be71e39d1c9521a61df0f6d6e1/tkt-maintenance.php#L43

https://github.com/TukuToi/tkt-maintenance/blob/18c552f0089296be71e39d1c9521a61df0f6d6e1/tkt-maintenance.php#L52

https://github.com/TukuToi/tkt-maintenance/blob/18c552f0089296be71e39d1c9521a61df0f6d6e1/tkt-maintenance.php#L75

These functions should be prefixed, not suffixed. Use one prefix across the whole plugin.

The last one seems a bit odd. You define the function and then call it. Why make it a function?

https://github.com/TukuToi/tkt-maintenance/blob/18c552f0089296be71e39d1c9521a61df0f6d6e1/tkt-maintenance.php#L64 Seeing your folders named admin and public, it sure seemed like you should do a check right here and only load (require) admin or public. Why is it a class? You could separate it easier if it wasn't a class, or make two classes. Edit: sorry, I think you do have more than one class, as evidenced by the names in the activate and deactivate functions.

[smileBeda]

@joyously - my plugins are all based upon the Boilerplate (and also the base code is always generated with that) suggested here: https://developer.wordpress.org/plugins/plugin-basics/best-practices/#boilerplate-starting-points This is the boilerplate https://github.com/DevinVinson/WordPress-Plugin-Boilerplate and this is the generator I use to kickstart the plugins https://wppb.me. They all use this syntax. As long the Function name is unique, which it is, I don't see an issue with this (and it would cost me tons of work to redo this, couldn't use the generator, and would need to diverge from that boilerplate)

Thus I would prefer not to change this.

Is there any crucial reason to still change it?

---

As for The last one seems a bit odd. You define the function and then call it. Why make it a function? I assume you refer to run_tkt_maintenance ()? That is just how OOP works. We instantiate the object, we call the object method to kickstart the logic, and we call it in a function, because we might want to do more things inside the function, that can't be done in the class (here not the case, but it might). It just keeps the code clean. It is the wrong place to check if is admin or not, because that distinction is not always Cristal clear, and also, we are instantiating the plugin here, not yet the single screens or more detailed things, which are defined later.

The plugin is (attempts to) structure public, admin, and includes (which is basically neither admin nor public, but both or none, if you so want).

---

I think I will leave this as is, mainly because it is how it is done in the boilerplate but also because all my plugins (and I have to many of them to change this at this stage) follow this structure. Unless there is a core rule that says "this is not allowed", of course.

Leaving it open for eventual feedback...

Thanks!

[joyously]

Is there any crucial reason to still change it?

Nothing crucial, but the best practices are generic. This plugin is so simple, it is way overkill to use a class and I don't think it's "best". This was the first file I looked at, so at the time I didn't know that you actually have many classes that all duplicate the fields, so it really defeats the whole purpose of a class, which is to encapsulate data and methods on that data. By its very nature, PHP is a filtering language, as you can hop in and out of it from line to line. Forcing it to do OOP when it's not needed is inefficient and hard to read.

As long the Function name is unique, which it is, I don't see an issue with this

Whatever. My tendency to a single prefix comes from reviewing themes. And anyone looking in the code appreciates being able to tell at a glance where the failing function is from.

I assume you refer to run_tkt_maintenance()?

Yes. Looking at it again, I can see that the function declares a variable for the object, which would be global if you did that without the function. But then the function ends, so what happens to the object? And you don't have to call a function to kickstart it. The constructor could do that, since it's just adding hooks and filters like you could do in the main file anyway.

It just keeps the code clean.

No, just indirect when it isn't needed. It looks very messy to me.

It is the wrong place to check if is admin or not,

Yes, plugin_load time is not when to check admin or not. But it is the right time to add_action for 'admin_init' to load only the admin code (and check if it's the right page for that) or whatever action for the front end, where you can check is_admin() to load the front end stuff. I'm just saying that it looks like the PHP is getting loaded regardless, when it doesn't have to.

but also because all my plugins (and I have to many of them to change this at this stage)

I wasn't reviewing your other plugins. There are some scenarios where a class is a good idea. This isn't one of them.

[smileBeda]

Thanks for this feedback. I will consider this in (this or future) development, for now, I leave it as is, also because WP Review Team did not complain about this, and I believe starting with "big" in mind is always better.

This plugin is small, but it might grow, perhaps get its own settings screen, and perhaps even a builder for the maintenance mode (just a tiny one) one day, depending on user feedback.

Thus for now I am leaving the standard approach used here and in other of my plugins as it simply makes my coding easier, than changing this now. I am not closing the ticket for the purpose of keeping this feedback close.

However I see your points, and will consider the, also they helped me review my own intentions versus "just use a boilerplate"

Thanks!!