Don't upgrade global tables for utf8mb4

[ocean90]

See #21212, [31349], [31358].

[johnjamesjacoby]

IMO, we only want to run this once on any installation. I would suggest:

• is_main_site()
• is_main_network()

Which means:

if ( is_main_site() && is_main_network() && ! defined( 'DO_NOT_UPGRADE_GLOBAL_TABLES' ) ) {

Nacin mentioned concerns about pre-schema issues with is_main_site(). I'd noticed that pre_schema_upgrade() uses is_main_network() already for sign-up tables, but I'm uncertain if that influences things.

[johnjamesjacoby]

Introduce wp_can_upgrade_global_tables()

[johnjamesjacoby]

32154.02.patch does the following:

• Introduces wp_can_upgrade_global_tables() (which could be moved out of upgrade.phplater, maybe into functions.php or something available to plugins that may also have global tables; like BuddyPress)
• Bails early if DO_NOT_UPGRADE_GLOBAL_TABLES is set, respecting the original linchpin intention
• Uses is_main_site() to ensure routine is happening from the main site of the current network
• Uses is_main_network() to ensure routine is happening from the main network of the current installation

This should cover all known and supported installation types:

• Single-site
• Multi-site
• Multi-network
• Multi-installation with mixed connections and global users tables

[johnjamesjacoby]

See also: #14221

[ocean90]

@pento What are your thoughts on this? We shouldn't break dotorg again. ;-)

We had also some discussion about ! ( defined( 'DO_NOT_UPGRADE_GLOBAL_TABLES' ) && DO_NOT_UPGRADE_GLOBAL_TABLES ) and decided that ! defined( 'DO_NOT_UPGRADE_GLOBAL_TABLES' ) is enough. Should be changed in [32380] too.

[pento]

I'm cool with adding a helper function for this, especially with the new multi-network things coming in.

I wonder if it's possible/practical to unit test upgrade routines? It'd be good for sanity checks, to make sure global tables aren't being upgraded when they shouldn't be.

[dd32]

In 33057:

Introduce wp_should_upgrade_global_tables() to determine if global tables should be upgraded automatically. Use it throughout the existing upgrade routines.

Props johnjamesjacoby for initial patch
Fixes #32154 for trunk

[dd32]

4.2 fixes can just be the defines, no need to backport the function IMHO.

I altered the patch from @johnjamesjacoby slightly by restoring some missing checks in the changes, and renamed it from wp_can_upgrade_global_tables to wp_should_upgrade_global_tables - WordPress can upgrade the tables, but it shouldn't.

[johnjamesjacoby]

Thanks for getting this in, Dion. A few thoughts:

• Is wp_should a convention we want to continue to use? I picked can since it matches existing nomenclature, and arguably WordPress should upgrade all tables all the time, but currently cannot for x/y/z reasons.
• Why remove the (bool) type cast? I think it's better to be explicit after filters are applied, to ensure strict comparisons don't produce undesirable results, especially with something sensitive like this.
• Thanks for adding the filter documentation in. Sorry I missed that bit.

[dd32]

Is wp_should a convention we want to continue to use? I picked can since it matches existing nomenclature, and arguably WordPress should upgrade all tables all the time, but currently cannot for x/y/z reasons.

On the other hand, WordPress can upgrade all tables all the time, but currently should not for x/y/z reasons (because it's not the right context, the constant is defined, etc.)

The only places we currently use can is when the code is making a decision based on the permissions and versions it has available to it. So for example, If it checked that it has the ALTER TABLE permission, then I'd expect it to use can.

Why remove the (bool) type cast? I think it's better to be explicit after filters are applied, to ensure strict comparisons don't produce undesirable results, especially with something sensitive like this.

I don't think we should be explicit when not necessary, there should be no strict checks on a boolean function, and plugins should behave themselves and return the documented data.
The only case where the removal would've been beneficial is when a) someone filters it and return a non-bool result, and b) when a plugin is running an explicit test, which wasn't needed in the first place.
There are no other cases (that I can see) where we use return (bool) apply_filters().

[johnjamesjacoby]

Replying to dd32:

The only places we currently use can is when the code is making a decision based on the permissions and versions it has available to it. So for example, If it checked that it has the ALTER TABLE permission, then I'd expect it to use can.

I understand. But we do use can in many places, and don't use should yet. Are we purposely introducing a new naming convention we can juice and reuse elsewhere, or adjusting it because it sorta reads nicer to some people?

I don't think we should be explicit when not necessary

We should be explicit, always.

There should be no strict checks on a boolean function

Doesn't the Scrutinzer ticket suggests otherwise?

plugins should behave themselves and return the documented data.

But they don't always, and WordPress can act responsibly to ensure global tables aren't accidentally updated because a plugin author made a boo boo.

There are no other cases (that I can see) where we use return (bool) apply_filters().

You are correct; we do not traditionally prevent return values of filters from doing terrible things in WordPress. We do occasionally in BuddyPress and almost always in bbPress, to guarantee the result being compared or requested is predictable, since plugins for plugins tend to have a lot of overlapping filters.

Thanks for the quick reply.

[dd32]

I understand. But we do use can in many places, and don't use should yet.

It looks like the only place we use should is in the Background Update methods. It looks like all other things are mostly can as they test the ability to (or a setting), followed by filters.

We should be explicit, always.

We actively discourage being explicit when not needed. We do not use if ( false === func() ) unless it's needed, we encourage if ( ! func() ) instead.

[dd32]

In 33058:

Don't upgrade global tables to utf8mb4 when wp_should_upgrade_global_tables() says not to.
Additionally fixes an incorrect conditional in [33057] which would result in the usermeta index being altered twice on Multisite updates.

See #32154

[dd32]

In 33059:

Don't upgrade global tables to utf8mb4 when DO_NOT_UPGRADE_GLOBAL_TABLES is defined.
This change also standardises on only checking DO_NOT_UPGRADE_GLOBAL_TABLES is defined, not it's value.

Fixes #32154 for 4.2

[johnjamesjacoby]

Thanks for being diligent with this Dion. A few more thoughts:

• After these changes, the 4.2 branch and trunk have two different conditions for when global tables will be upgraded. Is that acceptable?
• Re: r33059: it looks like the 4.2 branch will still attempt to upgrade global tables on each site of each network. Can you confirm that it's not?
• Re: r33058: How does the new is_multisite() check prevent the upgrade routine from running twice? The new function is designed to prevent global tables from being upgraded multiple times by only running on the main site of the main network. Under what conditions were you seeing the global table upgrade routine being repeated?

[dd32]

Something that's missing from this ticket description, is that the main issue here was that sites that were setup as single-site with a shared user table, ran the global updates on those shared user tables when they shouldn't have.

After these changes, the 4.2 branch and trunk have two different conditions for when global tables will be upgraded. Is that acceptable?

As far as I can tell, the 4.2, and trunk changes although slightly different conditionals due to the additional network checks are about the same, unless I've missed something, which is entirely possible because DB upgrades are a complete maze.
The 4.2 conditionals are not worse than in the past (they're as they should've been), and trunk conditionals appear to be better going forward.

Re: r33059: it looks like the 4.2 branch will still attempt to upgrade global tables on each site of each network. Can you confirm that it's not?

As far as I can tell, it doesn't. And it didn't previously. The change is not to upgrade global tables on single site installs when DO_NOT_UPGRADE_GLOBAL_TABLES is defined.

Re: r33058: How does the new is_multisite() check prevent the upgrade routine from running twice

upgrade_network() takes care of global tables in Multisite. pre_schema_upgrade is run for both Single site and Multisite, and handles the single site.
Without it, if global table upgrades are enabled, the index would be dropped and added in pre_schema_upgrade(), and then dropped and added again in upgrade_network()

[jeremyfelt]

More on nomenclature. :) We use wp_maybe_ and maybe_ in a few places for "should" checks.