WordPress.org

Make WordPress Core

Opened 18 months ago

Last modified 6 weeks ago

#22251 new enhancement

Helper function to simplify checking for constants

Reported by: evansolomon Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Bootstrap/Load Keywords: has-patch
Focuses: Cc:

Description

Love 'em or hate 'em, WordPress uses lots of constants. As a result, this pattern is all over core and plugins, and occasionally themes:

if ( defined( 'CONSTANT_NAME' ) && CONSTANT_NAME )

Right now on trunk, it's used 57 times (excluding 2 in Akismet).

~/code/wptrunk $ ack "defined\( ?('|\")([^'\"]+)\1 ?\) \&\& \2" -h --ignore-dir=wp-content/plugins/ | wc -l
      57

How about a new function to make that verbose logic a little bit less repetitive.

function wp_constant( $constant ) {
	return defined( $constant ) && constant( $constant );
}

Attachments (1)

22251.patch (598 bytes) - added by evansolomon 18 months ago.

Download all attachments as: .zip

Change History (24)

evansolomon18 months ago

comment:1 helenyhou18 months ago

Saw it discussed in IRC in relation to unit testing. I'm sure it's come up other times, too.

comment:2 toscho18 months ago

  • Cc info@… added

comment:3 follow-up: nacin18 months ago

If we are to do something like wp_constant() it woud probably be so constants are pluggable (for testing purposes). This patch conflates not defining a constant with defining it as something that evaluates to false. In core, we make that assumption often, but in a generic function, we may not be able to, which means we might be in a situation where we need multiple functions just to be able to test everything.

We've reduces the use of the defined && constant pattern a few times before, notably with WP_DEBUG, which is automatically defined as false if it is not defined before the bootstrap begins to load.

