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.

Sign up for GitHub

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

Jump to bottom

vp_option should use defaults specified in template #94

Open
liviucmg opened this issue Oct 12, 2014 · 4 comments · May be fixed by #95
Open

vp_option should use defaults specified in template #94

liviucmg opened this issue Oct 12, 2014 · 4 comments · May be fixed by #95

Comments

@liviucmg
Copy link

We have the following scenarios:

  1. The user has just activated the theme but has not yet opened and saved the settings page. No options are set yet. Similar to Load default options on theme activation #72.
  2. The user has just updated the theme, and the new version brings some new options. These new options are unset.

I believe that the vp_option should return the default values for these options, since they are already specified in the template file. Instead, it returns null, unless you manually specify a default value each time you call it.

For the sake of backward compatibility, I have created a new function vp_option_with_default (suggestions for a better name are welcomed) that fixes this. It will call vp_option and if that returns null, it will load the default values and extract the option from there.

To boost performance, I have also modified VP_Option_Control_Set to cache these default values so that they are not computed each and every time.

I have made a quick speed test on an unset option, with 1000 runs. Results:
vp_option('some.option', null); = 6ms
vp_option_with_default('some.option'); = 17ms
vp_option_with_default('some.option');, without caching the defaults = 427ms

It is pretty much unnoticeable with the cache fix.

Thoughts?
@liviucmg liviucmg linked a pull request Oct 12, 2014 that will close this issue
Open
@vladan-me
Copy link

I think that is actually expected output of vp_option() (including defaults) so I don't see a need for backward compatibility. Can you merge it in the original function?

@scrobbleme
Copy link

@liviucmg Thanks for the addition, but I see it as @vladan-me

Calling vp_option directly should return the default value if set. Optionally it could have a second parameter "default", which could be used to override the internal default.

So, what vp_option($key, $default = null) could do

  1. If value is set -> return value
  2. else if $default is set -> return $default
  3. else return default from configuration

@liviucmg
Copy link
Author

Okay guys, sounds good. I was just afraid that someone would have previously depended on vp_option to return null when no value is set, but that isn't the function's intended purpose. I will return with a PR later today. Cheers!

@liviucmg
Copy link
Author

Done. I've also added a proper PHPDoc comment, and fixed the indenting to tabs just like the rest of the file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

vp_option falls back to the default value as specified in the template
3 participants
@scrobbleme @liviucmg @vladan-me