Closed joyously closed 3 years ago
Tkt_Options
is not useless. It does 2 things: it centrally gets the options, puts them in an array, and prepares them for safe usage (escape). In the Template (where we render the maintenance template) later we can simply instantiate an object with this class, and we can then receive the options in a clean form (no need to escape, or worry about how these options are structured, since they will be delivered ready to use by the class. If we do not use the class, we call get_option on each and every single time we need the option and need to escape it each time. You can even discover the options if you don't know their names, by doing a print_r of $options->get_options();
(where $options is an instance of Tkt_Options
). This allows programmers to create their own templates using the options without even knowing the option names. It wouldn't be possible if you do not have this class, since you would have to know the option name, to get_option('option_name').
1a. In the file you mention (lines 92 to 94 of class-tkt-maintenance-public.php the class delivers us those options, sanitised. BUT, since they might be empty, we want defaults, thus we check if the option is empty or not and set the default.
Do I understand you right that it would thus be better to set the default in the get_options directly? Thus avoiding the if/else Elvis operators here?leaving open for eventual feedback on 1a
The options class Tkt_Options is not useless. It does 2 things
To make it a class is useless. It could be a function much easier. And if your plugin did anything with its options besides display, it would be a bad thing to manipulate them for output when retrieving. Since this is such a simple plugin, it probably works fine. If the options were an array, you would have one call to get the option, which core already provides. Each one wouldn't have to have the plugin prefix on it and pollute the database with lots of entries. If you chose to use the Customizer for the interface instead of the Reading settings page, you'd have a preview and even more support for handling them as an array.
Do I understand you right that it would thus be better to set the default in the get_options directly?
Yes, but. Using separate entries, it's best to provide the default to get_options()
. If you used an array, there would be an array as the default. It's good to make one function to supply the defaults, and that is filtered so it can be extended. Since you shouldn't write to the database unless you are told to (and never know if what was written is still all there), you have to supply defaults.
but pretty sure these do not have/should not be translated.
OK, I couldn't tell by the name if it was a message the user would see or a standard.
There are some code formatting inconsistencies in here that make it more difficult to follow the code.
https://github.com/TukuToi/tkt-maintenance/blob/18c552f0089296be71e39d1c9521a61df0f6d6e1/public/class-tkt-maintenance-public.php#L92-L94
The way the options are handled is quite awkward. The options class doesn't help at all, and just makes it more difficult to read and use.
https://github.com/TukuToi/tkt-maintenance/blob/18c552f0089296be71e39d1c9521a61df0f6d6e1/public/class-tkt-maintenance-public.php#L92
This is not translated.
https://github.com/TukuToi/tkt-maintenance/blob/18c552f0089296be71e39d1c9521a61df0f6d6e1/public/class-tkt-maintenance-public.php#L134
Couldn't you spoof this with the login URL as a query on a real URL?
https://github.com/TukuToi/tkt-maintenance/blob/18c552f0089296be71e39d1c9521a61df0f6d6e1/public/class-tkt-maintenance-public.php#L171
Using
current_user_can()
with a role is highly discouraged. Would it even work right for multisite? Should this logic have some parentheses, or are you trying to use short-circuiting? If they aren't logged in, they can't be administrator. Same withmaybe_run_maintenance_mode