I think this is clever but I would like to see what some PHP frameworks use to work with constants (for the ones they don't avoid). It'd be nice to see what we can do to reduce the number of constants we have. Those 57 instances have 30 unique constants among them.

comment:4 in reply to: ↑ 3 evansolomon18 months ago

Replying to nacin:

This patch conflates not defining a constant with defining it as something that evaluates to false. In core, we make that assumption often, but in a generic function, we may not be able to, which means we might be in a situation where we need multiple functions just to be able to test everything.

It definitely assumes that the distinction is not important. I agree multiple functions would probably make more sense than just one.

We've reduces the use of the defined && constant pattern a few times before, notably with WP_DEBUG, which is automatically defined as false if it is not defined before the bootstrap begins to load.

It's been increasing for a while. The WP_DEBUG change seems to have been in 2.9 (r12207), which also had the biggest jump in this pattern. Obviously in that time, lots of new code and features have been added too, so it may well have increased slower than it could have.

2.8 12
2.9 28
3.0 39
3.1 40
3.2 41
3.3 43
3.4 52

comment:5 follow-up: MikeSchinkel18 months ago

  • Cc mike@… added

Rather than go the direction of creating an access function I'd propose WordPress slowly deprecate all uses of PHP constants in the WordPress ecosystem and use another approach.

One potential other approach would be to deprecate constants in favor of UPPERCASED properties on the $wp object (uppercased properties should not conflict with current or I assume otherwise future potential "non-constant" properties.) This approach would change WP_DEBUG to be $wp->DEBUG.

Further for this approach we could:

  • Add a magic __get() method to class WP that would return false for any uppercased properties that have not been previously set and set the property as false to avoid future __get() method calls.
  • Add a magic __set() method to class WP that would define a PHP constant if not previously defined and if $wp->LEGACY_CONSTANTS == true.
  • A wp_const class defined in /wp-load.php to be used as a proxy for setting constants on $wp prior to the instantiation of WP and assignment to $wp that occurs in /wp-settings.php.

This approach would allow:

  • Access without having to test for existence.
  • Fast access to constants, i.e. it would not require the overhead of a function call in most cases.
  • Gradual migration, first for core and then for plugins and themes as developers decide to change.
  • Display of a "deprecated" message if $wp->WARN_CONSTANTS == true by testing get_defined_constants() in a 'shutdown' hook, or similar.

This approach makes sense because:

  • The $wp variable is the global object in WordPress and real constants are in the same scope.
  • It adds very little code and effectively no overhead compared to the current case.
  • It is consistent with existing practices as practically every entry-level plugin and theme developer understands the use of "global wp;" and/or "$GLOBALS['wp'].

I've implemented a proof-of-concept and it seems to work extremely nicely, better than I could have hoped for, actually.

If I get positive feedback on this idea I'll post the P.O.C. as a new ticket and a patch.

Last edited 18 months ago by MikeSchinkel (previous) (diff)

comment:6 MikeSchinkel18 months ago

Given the proposal above here's what part of a future wp-config.php might look like:

$wp->WP_DEBUG = true;

$wp->SAVEQUERIES = true;

$wp->WP_SITEURL = 'http://www.example.com';
$wp->WP_HOST = $wp->WP_SITEURL

// ** MySQL settings - You can get this info from your web host ** //
/** The name of the database for WordPress */
$wp->DB_NAME = 'site_db';

/** MySQL database username */
$wp->DB_USER = 'site_user';

/** MySQL database password */
$wp->DB_PASSWORD = 'abcde12345';

/** MySQL hostname */
$wp->DB_HOST = 'localhost';

/** Database Charset to use in creating database tables. */
$wp->DB_CHARSET = 'utf8';

/** The Database Collate type. Don't change this if in doubt. */
$wp->DB_COLLATE = '';

comment:7 follow-ups: wonderboymusic18 months ago

Sounds great, until that all gets obliterated by: http://core.trac.wordpress.org/browser/trunk/wp-settings.php#L244

comment:8 follow-up: rmccue18 months ago

Replying to MikeSchinkel:

One potential other approach would be to deprecate constants in favor of UPPERCASED properties on the $wp object (uppercased properties should not conflict with current or I assume otherwise future potential "non-constant" properties.) This approach would change WP_DEBUG to be $wp->DEBUG.

On the one hand, I think this might be be confusing, simply because it's a weird sort of syntax (to my eyes at least) to use $wp->ABC and that not being a proper constant. However, it is a cool idea, especially with the magic getter/setter. I also think it makes sense to have it under $wp.

Replying to nacin:

I think this is clever but I would like to see what some PHP frameworks use to work with constants (for the ones they don't avoid). It'd be nice to see what we can do to reduce the number of constants we have. Those 57 instances have 30 unique constants among them.

In most cases, they simply avoid them completely. For example, setting Symfony into debug mode is just another configuration option, not a constant. One way that this could be done in WP is to load the WP class before the configuration, allowing users to set e.g. $wp->debug = true in wp-config.php. This would be a fairly major change, I'd say.

That's essentially the same as Mike's changes above. However, I think the configuration file should stick to constants for most things. Allowing setting the properties in that manner would allow them to be set anywhere in the code without a complicated setter. Such a complicated setter would probably be a bad thing too, because I can't see how you could possibly code such a thing without causing huge problems.

comment:9 in reply to: ↑ 7 MikeSchinkel18 months ago

Replying to wonderboymusic:

Sounds great, until that all gets obliterated by: http://core.trac.wordpress.org/browser/trunk/wp-settings.php#L244

Why would they get obliterated? My proof of concept is working.

comment:10 in reply to: ↑ 8 MikeSchinkel18 months ago

Replying to rmccue:

I think the configuration file should stick to constants for most things. Allowing setting the properties in that manner would allow them to be set anywhere in the code without a complicated setter.

What is really wrong with allowing them to be set anywhere, in practice? After many years experience I'm still looking for a good reason that "constants" need to remain immutable.

Such a complicated setter would probably be a bad thing too, because I can't see how you could possibly code such a thing without causing huge problems.

Could you elaborate on the potential problems?

comment:11 in reply to: ↑ 7 ; follow-up: MikeSchinkel18 months ago

Replying to wonderboymusic:

Sounds great, until that all gets obliterated by: http://core.trac.wordpress.org/browser/trunk/wp-settings.php#L244

After re-reading your comment I think I see what you are saying. My P.O.C. does the following to deal with the issue:

/* 
 * Early in /wp-settings.php
 */
$wp_const = $wp;
unset( $wp );  // Some functions like register_taxonomy() will use $wp if it exists.
...
$wp = new WP();
/* 
 * This immediately follows the preceding line that assigns new WP() to $wp.
 */
foreach( get_object_vars( $wp_const ) as $property_name => $property_value ) {
  $wp->$property_name = $property_value;
}
unset( $wp_const );

comment:12 follow-up: rmccue18 months ago

Replying to MikeSchinkel:

What is really wrong with allowing them to be set anywhere, in practice? After many years experience I'm still looking for a good reason that "constants" need to remain immutable.

I completely agree, but I think if they're going to be uppercase, then they need to be only settable once so that they act like constants. That's why my personal preference would be to use lowercased properties just as normal properties.

Could you elaborate on the potential problems?

That was for if they could only be set in wp-config.php, where I can see problems with using includes in the config causing the complicated setter stuff to fail. I think that's a complicated path, but I personally would prefer eliminating the constants and constant-like behaviour (immutability) in favour of simply using properties (also renaming some such as $wp->db['host'] = 'localhost' etc.). This could be done completely in a backwards compatible manner, since we're inventing these from scratch, so as long as they check for the constants as well, it should be fine, e.g.

class WP {
    public $db = array( 'host' => '', /* ... */ );

    // ...

    public function set_from_constants() {
        defined('DB_HOST') && $this->db['host'] = DB_HOST;
    }

comment:13 in reply to: ↑ 11 ; follow-up: rmccue18 months ago

Replying to MikeSchinkel:

After re-reading your comment I think I see what you are saying. My P.O.C. does the following to deal with the issue:

/* 
 * Early in /wp-settings.php
 */
$wp_const = $wp;
unset( $wp );  // Some functions like register_taxonomy() will use $wp if it exists.
...
$wp = new WP();
/* 
 * This immediately follows the preceding line that assigns new WP() to $wp.
 */
foreach( get_object_vars( $wp_const ) as $property_name => $property_value ) {
  $wp->$property_name = $property_value;
}
unset( $wp_const );

I really like this, but how do we ensure $wp is defined before wp-config.php? If we don't, there'll be warnings (at least) from PHP. I guess adding $wp = (object) array(); to the top of wp-config.php by default is probably the easiest way.

comment:14 in reply to: ↑ 13 MikeSchinkel18 months ago

Replying to rmccue:

I really like this, but how do we ensure $wp is defined before wp-config.php? If we don't, there'll be warnings (at least) from PHP. I guess adding $wp = (object) array(); to the top of wp-config.php by default is probably the easiest way.

This is the rest of the P.O.C. code. I have it placed in /wp-load.php just before wp-config.php is called:

class wp_const {
  var $LEGACY_CONSTANTS = true;
  function __set( $property_name, $value ) {
    if ( $this->LEGACY_CONSTANTS && ! defined( $property_name ) ) {
      define( $property_name, $value );
    }
    $this->$property_name = $value;
  }
  function __get( $property_name ) {
    if ( preg_match( '#^[A-Z_0-9]+$#', $property_name ) ) {
      $this->$property_name = false;
    }
    return $this->$property_name;
  }
}
$wp = new wp_const();

Actually, I also have added the var $LEGACY_CONSTANTS and two magic functions to the WP class. Too bad Traits are not supported in PHP 5.2...

Last edited 18 months ago by MikeSchinkel (previous) (diff)

comment:15 in reply to: ↑ 12 MikeSchinkel18 months ago

Replying to rmccue:

I completely agree, but I think if they're going to be uppercase, then they need to be only settable once so that they act like constants. That's why my personal preference would be to use lowercased properties just as normal properties.

This approach could not support that and also offer the fast access to the properties. I guess it could be extended to have "read-only constants" by storing those in an internal array and thus always using the magic methods for access but the tradeoffs would obviously be slower access. That might be okay for constants not used often.

But even so, I've only seen immutable constants causing trouble vs. helping (such as for testing); in contrast I haven't seen any real issues with get_option() values being mutable.

That was for if they could only be set in wp-config.php, where I can see problems with using includes in the config causing the complicated setter stuff to fail.

I think this approach bypasses those issues. Maybe, maybe not?

I personally would prefer eliminating the constants and constant-like behaviour (immutability) in favour of simply using properties (also renaming some such as $wp->db['host'] = 'localhost' etc.).

Agreed. Obviously. :)

comment:16 alex-ye18 months ago

  • Cc nashwan.doaqan@… added

comment:17 follow-up: alex-ye18 months ago

It might be a stupid question , but You worried a lot of the constants count ?! Is it really bad ?! and You just suggestion to use class vars instead of ?! Yes it's a good to clean the code but sometimes we the constants is a good practice ...

comment:18 in reply to: ↑ 17 MikeSchinkel18 months ago

Replying to alex-ye:

It might be a stupid question , but You worried a lot of the constants count ?! Is it really bad ?! and You just suggestion to use class vars instead of ?!

This is not about comment count, it is about making access easier i.e. so that you don't first have to test for if (defined('...')), and immutable constants cause difficulty in scenarios such as unit testing and/or with some plugin use-cases.

comment:19 in reply to: ↑ 5 scribu18 months ago

  • Cc scribu added

comment:20 nacin3 months ago

  • Milestone changed from Awaiting Review to Future Release

In theory, I like this. Making them immutable is an interesting idea.

Has anyone ever taken a look at old bbPress? It has a $bb object where you can set option names/values. I wonder if we can learn from their implementation (including what not to do).

I am all for repurposing the WP class for more things when it makes sense.

comment:21 nacin3 months ago

  • Component changed from General to Bootstrap/Load

comment:22 deepwater wells3 months ago

Are there reasons not to implement a configuration object for this explicit purpose? Doing so would keep WP as a single-responsibility object (basically a router) while also allowing for the use of magic methods, defaults/fallbacks, etc. on config items. It would also allow the config object to be instantiated independent of (before) WP.

The config object (WP_Config?) could be accessible via a WP property (once instantiated), like $wp->config->some_setting, and/or through functions like wp_config_get('some_setting'). I think this would be more future-proof and intuitive than accessing WP directly. Thoughts?

comment:23 ircbot6 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.

Note: See TracTickets for help on using tickets.