WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 22 months ago

#12832 closed defect (bug) (fixed)

Use the same data type for site statuses

Reported by: ocean90 Owned by: database
Milestone: 3.7 Priority: normal
Severity: normal Version: 3.0
Component: Database Keywords: needs-patch dev-feedback
Focuses: Cc:

Description

For status archived we use:

archived enum('0','1') NOT NULL default '0',

Fo the others:

public tinyint(2) NOT NULL default '1',
mature tinyint(2) NOT NULL default '0',
spam tinyint(2) NOT NULL default '0',
deleted tinyint(2) NOT NULL default '0',

Shouldn't we use the same data type for the others?

Attachments (1)

12832.diff (1.9 KB) - added by nacin 22 months ago.

Download all attachments as: .zip

Change History (21)

comment:1 @nacin5 years ago

I'd have no problem doing this, but the way the schema is baked into the pluggable install_network(), instead of a global...

make_db_current() should be running any updates to the MS tables as well. This seems like something we definitely need to handle before release, because right now changes to the schema will be very convoluted.

comment:2 @ocean905 years ago

  • Keywords needs-patch added

comment:3 follow-up: @wpmuguru5 years ago

There are plugins that set some of those flags to something other than 0 or 1. If there is a consensus to change this to make them a consistent datatype then the archived flag should be changed to a tinyint.

comment:4 in reply to: ↑ 3 ; follow-up: @nacin5 years ago

Replying to wpmuguru:

There are plugins that set some of those flags to something other than 0 or 1. If there is a consensus to change this to make them a consistent datatype then the archived flag should be changed to a tinyint.

I agree. We eliminated enum fields in 2.5, this one should go too.

I'm leaving this in 3.0 because I think we need to make some wider adjustments. Any suggestions on how we can improve MS schema upgrades?

Is there a reason for install_network() to be pluggable? Or specifically, for the table definitions to be pluggable?

comment:5 @wpmuguru5 years ago

I'm not sure which ticket the checkboxes for site status were added, but they need to be switched back to radio buttons. If the checkboxes are not checked and the super admin updates the site's options, the tiny int values are set to 0 (even though before editing they had a value of something other than 0 or 1).

comment:6 in reply to: ↑ 4 ; follow-up: @wpmuguru5 years ago

Replying to nacin:

Is there a reason for install_network() to be pluggable? Or specifically, for the table definitions to be pluggable?

That was pluggable in MU. If someone wants to add columns to global tables, the schemas have to be pluggable so that a dbDelta in a subsequent upgrade doesn't drop the columns.

comment:7 in reply to: ↑ 6 @nacin5 years ago

Replying to wpmuguru:

Replying to nacin:

Is there a reason for install_network() to be pluggable? Or specifically, for the table definitions to be pluggable?

That was pluggable in MU. If someone wants to add columns to global tables, the schemas have to be pluggable so that a dbDelta in a subsequent upgrade doesn't drop the columns.

We didn't call it install_network() in MU though, they were simply part of the global variable WP uses. I think we should add them back into that variable, appending them conditionally. (And also appending global terms conditionally.)

We should be able to make changes to $wp_queries and have it work. Right now we have to call install_network() in an upgrade routine, which seems odd. If someone wants to add columns, they'd tap into $wp_queries like the rest, like how it was done in MU.

comment:8 @nacin5 years ago

(In [14277]) Restore radios for MS site flags, as the schema supports other values. see #12832

comment:9 @westi5 years ago

We should definitely avoid ENUMs for SQL schema we made that decision a long time ago to make it more db agnostic.

Need to read around in more detail to answer the other questions.

comment:10 @wpmuguru5 years ago

Per the IRC discussion, the global tables were excluded from upgrades in MU beginning with 2.8 (http://trac.mu.wordpress.org/changeset/1797/trunk/wp-admin/includes/schema.php).

So, IRT the 3.0 merge feature/enhancement, we don't need to include install_network() in the upgrade process.

I agree that we should eliminate the enum and incorporate the global tables into the upgrade process. For 3.0, we could change the network schema away from the enum because it would not affect existing MU installs, and put the incorporate the schema in the upgrade process on the list for 3.1.

2 weeks isn't much time to warn MU install owners that if they have changed their global tables they have to create a custom version of install_network to preserve their changes.

comment:11 @nacin5 years ago

Works for me. Does install_network() even need to be pluggable in 3.0 then?

comment:12 @ryan5 years ago

  • Milestone changed from 3.0 to 3.1

Let's be conservative with schema changes in 3.0. Postponing to 3.1, where we can discuss how to handle schema changes to global ms tables.

comment:13 @hakre5 years ago

October is starting. Any feedback for 3.1 line-up? Postponing to 3.2?

comment:14 @nacin5 years ago

  • Milestone changed from Awaiting Triage to Future Release

We currently have no standard way to trigger an upgrade on global table schemas. At any real scale it isn't something we should be running.

We made a commitment to no schema changes in 3.1. Even as just a default on future sites, let's hold off until 3.2.

comment:15 @ocean903 years ago

Re-visit for 3.4?

comment:16 follow-up: @jeremyfelt23 months ago

  • Keywords dev-feedback added
  • Milestone changed from Future Release to 3.7

Moving to 3.7 for discussion, do we have a nicer way to modify schema now?

comment:17 in reply to: ↑ 16 ; follow-up: @nacin22 months ago

  • Component changed from Multisite to Database

Replying to jeremyfelt:

Moving to 3.7 for discussion, do we have a nicer way to modify schema now?

We do, but I'm not sure if ENUM converts to an integer field as smoothly as it does a varchar.

comment:18 in reply to: ↑ 17 ; follow-up: @pento22 months ago

Replying to nacin:

We do, but I'm not sure if ENUM converts to an integer field as smoothly as it does a varchar.

It doesn't. When an ENUM converts to an INT, it returns the ENUM index, not the CAST(... AS INT) value of the ENUM. In the case of the archived column, ENUM '0' has an index of 1, and ENUM '1' has an index of 2.

We could totally convert it to VARCHAR in 3.7, then INT in 3.8, though.

@nacin22 months ago

comment:19 in reply to: ↑ 18 @nacin22 months ago

Replying to pento:

It doesn't. When an ENUM converts to an INT, it returns the ENUM index, not the CAST(... AS INT) value of the ENUM. In the case of the archived column, ENUM '0' has an index of 1, and ENUM '1' has an index of 2.

That's what I assumed.

We could totally convert it to VARCHAR in 3.7, then INT in 3.8, though.

Unfortunately, we can't do a two-step like that. The upgrade routine is designed to work in layers, version by version, such that you could upgrade across many major versions at once and still be OK. The schemas + dbDelta() wouldn't know, of course, that one needs to do a double switch.

12832.diff tries doing this in one shot, in pre_schema_upgrade(). It actually bypasses dbDelta entirely, by doing both ALTER queries at the same time.

comment:20 @nacin22 months ago

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

In 25448:

Multisite blogs table: Convert the archived field from enum to tinyint to match the other status fields. fixes #12832.

Note: See TracTickets for help on using tickets.