Removed dependency on options.php #78

Merged
merged 6 commits into from Jan 1, 2012

Projects

None yet

4 participants

@inxilpro
Contributor
inxilpro commented Nov 2, 2011

I've modified the framework to not depend on options.php, and more importantly, to not depend on global functions. I take an OOP approach to theme code, and those two mandatory global functions (optionsframework_option_name() and optionsframework_options()) were constantly bugging me. I also found that they made it difficult to manipulate the options of a parent theme in a child theme.

So I made two changes:

More flexibility in setting options

I've added two new ways to set options:

  1. Returning an array() in options.php
  2. Adding a filter on 'of_options'

The main advantage of using a filter is that this provides more granular control over options. For example, a child theme could add its own options in addition to the parent theme's options using this method. It also means that options can be set anywhere in your project. For example:

In parent theme:

add_filter('of_options', function($options) {
    $options[] = array(
        'name' => 'Input Text Mini',
        'desc' => 'A mini text input field.',
        'id' => 'example_text_mini',
        'std' => 'Default',
        'class' => 'mini',
        'type' => 'text'
    );

    return $options;
});

In child theme:

add_filter('of_options', function($options) {
    $options[] = array(
        'name' => 'Another Text Field',
        'desc' => 'This is a second text field, added in the child theme.',
        'id' => 'example_text2',
        'type' => 'text'
    );

    return $options;
});

And in the end your options will contain both fields.

Made optionsframework_option_name() optional

The option name can now be set using the Wordpress action optionsframework_option_name rather than requiring the optionsframework_option_name() function. If no name is set, the plugin defaults to optionsframework_global_options. Obviously most themes are going to want to override this behavior using the action, but it's there if not. optionsframework_option_name() still works, but it's now optional.

inxilpro added some commits Nov 1, 2011
@inxilpro inxilpro Added more flexibility in setting options
Now there are three ways to set options framework options:

  1. The old optionsframework_options() method
  2. Returning an array() in options.php
  3. Adding a filter on 'of_options'

The advantage of #3 is that this provides more granular control
over options.  For example, a child theme could add its own options
in addition to the parent theme's options using this method.  It
also means that options can be set anywhere in your project--the
options.php file isn't necessarily required (well, it still needs
to be there for optionsframework_option_name(), but I plan to
change that, too, soon).
edb3623
@inxilpro inxilpro Bug fixes
There were a few bugs with the new _optionsframework_options() method
that I've now fixed.  Everything should work well, now.
ed6232e
@inxilpro inxilpro Removed dependency on options.php
You can now use the Options Framework without the options.php file.
This flexibility is particularly useful in object oriented themes
where global functions are frowned upon.  Both the options and the
options name can be set via filters or actions.
bf13987
@inxilpro inxilpro Added a default option name
If no option name is set by the current theme, Options Framework chooses
the name "optionsframework_{theme_slug}"

Also fixed some minor code formatting inconsistencies (to better match
the Wordpress coding standards)
fb3d6c8
@inxilpro inxilpro Changed the default options name. e0e6eb8
@inxilpro inxilpro referenced this pull request Nov 2, 2011
Closed

filter options.php location #75

@inxilpro
Contributor
inxilpro commented Nov 2, 2011

It's important to note that all these changes are 100% backwards compatible.

@mattwiebe

This sounds like a great approach. The parent + child options is a big win.

@stefanoverna

That would be awesome!

@inxilpro
Contributor

Let me know if there's anything I can do to help get this merged into the project, Devin.

Chris

@devinsays devinsays merged commit e745bca into devinsays:master Jan 1, 2012
@devinsays
Owner

Thanks for all your work on this Chris. Initial testing looks good. I just merged the pull request and will do more testing this week. I'll also try to write a little blog post about it.

@devinsays
Owner

Instead of setting the default theme name to 'optionsframework_global_options', which would get overwritten if someone switched to a new theme that didn't have the optionsframework_option_name function:

// Set default name if none is set
    if ( !isset( $optionsframework_settings['id'] ) ) {
        $optionsframework_settings['id'] = 'optionsframework_global_options';
        update_option( 'optionsframework', $optionsframework_settings );
    }

How about making the theme name the default, like it is in the "Options Check":

    // Set default name if none is set
    if ( !isset( $optionsframework_settings['id'] ) ) {
            // This gets the theme name from the stylesheet (lowercase and without spaces)
            $themename = get_theme_data(STYLESHEETPATH . '/style.css');
            $themename = $themename['Name'];
            $themename = preg_replace("/\W/", "", strtolower($themename) );
        $optionsframework_settings['id'] = $themename;
        update_option( 'optionsframework', $optionsframework_settings );
    }

This would allow most people to rip out that function entirely from their options.php.

@devinsays
Owner

Can you explain a little more why you added the do_action( 'optionsframework_option_name' ) hook and how someone might use it?

@inxilpro
Contributor
inxilpro commented Jan 2, 2012

The optionsframework_option_name hook was meant to be a replacement for the optionsframework_option_name() function. You would use it exactly the same as the function, but it gives you more flexibility in what that function was called. For example, you might have a class that set the name like this:

add_action('optionsframework_option_name', array($this, 'setOfOptionsName'));

Then MyClass::setOfOptionsName() would set the id property of the optionsframework Wordpress option. Doing this in a filter might make more sense, but I thought this technique was more in line with the way things are done right now. Also, the id property is read at a number of different points, so to use a filter would require refactoring the code to do this in one place only (which would be a good idea, but more work).

As for your method of setting the default name—I totally agree. For some reason I thought that using optionsframework_global_options would be easier for everyone, but I personally use the stylesheet name. I would propose slightly different code, though:

if ( !isset( $optionsframework_settings['id'] ) ) {
    $id = 'optionsframework_' . preg_replace( '/\W/', '', strtolower( get_option( 'stylesheet' ) ) );
    $optionsframework_settings['id'] = $id;
    update_option('optionsframework', $settings);
}

I think it's important to namespace the option name, and also, using get_option('stylesheet') is going to be more performant than get_theme_data() (which hits the filesystem).

@inxilpro
Contributor
inxilpro commented Jan 2, 2012

Oh! Now I remember why I did it the way I did. Because the code above only runs if id is not set, it won't run when the user changes their theme (so it's effectively a global option). If we try to get around this by setting it each time the theme changes, then we've prevented people from using their own naming scheme.

@inxilpro
Contributor
inxilpro commented Jan 2, 2012

One way to get around this would be by checking the option prefix. If it uses the default prefix, then automatically change the id when the theme changes. If not, leave it alone:

$prefix = 'of_theme_options_';
$id = $optionsframework_settings['id'];
$new_id = $prefix . get_option( 'stylesheet' );

if ( !$id || ( 0 == strpos( $id, $prefix ) && $id !== $new_id ) ) {
    $optionsframework_settings['id'] = $new_id;
    update_option('optionsframework', $settings);
}

That way, someone could set the id to mycustomglobaloptions and the plugin wouldn't mess with it. But if the id is of_theme_options_twentyten and the user switches to twentyeleven, the options framework would automatically change the id to of_theme_options_twentyeleven.

Also, there's no need to bother with preg_replace and strtolower on the ID. It's just extra processing, and there's no technical reason the ID couldn't be op_theme_options_My Theme. Technically my-theme and mytheme could be two different themes, but would have unexpected behavior because they'd load the same options.

@devinsays
Owner

Let's see if this logic makes sense:

a) If the function optionsframework_option_name exists, $optionsframework_settings['id'] will be set

If that function is not being used:

b) Check elseif $optionsframework_settings['id'] exists and if it matches the default (prefixed?) themename id

If it matches were good to go. Else:

c) Set $optionsframework_settings['id'] to (prefixed?) themename id

And none of that should actually be run if an $options array isn't actually set from &_optionsframework_options.

I am still not entirely convinced that do_action( 'optionsframework_option_name' ) is necessary. It seems like someone could just as easily use optionsframework_option_name.

However, we could add another elseif between (a) and (b), that if has_action('optionsframework_option_name') then o_action( 'optionsframework_option_name' ).

@devinsays
Owner

I believe theme id does need to be lowercase with no spaces as it's also being saved directly as a database option. See (http://codex.wordpress.org/Function_Reference/get_settings#Parameters). However, I believe it currently just strips the spaces. That should probably be changed to make them underscores.

@inxilpro
Contributor
inxilpro commented Jan 2, 2012

I think that it's important to have that flexibility. Wordpress relies heavily on global functions, but it's generally not a good practice. My theme code is namespaced and separated into logical classes, and the optionsframework_option_name() function has no place in that setup. Being able to set the name in \MyShop\Themes\MyTheme::setOptionsFrameworkId() is an important part of keeping things in their proper place.


As for your logic, it looks right to me. If optionsframework_option_name() exists, it's called. Then the optionsframework_option_name action is called (this could go in an else statement, but there's no need to check has_action() first as do_action() just returns if no action is set). Finally, the code checks 3 things:

  1. Is an ID set?
  2. If so, does that ID start with "of_theme_options_?"
  3. If so, is the ID for the currently active theme?

If 1 is true, OR if 2 AND 3 are true, set the ID to "of_theme_options_THEME". Otherwise leave things be (an ID is set, and it's not the default, so leave it alone).


Finally, on the issue of the ID formatting—it's stored in two places:

  1. As a value of the id property in the optionsframework option. This is just a string stored in a serialized array, so it can be anything.
  2. As a value of the option_name column in the wp_options table. This is a VARCHAR column, so the only restriction is the size, which is limited to 64 characters.

So I can't see any reason why it'd need to be changed at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment