[wp-trac] [WordPress Trac] #56849: Modify `_config_wp_home()` and `_config_wp_siteurl()` to become pre_option filters

[WordPress Trac]

#56849: Modify `_config_wp_home()` and `_config_wp_siteurl()` to become pre_option
filters
--------------------------------+---------------------
 Reporter:  flixos90            |       Owner:  (none)
     Type:  enhancement         |      Status:  new
 Priority:  normal              |   Milestone:  6.2
Component:  Options, Meta APIs  |     Version:  2.2
 Severity:  normal              |  Resolution:
 Keywords:  has-patch commit    |     Focuses:
--------------------------------+---------------------
Changes (by costdev):

 * keywords:  2nd-opinion has-patch => has-patch commit
 * version:   => 2.2


Comment:

 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 [https://github.com/WordPress/wordpress-develop/blob/2.1.0
 /wp-includes/functions.php#L203 2.1] (pre) and
 [https://github.com/WordPress/wordpress-develop/blob/2.2.0/wp-
 includes/functions.php#L206 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`.
 - [https://github.com/WordPress/wordpress-develop/pull/3664 PR 3664] tests
 well and xDebug shows the expected behaviour. Adding for `commit`
 consideration.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/56849#comment:4>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform