WP_INSTALLING causes leakage between unit tests

[boonebgorges]

A number of multisite unit tests invoke WP_UnitTest_Factory_for_Blog::create(), which uses wpmu_create_blog() to create a blog. Whenever this happens, the WP_INSTALLING constant is defined as true for all later tests. This has a number of ramifications, especially in the Options API, which skips the cache when WP_INSTALLING. See [28965], [31280], #28706.

I see four strategies for working around the problem:

1. For any test that breaks when WP_INSTALLING, mark it skipped on multisite. This is what we're currently doing, and is lame.
2. Create wrapper functions for WP_INSTALLING so that it can be toggled on and off (unlike a constant). Either apply_filters(), or a global toggle like wp_suspend_cache_invalidation(). Then, in WP_UnitTest_Factory_For_Blog::create(), toggle it off after wpmu_create_blog() has turned it on. The main downside here is that WP_INSTALLING is not really the kind of thing we want to be toggleable by devs, so we'd have to document it accordingly.
3. Run all blog-creating tests in separate processes that don't preserve the global state. That way, the constant would be set in the other process, but not when returning to run the rest of the tests. I played with this a little, and it looks like it's not going to be trivial to make it work - we may need to bootstrap WordPress for every test or something like that.
4. Make sure that all tests that create a blog are run at the very end of the suite.

Option (2) seems like the most natural fix, but it's also the only one that touches src/. Any additional thoughts are welcome.

[jeremyfelt]

I think some flavor of option 2 is the sane way forward. I did some exploration of this with DOMAIN_CURRENT_SITE and PATH_CURRENT_SITE during the 4.0 cycle in 27884.diff, but backed off quite a bit. There are a handful of constants that would be nice to flip on and off or back and forth without having to fire up an entirely new process. WP_INSTALLING is a pretty brutal one, so we'll need to take care.

Option 3 would be the next best bet if it worked. Trying to get the multisite tests to actually run in a separate process was not an enjoyable time as the separate processes would use the default config and the multisite tables would not be available. I'm sure there is a way to do this, I just couldn't find it.

Option 4 would work, but would always be a source of possible error when committing new tests.

Related #27884, #30256

[jeremyfelt]

There is some good conversation in #25669 on this as well.

[jeremyfelt]

And more good conversation in #22251, with an ​IRC discussion to match.

[jeremyfelt]

31130.diff is an initial attempt at solving this.

• Introduce $wp_config in wp-settings.php to hold any configuration data normally stored in constants.
• Introduce wp_define() in default-constants.php for core to use as a way to define constants or to set the property in $wp_config if tests are running.
• Introduce wp_defined() in default-constants.php for core to use for retrieval of constants or $wp_config data if tests are running.
• Change all instances of define() and defined() used for WP_INSTALLING to wp_define() and wp_defined(). Some are not changed due to load order in places such as wp-activate.php or install.php, but may be possible with further exploration. These are not requirements for unit testing right now.
• Reset the global $wp_config when tearing down tests.
• Un-skip multisite tests that were previously skipped for WP_INSTALLING reasons. These appear to all pass.
• Use wp_schedule_update_network_counts() when testing the same function, something we could not do before with WP_INSTALLING defined.

Caveats...

I did get one random failed test out of several attempts for test_orderby_meta(), but it never appeared again.

1) Tests_Comment_Query::test_orderby_meta
Failed asserting that '322' matches expected 320.

/srv/www/wordpress-develop/tests/phpunit/tests/comment/query.php:616

And the test of current_user_can_for_blog() on a non-existing site now generates a missing table error since the test actually runs now that WP_INSTALLING is not available.

Overall, the results are good.

• Before patching, all tests passed with 14 skipped.
• After patching, but without resetting the $wp_config global to clear WP_INSTALLING, 5 tests failed and 9 skipped.
• After resetting the $wp_config global, all tests passed with 9 skipped.

This patch/discussion could also belong on #22251. Outside of a specific solution for WP_INSTALLING, there are other things to consider—is this a good approach, does this belong in $wp rather than $wp_config, new function naming, etc...

[dd32]

This patch/discussion could also belong on #22251. Outside of a specific solution for WP_INSTALLING, there are other things to consider—is this a good approach, does this belong in $wp rather than $wp_config, new function naming, etc...

I think we should really invest some time on #22251 and come up with a proper generic approach that isn't unit-test specific, 31130.diff​ certainly proves it's usefulness though.

[boonebgorges]

In 31628:

Introduce $autoload parameter to update_option().

When creating an option via add_option(), the $autoload param allows you to
tell WP whether the option should be loaded as part of the 'alloptions' cache
during every pageload. update_option(), when used with a non-existent option
calls add_option() internally. The new $autoload param in update_option()
is passed along to add_option() in cases where the option does not yet exist.

The associated unit tests are skipped on multisite due to an issue that causes
WP_INSTALLING to force cache misses. See #31130.

Props codix, nofearinc, MikeHansenMe.
Fixes #26394.

[boonebgorges]

In 31764:

Don't run wp_get_archives() cache test on multisite.

The introduction of a Customizer test that creates a new blog [31707] causes
WP_INSTALLING to be set by the time the wp_get_archives() tests run. This,
in turn, causes the query counts to vary in unpredictable ways.

See #31130.

[johnbillion]

Latest breakage: #30380

[johnbillion]

In 33125:

Skip the Tests_Option_Transient::test_nonexistent_key_old_timeout() tests on multisite for now.

See #31130
Fixes #30380

[boonebgorges]

In 34540:

Don't run get_page_of_comment() cache test on Multisite.

get_page_of_comment() uses get_option(), and WP_INSTALLING earlier in the
test suite causes get_option() to miss the cache. See #31130.

See #11334.

[boonebgorges]

In 34719:

Prevent Multisite term tests from hitting database for 'db_version'.

[34718] introduced a 'db_version' check to term meta functions, to ensure that
they don't run when the term meta schema is not yet in place. This call to
get_option() causes a database hit during Multisite tests, due to the
presence of the WP_INSTALLING constant. See #31130. The extra database
queries are causing cache tests to fail.

In similar cases, we have markTestSkipped() when is_multisite(). Because
the term meta API is so extensive - term meta caches can be primed anywhere a
WP_Query loop is fired up - we implement a more generous workaround in this
case. To prevent get_option( 'db_version' ) from hitting the database during
multisite unit tests, we use a 'pre_option_' filter.

Heaven help us.

See #34091.

[boonebgorges]

In 34720:

Fix db_version juggling during non-multisite tests.

Continuing with the "code is poetry" theme after [34719], we need to continue
to update the option in the database on non-multisite in this
wp_insert_term() test.

See #31130.

[boonebgorges]

This is driving me bonkers.

Thinking about jeremyfelt's suggestions: We who are familiar with the codebase think of the various constants as being related to each other, by virtue of being constants. But to a newcomer, they have nothing to do with each other. So wrapping them in a wp_defined() function doesn't make a lot of sense.

For the purposes of this ticket, I'd like to suggest a more focused wp_installing() function. See 31130.2.diff​. It works just like force_ssl_admin() and its ilk: store the current setting in a static variable; wp_installing() with no args will return the setting; wp_installing( $bool ) will set the value to $bool and return the previous value. The one fancy bit is that we must continue to support the WP_INSTALLING constant, since WP defines it in various places before the bootstrap (and thus before wp_installing() is loaded). So, wp_installing() will default to true if defined( 'WP_INSTALLING' ) && WP_INSTALLING.

Then, in WP_UnitTest_Factory_For_Blog, call wp_installing( false ) just after creating the blog. This solves the problem raised in the current ticket.

Again, I'm not opposed to a more general approach to the use of constants, but WP_INSTALLING is more of an internal tool which is a constant by technical necessity, than it is a "config" setting. A standalone solution seems prudent.

[jeremyfelt]

Replying to boonebgorges:

Again, I'm not opposed to a more general approach to the use of constants, but WP_INSTALLING is more of an internal tool which is a constant by technical necessity, than it is a "config" setting. A standalone solution seems prudent.

Agree completely.

