Modify `_config_wp_home()` and `_config_wp_siteurl()` to become pre_option filters

[flixos90]

The functions _config_wp_home() and _config_wp_siteurl() are currently used to enforce values for the "home" and "siteurl" options, if the respective WP_HOME / WP_SITEURL constant is set.

However, the way this currently happens is by hooking into the option_home and option_siteurl filters. This is a bit wasteful since it means all the logic to still look up the option is still run unnecessarily.

We should change the approach so that the two callbacks become filter callbacks for pre_option_home and pre_option_siteurl instead. This way, if the constants are defined, the logic in get_option() can bail early.

Trac ticket: https://core.trac.wordpress.org/ticket/56849

[pbearne]

Adding unit-tests in own tickets

[pbearne]

Tests for the _config_wp_siteurl and _config_wp_home
that include testing the prefilters
#57179
#57180

[costdev]

Hi @flixos90 and @pbearne, thanks for the ticket and patch!

The logic for this change seems sound to me. The original intent of hooking option_home and option_siteurl was:

Allow siteurl and home to be defined as constants in wp-config, bypassing the DB.
[5093] (WordPress 2.2)

Looking at the ​2.1 (pre) and ​2.2 (post) versions of get_option(), these filters may bypass DB calls in other parts of Core, but not in get_option() itself. This ticket could be seen as improving on the original intent of these filters.

PHPUnit tests

PHPUnit tests for this are difficult because they require the WP_HOME and WP_SITEURL constants to be defined. As all tests run in the same process, this may cause other tests to break.

runInSeparateProcess is not available to us in the WP Core Test Suite (errors out in bootstrap), and setting up separate groups for these would start a trend that would result in many, many groups for tests relying on constants being defined. Given that, manual testing should be enough for this ticket IMHO.

Manual testing

I've tested the PR by ensuring that pages on the frontend and backend continue to load.

Manual tests:

• With neither constant defined. ✅
• With only WP_HOME defined. ✅
• With only WP_SITEURL defined. ✅
• With both constants defined. ✅
• With WP_HOME and a trailing slash. ✅
• With WP_SITEURL and a trailing slash. ✅
• Both with trailing slashes. ✅

I also stepped through in xDebug:

WP_HOME

• When the WP_HOME constant isn't defined, _wp_config_home() receives false and passes it directly back to get_option(), which proceeds to cache/DB calls to get the value. This is the expected behaviour. ✅
• When the WP_HOME constant is defined, _wp_config_home() receives false, runs untrailingslashit() on the value of WP_HOME, and returns it to get_option(). This is stored in $pre, and is returned immediately after the pre_option filters have run. This is the expected behaviour. ✅

WP_SITEURL

• When the WP_SITEURL constant isn't defined, _wp_config_siteurl() receives false and passes it directly back to get_option(), which proceeds to cache/DB calls to get the value. This is the expected behaviour. ✅
• When the WP_SITEURL constant is defined, _wp_config_siteurl() receives false, runs untrailingslashit() on the value of WP_SITEURL, and returns it to get_option(). This is stored in $pre, and is returned immediately after the pre_option filters have run. This is the expected behaviour. ✅

Some gardening work

• Both of these were originally hooked to option_home and option_siteurl respectively in [5093]. Setting Version to 2.2.
• This change seems sound to me. Removing 2nd-opinion.
• ​PR 3664 tests well and xDebug shows the expected behaviour. Adding for commit consideration.

[peterwilsoncc]

Both site_url and home_url options are autoloaded, if the constants are defined during the install are the options created or left out of the options table?

The thought behind the question: there are two potential for back-compat breaks:

