WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#32154 closed defect (bug) (fixed)

Don't upgrade global tables for utf8mb4

Reported by: ocean90 Owned by: dd32
Milestone: 4.2.3 Priority: normal
Severity: normal Version: 4.2
Component: Database Keywords: has-patch fixed-major
Focuses: multisite Cc:

Description

Attachments (2)

32154.patch (1.2 KB) - added by ocean90 3 years ago.
32154.02.patch (5.5 KB) - added by johnjamesjacoby 3 years ago.
Introduce wp_can_upgrade_global_tables()

Download all attachments as: .zip

Change History (23)

@ocean90
3 years ago

This ticket was mentioned in Slack in #core-multisite by ocean90. View the logs.


3 years ago

#2 @johnjamesjacoby
3 years ago

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
3 years ago

Introduce wp_can_upgrade_global_tables()

#3 @johnjamesjacoby
3 years ago

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

This ticket was mentioned in Slack in #core-multisite by jjj. View the logs.


3 years ago

#6 @ocean90
3 years ago

@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.

#7 @pento
3 years ago

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.

This ticket was mentioned in Slack in #core by netweb. View the logs.


3 years ago

#9 @netweb
3 years ago

  • Milestone changed from 4.1.3 to 4.2.3

#10 @dd32
2 years ago

  • Owner set to dd32
  • Resolution set to fixed
  • Status changed from new to closed

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

#11 @dd32
2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#12 @johnjamesjacoby
2 years ago

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.
Last edited 2 years ago by johnjamesjacoby (previous) (diff)

#13 follow-up: @dd32
2 years ago

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().

#14 in reply to: ↑ 13 @johnjamesjacoby
2 years ago

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.

#15 @dd32
2 years ago

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.

#16 @dd32
2 years ago

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

#17 @dd32
2 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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

#18 @johnjamesjacoby
2 years ago

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?

#19 @dd32
2 years ago

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()

#20 @jeremyfelt
2 years ago

  • Focuses multisite added

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

#21 @samuelsidler
2 years ago

  • Keywords fixed-major added
Note: See TracTickets for help on using tickets.