WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#5384 closed enhancement (wontfix)

wp_flag api

Reported by: tellyworth Owned by:
Milestone: Priority: low
Severity: normal Version:
Component: General Keywords:
Focuses: Cc:

Description

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.

Attachments (2)

wp-flag-api-r6342.patch (1.4 KB) - added by tellyworth 6 years ago.
wp-flag-api-r6342-isset.patch (1.4 KB) - added by tellyworth 6 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 santosj6 years ago

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;

comment:2 tellyworth6 years ago

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.

comment:3 rob1n6 years ago

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.

comment:4 tellyworth6 years ago

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.

comment:5 darkdragon6 years ago

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.

comment:6 darkdragon6 years ago

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();
}

comment:7 darkdragon6 years ago

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

comment:8 matt6 years ago

  • Priority changed from normal to low

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

comment:9 darkdragon6 years ago

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.

comment:10 rob1n6 years ago

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

comment:11 tellyworth6 years ago

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

comment:12 tellyworth6 years ago

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

comment:13 darkdragon6 years ago

  • Milestone changed from 2.6 to 2.7

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?

comment:14 santosj6 years ago

  • Milestone 2.7 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

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

Note: See TracTickets for help on using tickets.