• sites removing the filters using remove_filter( 'option_... in the manner of ms-default-filters.php
• an order of operations issue for sites already filtering pre_option_... in a plugin -- there's a risk the existing code's return value will be replaced with the value of _config_wp_...().

On enterprise sites, the latter is more likely than the former as the options are often hardcoded in an mu-plugin to allow for them to be transferred between different environments more easily.

[costdev]

@peterwilsoncc Both site_url and home_url options are autoloaded, if the constants are defined during the install are the options created or left out of the options table?

I applied the changes in the PR, defined both constants and ran a manual install (via browser). Both values are added to the options table.

Regarding the order of operations, from what I can see:

• default-filters.php is included ​here (Line 134).
• ms-default-filters.php is included ​here (Line 359).
• MU plugins are included ​here (Line 372).
• Network plugins are included ​here (Line 392).
• Plugins are included ​here (Line 447).
• Theme setup begins ​here (Line 549).

I created a plugin that hooks pre_option_home and pre_option_siteurl, and once the plugin had been loaded and the hooks added, they ran after the _wp_config_home|siteurl() hooks.

---

However, and I could use a confidence check on this one, if WP_HOME or WP_SITEURL are defined, get_option() would now return after pre_option, so hooks on option_{$home} will never run (think translations with home/lang_code). That's a BC break unless we apply "option_${option}" filters for home and siteurl, right?

Which would mean something like this I think:

// pre_option_{$option}
// pre_option

if ( false !== $pre ) {
    if ( 'home' === $option || 'siteurl' === $option ) {
        $pre = apply_filters( "option_{$option}", $value, $option );
    }

    return $pre;
}

---

Removing commit while discussion continues.

[flixos90]

#57179 was marked as a duplicate.

[flixos90]

#57180 was marked as a duplicate.

[flixos90]

@costdev @peterwilsoncc Those are great points. Sharing my thoughts on them:

• I like the idea of running the "option_{$option}" filters too in those cases, this should help with BC. This should address the main concerns here.
• Checking the usage of _config_wp_home and _config_wp_siteurl across plugins, it is extremely rare, and almost none that adjusts the filters. The only relevant usage is in a plugin called "Uploads by Proxy". See ​https://wpdirectory.net/search/01GQFRDPZP9WFQ7YY5KBEH6WS4 and ​https://wpdirectory.net/search/01GQFRM6GBN1YSCK80BCCXJMFZ.
• What we could potentially do to work around even more edge cases is to automatically migrate the removal and addition of the _config_wp_*() to the "option_{$option}" filter if that happens in third party code. We can add a condition to remove_filter() and add_filter() that handles this, maybe even with a PHP notice (or deprecation warning?), that those callbacks should be attached to "pre_option_{$option}" instead.
• We can also write a brief dev note on this change as a reference for developers that may be relying on this logic in some special ways.

WDYT?

[costdev]

Also, here's some usage of the option_home and option_siteurl filters:

• ​option_home
• ​option_siteurl

Provided we run the options_{$option} filters, these should be fine.

(I'll read and respond to the rest after work)

[costdev]

Apologies for the delay in getting to respond to all of the items Felix.

I think migrating may cause an issue for extenders using remove_all_filters() on option_home or option_siteurl, as these will now exist on a different hook and we can't reliably remove those.

Conditionally calling apply_filters( "option_{$option}" ) for home and siteurl should still work fine for remove_all_filters().

We could still handle remove_filter() though, by checking if the callback also exists in the pre_option_home / pre_option_siteurl filters and removing it there too. In terms of performance, it looks like remove_filter() and WP_Hook::remove_filter() do isset() lookups, so performance impact should be minimal.

Something like this at ​around L323-L324 in remove_filter(), for example:

if ( ! $wp_filter[ $hook_name ]->callbacks ) {
        unset( $wp_filter[ $hook_name ] );
}

+ if ( $r && ( 'option_home' === $hook_name || 'option_siteurl' === $hook_name ) ) {
+       remove_filter( 'pre_' . $hook_name, $callback, $priority );
+ }

What do you think?

[peterwilsoncc]

I wrote up a mini plugin to ​test the advantages of this change.

Filter	Time
option_siteurl	4.357578754425 * 10-5 seconds
pre_option_siteurl	1.505925655365 * 10-5 seconds

As a multiple it seems impressive, but for a single run it's 28 microseconds. This is using vagrant on virtual box which is typically a slow system.

Granted, the option is retrieved numerous times during the typical request, but even at a generous 100 runs, the time saved is around 3 milliseconds.

If the change requires adding snowflake code to get_option or remove_filter as a special case, I don't see the value in doing so. With low usage of the constants, there's greater value to be gained improving performance elsewhere.

As the most popular plugins linked to above are using the later filters to ensure the sites are running over https, rather than http, I think there's some risk in simply moving the hook to run earlier.

[flixos90]

Fair point @peterwilsoncc. I think given the concerns there is not enough justification to make this change. Certainly would need more time than the 6.2 Beta to iron out, but I'm not sure it's worth the investment. Let's just close it as maybelater for now.