Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#33206 closed defect (bug) (fixed)

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

Reported by: nofearinc's profile nofearinc Owned by: dd32's profile 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 10 years ago.
33206.diff (450 bytes) - added by obenland 10 years ago.
33206.2.diff (470 bytes) - added by SergeyBiryukov 10 years ago.
33206.3.diff (867 bytes) - added by dd32 10 years ago.
untested; based off of nofearinc's patch
33206.4.diff (867 bytes) - added by pento 10 years ago.
33206.wpdb.diff (847 bytes) - added by tellyworth 10 years ago.
replace $this with $wpdb in r33597

Download all attachments as: .zip

Change History (28)

@nofearinc
10 years ago

#1 @SergeyBiryukov
10 years ago

  • Keywords commit added

#2 @obenland
10 years ago

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

#3 @dd32
10 years 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.


10 years ago

#5 @obenland
10 years ago

  • Keywords needs-refresh added; commit removed

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

#6 @obenland
10 years ago

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

@obenland
10 years ago

#7 @obenland
10 years 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
10 years 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
10 years 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.


10 years ago

#11 @pento
10 years 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.


10 years ago

@dd32
10 years ago

untested; based off of nofearinc's patch

#13 in reply to: ↑ 9 @dd32
10 years 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
10 years 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 10 years ago by pento (previous) (diff)

@pento
10 years ago

#15 follow-up: @pento
10 years 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
10 years 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
10 years ago

  • Keywords commit added

#18 @dd32
10 years 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
10 years 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.


10 years ago

@tellyworth
10 years ago

replace $this with $wpdb in r33597

#21 @azaozz
10 years ago

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

#22 @azaozz
10 years 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.