Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement: for options page - ability to autoload=false for options #1093

Closed
slaFFik opened this issue Feb 18, 2018 · 10 comments
Closed

Enhancement: for options page - ability to autoload=false for options #1093

slaFFik opened this issue Feb 18, 2018 · 10 comments

Comments

@slaFFik
Copy link
Member

slaFFik commented Feb 18, 2018

Expected Behavior:

When creating an options page using \new_cmb2_box() give ability to define the autoloader, so devs could define that those options should NOT be autoloaded.

As a developer, you know the drawbacks of autoload=yes.

Actual Behavior:

currently it defaults to yes

Steps to reproduce (I have confirmed I can reproduce this issue on the trunk branch):

Use the https://github.com/CMB2/CMB2-Snippet-Library/blob/master/options-and-settings-pages/options-pages-with-tabs-and-submenus.php snippet and save options.

@slaFFik
Copy link
Member Author

slaFFik commented Feb 18, 2018

In CMB2_Options::set() we have this:

// If no override, update the option
return update_option( $this->key, $this->options );

Either a filter there (the easiest path), that will allow to add a 3rd param to update_option() call, or a param that is passed to new_cmb2_box() call and should be passed down to the set() method.

@jtsternberg
Copy link
Member

Added a filter, and parameter on options-page boxes toggles the filter. Please test: https://github.com/CMB2/CMB2/compare/options-autoload-feature

@slaFFik
Copy link
Member Author

slaFFik commented Feb 22, 2018

@jtsternberg I've tested the PR.

Switching from yes to no via both filter and param work.
Swithcing back from no to yes doesn't work, it always stays the same no.

@jtsternberg
Copy link
Member

@slaFFik can you provide your code that you tested with?

@jtsternberg
Copy link
Member

Also, you are right. It will not switch the autoload value in the DB after the first time it is inserted. If you want to test switching, you will need to actually delete the option from the database first.

@slaFFik
Copy link
Member Author

slaFFik commented Feb 22, 2018

Default code from https://github.com/CMB2/CMB2-Snippet-Library/blob/master/options-and-settings-pages/options-pages-with-tabs-and-submenus.php snippet (100% the same) +

add_filter( 'cmb2_should_autoload_test_main_options', '__return_true' ); //_false

or

'autoload'     => true, // false

@slaFFik
Copy link
Member Author

slaFFik commented Feb 22, 2018

Using this

update_option( 'test_test', time(), true ); // false

actually udpates autoload no to yes. So you don't need to remove option and update it again to change this value.

@slaFFik
Copy link
Member Author

slaFFik commented Feb 22, 2018

Just remember, that update_option() doesn't fire $wpdb->update() to save data if old/new values are the same.

So the bug in the commit is that you default to null, which in case of already saved no uses null as default that later omits the $update_args['autoload'] (which is redefined only if not $autoload !== null) completely and don't pass the updated value to ->update() (that's why current code can't swtich from no to yes).

In short, you need to always pass the boolean no matter what, instead of a null.

@jtsternberg
Copy link
Member

Ok, good feedback, thank you. Updated. Please test again?

@slaFFik
Copy link
Member Author

slaFFik commented Feb 22, 2018

Looks good, tests for both filter and param for all scenarios passed fine when true/false is used.

There will be a problem with 'no' string, but I'm not sure something should be done here, at your discretion. Core is explicitly check 'no' and false, but we (as developers) want strict types :)

Thank you for this feature!

jtsternberg added a commit that referenced this issue Feb 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants