WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 23 months ago

#33206 closed defect (bug) (fixed)

Ignore the sitecategories table during network update if global terms are not enabled

Reported by: nofearinc Owned by: dd32
Milestone: 4.3 Priority: low
Severity: normal Version: 4.2
Component: Upgrade/Install Keywords: has-patch commit
Focuses: Cc:

Description

While updating a multisite network, I saw that in the error log:

    WordPress database error Table 'automobile.au_sitecategories' doesn't exist for 
    query SHOW FULL COLUMNS FROM `au_sitecategories` made by 
    wp_upgrade, upgrade_network, maybe_convert_table_to_utf8mb4

I found that upgrade_network runs maybe_convert_table_to_utf8mb4 in wp-admin/includes/upgrade.php by fetching $wpdb->tables( 'global' ); which always returns the sitecategories table even if it wasn't defined.

I've added the solution provided by nacin for #12964 in both changeset upgrade statements to ignore the database query if global terms are not enabled. I also wonder whether this shouldn't be handled in the wpdb class when calling the switch within the tables function.

Attachments (6)

33206.patch (943 bytes) - added by nofearinc 2 years ago.
33206.diff (450 bytes) - added by obenland 23 months ago.
33206.2.diff (470 bytes) - added by SergeyBiryukov 23 months ago.
33206.3.diff (867 bytes) - added by dd32 23 months ago.
untested; based off of nofearinc's patch
33206.4.diff (867 bytes) - added by pento 23 months ago.
33206.wpdb.diff (847 bytes) - added by tellyworth 23 months ago.
replace $this with $wpdb in r33597

Download all attachments as: .zip

Change History (28)

@nofearinc
2 years ago

#1 @SergeyBiryukov
2 years ago

  • Keywords commit added

#2 @obenland
23 months ago

@pento/@dd32 could you take a look at this and punt/commit?

#3 @dd32
23 months ago

This really feels like it should be handled inside wpdb::tables(), but there's no need to check the DB IMHO, just seeing if the feature is enabled or not is probably enough.

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


23 months ago

#5 @obenland
23 months ago

  • Keywords needs-refresh added; commit removed

@nofearinc, could you update your patch with Dion's feedback?

#6 @obenland
23 months ago

  • Owner set to dd32
  • Status changed from new to assigned

@obenland
23 months ago

#7 @obenland
23 months ago

  • Keywords needs-refresh removed

33206.diff: Am I doing this right? Should it only be unset if blog prefixes are requested?

#8 follow-up: @SergeyBiryukov
23 months ago

33206.diff causes a fatal error:

Fatal error: Call to undefined function wp_cache_get() in wp-includes/option.php on line 995

#9 in reply to: ↑ 8 ; follow-up: @SergeyBiryukov
23 months ago

Replying to SergeyBiryukov:

33206.diff causes a fatal error

In wp-settings.php, wp_set_wpdb_vars() happens before wp_start_object_cache(), so the object cache is not yet available. We're trying to call get_site_option() via global_terms_enabled() too early.

Checking the DB table seems to work, see 33206.2.diff.

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


23 months ago

#11 @pento
23 months ago

The disconnect between the setting and the existence of the table is slightly off-putting, but we'd have the same thing if we tried to check the setting, instead. With that in mind, I think checking for the table's existence is the more correct option for WPDB.

I don't think we have any global terms unit tests. It'd be nice to add a simple test for this, but it's probably the least of our concerns if we really wanted to test global terms.

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


23 months ago

@dd32
23 months ago

untested; based off of nofearinc's patch

#13 in reply to: ↑ 9 @dd32
23 months ago

Replying to SergeyBiryukov:

Replying to SergeyBiryukov:

33206.diff causes a fatal error

In wp-settings.php, wp_set_wpdb_vars() happens before wp_start_object_cache(), so the object cache is not yet available. We're trying to call get_site_option() via global_terms_enabled() too early.

Checking the DB table seems to work, see 33206.2.diff.

We could flip it so that wp_set_wpdb_vars() happens after wp_start_object_cache(), however, get_site_option still requires that wp_set_wpdb_vars() has run, so we've got a circular dependency we can't work around.

So; I'd rather handle it outside of wpdb if a query would always be required. 33206.3.diff is a untested version of nofearinc's patch which IMHO should be good to go.

#14 @pento
23 months ago

Just for fixing this specific bug, I'm cool with 33206.3.diff (minus the is_multisite() check, which already happens in global_terms_enabled()).

It kind of depends on if we want to put any effort at all into maintaining global terms, or if it's finally time to purge it.

Last edited 23 months ago by pento (previous) (diff)

@pento
23 months ago

#15 follow-up: @pento
23 months ago

Upon further reflection, lets kill off global terms.

In the mean time, doing the check during upgrade is the least offensive method to fix this bug, though I prefer checking that the table exists, as we're performing operations directly on the table: 33206.4.diff.

#16 in reply to: ↑ 15 @dd32
23 months ago

Replying to pento:

In the mean time, doing the check during upgrade is the least offensive method to fix this bug, though I prefer checking that the table exists, as we're performing operations directly on the table: 33206.4.diff.

Lets make that happen then! (Just noting, 33206.4.diff is very similar to 33206.patch)

#17 @obenland
23 months ago

  • Keywords commit added

#18 @dd32
23 months ago

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

In 33597:

Upgrade: Skip the sitecategories table when it doesn't exist (Global Terms is disabled).
Props nofearinc, obenland, SergeyBiryukov, and pento.
Fixes #33206

#19 @boonebgorges
23 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[33597] uses $this when it should use $wpdb, and causes fatal errors during network updates.

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


23 months ago

@tellyworth
23 months ago

replace $this with $wpdb in r33597

#21 @azaozz
23 months ago

33206.wpdb.diff fixes the typos, +1 to commit.

#22 @azaozz
23 months ago

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

In 33609:

Fix paste typos in upgrade.php.
Props tellyworth. Fixes #33206.

Note: See TracTickets for help on using tickets.