Make WordPress Core

Opened 9 years ago

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

Download all attachments as: .zip

Change History (28)

@nofearinc
9 years ago

#1 @SergeyBiryukov
9 years ago

  • Keywords commit added

#2 @obenland
9 years ago

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

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


9 years ago

#5 @obenland
9 years ago

  • Keywords needs-refresh added; commit removed

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

#6 @obenland
9 years ago

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

@obenland
9 years ago

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


9 years ago

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


9 years ago

@dd32
9 years ago

untested; based off of nofearinc's patch

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

@pento
9 years ago

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

  • Keywords commit added

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


9 years ago

@tellyworth
9 years ago

replace $this with $wpdb in r33597

#21 @azaozz
9 years ago

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

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