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-options #16

Closed joyously closed 3 years ago

joyously commented 3 years ago

It would be better if the options were stored in an array in the database instead of many separate option entries. It would also be better if there were valid defaults used.

It is a bit confusing to have a function named set_options which doesn't actually set the options in the database. And then get_options calls set_options.

https://github.com/TukuToi/tkt-maintenance/blob/18c552f0089296be71e39d1c9521a61df0f6d6e1/public/class-tkt-maintenance-options.php#L62

Is this going to produce valid CSS? Core uses simply strip_tags() for the Additional CSS output.

smileBeda commented 3 years ago

It does produce valid CSS (tested)

I agree on the set_options and get_options issue. Will revise this. About options array or single options, this is just how native WP options api works. Each setting creates an entry. If it is recommended to push them all into one entry, why does WP not do this already or at least mention this in the DOC?

Not sure about the defaults either, In most cases we want empty as default, which is exactly what I pass here when I get the defaults. As for when I save the options, this is done in another place, and also there we offer defaults (empty, i.g)

I will keep this one open to revise the Tkt_Options class.

joyously commented 3 years ago

If it is recommended to push them all into one entry, why does WP not do this already or at least mention this in the DOC?

I was reading that doc yesterday, and it does mention that the type can be an array.

smileBeda commented 3 years ago

The CSS and JS thingy got removed as considered invalid by WP Review Team

I will keep the single options mainly because of this: https://github.com/TukuToi/tkt-maintenance/issues/17#issuecomment-864908055:

[...]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').[...]

Keeping this open to change the set and get options as it is indeed confusing and wrong to use like this. NFM: Also observe #17 when digging into this.

smileBeda commented 3 years ago

Removed the set_options method as useless, kept get_options as a public method.