Question: Why use update_option instead of pre_option_{$option} filter about intervention HOT 9 CLOSED

I like the pre_option way of doing it b/c you can see it easier in the codebase. People understand how it relates to WordPress by using a core function. By doing an array compare, you are introducing custom logic that might be foreign to outsiders. You are also introducing a storage place for the temp version. Where does that get stored? Can it be lost or corrupted? How can you delete it when needed?

from intervention.

Yes, I have the same question.

In principle, the pre_option_{$option} seems better, which bypass all the logic of the get_option function and returns the desired value. The downside is that you lose all settings when exporting or if you need to disable Intervention.

The update_option store your config on DB and solves previous downside. It should really only update on the first page load because update_option compares the values and if they are equal, they are not updated and there is no database hit. But there is a problem with numeric values: they are strictly compared with DB (string) value and always return false so forces an DB update on every page load.

I have actually added a filter that prevents this problem on this branch (I'm not sure if it's the best file to add this code) and stops with DB updates on every load.

I am open to suggestions to modify it or to make a pull request 🙂

from intervention.

I'm all with @claytoncollie here. pre_option seems the better option IMO. I think it's fair to assume that using the plugin is short cutting options handling and obviously deactivating it will result in falling back to default, unchanged, database options. Describing clearly that behavior in the readme should be enough.

Maybe the best option would be to introduce a way to manually set options to DB if needed?

from intervention.

Hey @claytoncollie,

I'm open to the suggestion of changing to pre_option, initially I wanted to update the DB value, but perhaps that's not necessary. What's your thoughts? I'm trying to preempt any downsides to using pre_option for users.

from intervention.

I have not used this package yet so this is only thinking out loud. If you enabled all options in your config array then went to random pages in the wp-admin with Query Monitor turned on, you could see how many queries are being generated on each page load. My thought is that you are performing 2 database actions for each page load for each option. First to make the update_option call, then the standard get_option call afterward. With pre_option_ it might be able to get rid of both. The only problem is that your database is not a source of truth anymore when you export for some reason. Pushing and pulling between environments would be fine if the codebase is version controlled, but your DB could potentially be "out of date" to what your config is.

My original question was to see if any thought was given to it and if using update_option was a conscious decision. Sounds like it was when you first started the project. I don't know the project well enough if you can give the option to filter the option or update the database.

from intervention.

Hey @claytoncollie,

Let me give it some more thought and do a little prototyping, perhaps an approach that uses both, only updating the DB at certain intervals, or on the dashboard/settings pages, or a version of that, and pre_option otherwise.

Edit: Thinking about it more, I don't think it's a deal breaker if the DB actually doesn't update, the plugin should always be enabled to pass across those options in my opinion. I'll keep this issue open so that others can also weigh in.

from intervention.

Hey @pacotole,

This looks great, thank you! I think having it in that file is perfect.

I'll do some prototyping with update_option and pre_option.

Another line of thought could be comparing the application config with a temp version, and if there are changes to the application config, updating the DB and the temp version. Then we wouldn't even really need pre_option, while also removing unnecessary requests/guesswork. Thoughts?

Edit: PR request for the above would be great, I'll prob change array_map to an Laravel collection to match the rest of the codebase, but I can change that up!

from intervention.

Yea, fair points that I agree with, thanks for thinking that through. Alright, so let’s go with pre_option and an occasional DB request! I’ll take a look at getting it changed soon.

from intervention.

This has been addressed in d5ee64 with a new internal Options API, which now uses pre_option.

from intervention.