search

TukuToi / tukutoi-maintenance

Enable and Control a Custom Maintenance Mode for your WordPress Website.
https://www.tukutoi.com/program/tukutoi-maintenance/
GNU General Public License v2.0
0 stars 1 forks source link

class-tkt-maintenance #11

Open joyously opened 3 years ago

joyously commented 3 years ago

https://github.com/TukuToi/tkt-maintenance/blob/18c552f0089296be71e39d1c9521a61df0f6d6e1/includes/class-tkt-maintenance.php#L17

Maybe it's just me, but you shouldn't use the word "core" anywhere in a plugin.

https://github.com/TukuToi/tkt-maintenance/blob/18c552f0089296be71e39d1c9521a61df0f6d6e1/includes/class-tkt-maintenance.php#L22

This contradicts the fact that the plugin headers are in the main file and there is a define there for the version number.

https://github.com/TukuToi/tkt-maintenance/blob/18c552f0089296be71e39d1c9521a61df0f6d6e1/includes/class-tkt-maintenance.php#L110

Why load both admin and front end for every request? It's either one or the other, not both.

https://github.com/TukuToi/tkt-maintenance/blob/18c552f0089296be71e39d1c9521a61df0f6d6e1/includes/class-tkt-maintenance.php#L155

Why do you need a whole class for a single function call?

https://github.com/TukuToi/tkt-maintenance/blob/18c552f0089296be71e39d1c9521a61df0f6d6e1/includes/class-tkt-maintenance.php#L140

Why is the loader separate? You have a main class to load the loader. Weird.

smileBeda commented 3 years ago
  1. I hear this the first time, and it is used in the recommended WP Plugin Boilerplate. However, I see your point. Core is core, adding features is not core. I removed this and will try to avoid it in future. I used "main" instead.
  2. No, this does not contradict with the WP Default Plugin header. In fact, here we define using a Constant, which is defined in the main plugin file. That value, along with the plugin header value are the only ones that ever need to be touched. Why we define it here is that maybe we might need to check on plugin version (for example if we update the plugin and maybe include non-backwards compatible updates). We do this here instead of just listen to the TKT_MAINTENANCE_VERSION because that constant might not be defined, for whatever reason, thus, we go safe and define a class property with the variable to be used thru the entire plugin. This value falls back to a default version just in case constant is not defined.
  3. Right, we could separate this, but as said earlier, unfortunately admin and front end is not always so Cristal clear separated. Thus I prefer to separate later (unless, the dependencies are 100% only backend or only front end). Since we use all other dependencies listed here potentially both in backend and frontend, and the only 2 "separate" dependencies do not outnumber all dependencies, I decided to keep the boilerplate structure here. If there would be many more deps, I would probably separate this earlier, agreed.
  4. Actually - plain and simple because it is like that in the boilerplate, and I assume it is made like that so we could load more functionality in that class. Honestly, I usually delete any translation related stuff. I left it here, because the plugin is intended for repo, thus, also translation. Is there any problem with loading a class instead of a single function? Since the entire Plugin is OOP I would prefer to leave it. However, if there is some strong concern or rule speaking against it, I would consider removing this
  5. Agreed, never even noticed this. The method is never used, it is a remnant from the boilerplate, and I clearly also do not see any need for this, it would simply return is the same as the actual class object we define earlier. Before I remove it, I want to clarify why this was added and what the intention of it was/is, thus, https://github.com/DevinVinson/wppb-demo-plugin/issues/4

Keeping this issue open for eventual followup after WPPB replies.

joyously commented 3 years ago

Since the entire Plugin is OOP

Many programmers think they are doing OOP, but it is really object-based, and a mess. You have no real objects, since none are needed in this plugin. And any object that needs a controller or outside thingy to "make it go" isn't designed as OOP. The WordPress environment, and PHP, are not the right place for OOP because you have to force it. I don't agree with using classes just to use classes, but if you choose to do that, don't call it OOP when it's not.