31130.3.diff is a refresh of 31330.2.diff. Some recent churn confused things a bit. I also had to add DB error suppression in test_current_user_can_for_blog() for the one check against a non-existent site. I'm not entirely sure why this was causing DB error output after patch and not before, but that's one of the reasons we're in this ticket... :)

With this patch:

• All standard, multisite, and ajax tests pass locally.
• I was able to install WordPress from scratch.
• I was able to create a network.
• I was able to add a new site to the network.

I like it.

[boonebgorges]

In 34828:

Use wp_installing() instead of WP_INSTALLING constant.

The WP_INSTALLING constant is a flag that WordPress sets in a number of
places, telling the system that options should be fetched directly from the
database instead of from the cache, that WP should not ping wordpress.org for
updates, that the normal "not installed" checks should be bypassed, and so on.

A constant is generally necessary for this purpose, because the flag is
typically set before the WP bootstrap, meaning that WP functions are not yet
available. However, it is possible - notably, during wpmu_create_blog() -
for the "installing" flag to be set after WP has already loaded. In these
cases, WP_INSTALLING would be set for the remainder of the process, since
there's no way to change a constant once it's defined. This, in turn, polluted
later function calls that ought to have been outside the scope of site
creation, particularly the non-caching of option data. The problem was
particularly evident in the case of the automated tests, where WP_INSTALLING
was set the first time a site was created, and remained set for the rest of the
suite.

The new wp_installing() function allows developers to fetch the current
installation status (when called without any arguments) or to set the
installation status (when called with a boolean true or false). Use of
the WP_INSTALLING constant is still supported; wp_installing() will default
to true if the constant is defined during the bootstrap.

Props boonebgorges, jeremyfelt.
See #31130.

[boonebgorges]

Thanks, jeremyfelt. Let's see what breaks after [34828].

I'm thinking that a make/core post is probably appropriate for this change. I just spent some time looking through the plugin repo to see what people were doing with WP_INSTALLING. Most of them fall into one of a few camps: (a) those that check the constant to make sure we're not literally installing WordPress, (b) those that define the constant because they're doing things like cloning a site, (c) those that define the constant because they're bootstrapping WordPress again (?), and (d) caching plugins that point wp_cache_get() to delete() rather than get() when WP_INSTALLING. From what I can see, those in camp (a) will be unaffected in any meaningful way, and those who are doing (c) have already voided their warranty. (b) and (d) are a little less clear to me. For the vast majority of cases (ie, during a normal installation/upgrade), they won't be affected, since WP_INSTALLING is defined as part of the bootstrap. The question is whether not having the constant available during site creation will do anything funny with cache implementations. I don't *think* it will, since the cache for a newly created site will be empty anyway, but there could turn out to be cache invalidation problems for global cache groups. Anyone have thoughts about this?

[adamsilverstein]

I'm seeing a fatal in load.php after [34828] if I have a .maintenance file in my wp root. This is because functions.php hasn't loaded yet when wp_maintenance is called, and PHP dies when wp_installing is undefined.

Not certain what to do. Patch incoming that reverts this line change so no more fatals, yea! maybe better would be to define the function where wp_maintenance would have access to it, perhaps in load.php itself?

[adamsilverstein]

revert change to load.php in wp_maintenance

[adamsilverstein]

In 31130.4.diff​:

• Use Global instead of wp_installing function in load.php as functions.php hasn't loaded yet and wp_installing is undefined

[adamsilverstein]

I think I like this better as it doesn't undo this ticket :)

In 31130.5.diff​:

• Moves new wp_installing function to load.php, ensuring its available for use there

Fixes a PHP fatal when .maintenance file present.

[boonebgorges]

Moves new wp_installing function to load.php, ensuring its available for use there

Ding ding ding! We have a winner. Seems appropriate that it'd be in the same place as wp_maintenance() anyway.

[boonebgorges]

In 34896:

Move wp_installing() to load.php.

Various functions in load.php need to check whether WP is in installation mode.
Let's let them.

Props adamsilverstein.
See #31130.

[adamsilverstein]

Replying to boonebgorges:

Ding ding ding! We have a winner. Seems appropriate that it'd be in the same place as wp_maintenance() anyway.

:)

[boonebgorges]

The sky hasn't fallen, so let's close.