wp_flag api

[tellyworth]

There's a common pattern in wp code that looks like this:

turn_something_on();
if (something_is_on())
  do_something();
turn_something_off();

Currently there are two messy options for storing the toggle value when it needs to be shared between functions: either use a global, or write some get/set functions that store a static variable.

The enclosed patch provides a simple reusable api for this and similar patterns. Simple example:

wp_flag_set('myflag');
if ( wp_flag_get('myflag') )
  do_something();
wp_flag_unset('myflag');

It's not limited to booleans, you can store and fetch a value of any type:

wp_flag_set('myflag', 'myvalue');
if ( wp_flag_get('myflag') == 'myvalue' )
  do_something();
wp_flag_unset('myflag');

Flag values are not persistent and not shared across concurrent requests. They are stored in a static array so as not to pollute the global namespace.

Unit tests are in ​http://svn.automattic.com/wordpress-tests/wp-testcase/test_includes_functions.php, in the TestFlagFunctions class.

[santosj]

I'm not sure what the fascination is with using @$var['key'], may I suggest calling isset and returning it else returning null? If you don't want to create a branch.

return (isset($_flags[$name]) ? $_flags[$name] : null;

[tellyworth]

isset() uses more code to do the same thing. More code means more bugs. If there's a benefit to using isset, fair enough. If not I don't see the point in making it look more complicated than it is.

[rob1n]

More code? You mean that one line santosj had?

I hardly think more bugs could be introduced. That is a ridiculous argument, but I may not know the whole story.

[tellyworth]

One line of code but with more complexity. At the moment it's fine, but at some point someone will change it. The more complex it is the higher the chances a bug will be introduced.

You want to argue the toss over a single line? One alternative patch coming up. Take your pick, one expression vs a ternary operator, one function call and two expressions.

[darkdragon]

Dude. I'm sorry. I'm not saying your idea is bad, I actually do like it a lot. I just don't agree with code that uses '@' to suppress errors and warnings. Which all in fact you are doing is covering up a possible bug.

What does @$_flags[$name] return when it doesn't exist? I would really like to know. The most logical answer is 'NULL', but how can you be sure? It might return an empty string, or '0'. You also suppress the notice or warning that $_flags[$name] does not exist.

Sure, I get that. However for most PHP versions, using '@' takes a performance hit. It isn't until PHP 5.1+ that the performance hit is fixed. Now, the question you must answer is how many people are using the latest version that fixes that performance hit and how much of an performance hit will be dealt to those that don't have that version?

Don't take this personally, just explain these issues to me and clarify from what I think I know and educate me on what '@' is better than defensively programming.

If something may not exist, then I was always taught to test to make sure that it does exist, explicitly. Using isset() does not introduce bugs, it removes them by explicitly testing what might have been failure in other instances. Less code does not mean always mean less bugs, sometimes you need more code to make sure that bugs won't show up.

Thanks for the updated patch, but as you are creating an Registry Pattern, I think a class would provide a better solution instead.

[darkdragon]

class WP_Registry
{
    var $_data = array();

    function set($name, $value) {
        $this->_data[$name] = $value;
    }

    function get($name) {
        if( isset($this->_data[$name]) )
            return $this->_data[$name];

        return null;
    }
}

function &_wp_flag() {
    static $objRegistry;

    if($objRegistry == null)
        $objRegistry = new WP_Registry();

    return $objRegistry;
}

function wp_flag_set($name, $value=true) {
    $registry = _wp_flag();
    $registry->set($name, $value);
}

function wp_flag_get($name) {
    $registry = _wp_flag();
    return $registry->get($name);
}

function &wp_registry() {
    return _wp_flag();
}

[darkdragon]

For the unsetting, just add another method to WP_Registry class, and use the same setup as wp_flag_get() and wp_flag_set().

[matt]

I can only imagine one or two places where we could possibly use this.

[darkdragon]

Well, as an registry, it would generally get rid of the whole globals hell. So you take all of the globals and use this API instead. In that context, you would use this everywhere.

However, my theory, is that you can return a reference to the value contained in the array and it will update based on changes made later. If this were to be the case, then it would completely replace globals and not require a huge update of the core code.

[rob1n]

I'm with Matt. This seems a bit useless, and there's not really much of an issue with how WP currently does things.

[tellyworth]

Use cases include:

• WP_INSTALLING and WP_IMPORTING. These prevent some code from being unit tested because they can't be changed.

• $wp_smiliessearch and other low priority globals

• Patches like ​http://trac.wordpress.org/ticket/5377 and ​http://trac.wordpress.org/ticket/5232

[tellyworth]

If the consensus is that globals are an acceptable way to handle these types of problems, then fair enough - the patch is unnecessary.

[darkdragon]

I rather like this idea. Is it ever going to be acceptable in the core? If not now, then perhaps when WordPress 3 comes out?

[santosj]

The performance hit from implementing this would not be worth time and testing it would take to implement.