WordPress.org

Make WordPress Core

Opened 7 years ago

Last modified 5 years ago

#4027 new enhancement

Upgrade function don't do ANY error checking whatsoever

Reported by: skjaeve Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 2.1.2
Component: Upgrade/Install Keywords: needs-patch
Focuses: Cc:

Description

When I upgraded from 2.0.5 to 2.1.2, there was an error in my MySQL privileges, so the ALTER TABLE commands failed:

    WordPress database error: [ALTER command denied to user 'aasmunds'@'localhost' for table 'wp_categories']
    ALTER TABLE wp_categories ADD COLUMN link_count bigint(20) NOT NULL default '0'

    WordPress database error: [ALTER command denied to user 'aasmunds'@'localhost' for table 'wp_categories']
    ALTER TABLE wp_categories ADD COLUMN posts_private tinyint(1) NOT NULL default '0'

    WordPress database error: [ALTER command denied to user 'aasmunds'@'localhost' for table 'wp_categories']
    ALTER TABLE wp_categories ADD COLUMN links_private tinyint(1) NOT NULL default '0'

There were also a lot of secondary errors due to the above failed commands. However, the upgrade.php script still claimed that all was well:

There's actually only one step. So if you see this, you're done. Have fun!

It would be more helpful if WP would detect the MySQL errors and actually say that something went wrong and must be fixed, instead of saying that all is well even though there were heaps of errors.

This only happens once: If I run the upgrade.php script again, it just says 'all is well' without any errors, so the MySQL commands that would fail are not run again. However, the database is still broken.

It would be better if WP discovered that an upgrade failed, and set a flag somewhere that said 'this upgrade must be run again'.

See also http://wordpress.org/support/topic/111398?replies=2

Attachments (1)

4027.diff (632 bytes) - added by Denis-de-Bernardy 5 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 rob1n7 years ago

  • Owner changed from anonymous to rob1n
  • Status changed from new to assigned

Looking.

comment:2 rob1n7 years ago

  • Milestone changed from 2.3 to 2.2
  • Owner rob1n deleted
  • Priority changed from low to high
  • Severity changed from normal to major
  • Status changed from assigned to new

There is currently no checking that wp_upgrade() or its associated functions failed. I'm not sure what to do about this, as having an actual error reporting system would require rewriting parts of some of the functions.

Perhaps we could make use of WP_Error?

comment:3 rob1n7 years ago

  • Severity changed from major to critical
  • Summary changed from upgrade.php doesn't detect that ALTER TABLE fails to Upgrade function don't do ANY error checking whatsoever

comment:4 rob1n7 years ago

  • Milestone changed from 2.2 to 2.3

comment:5 markjaquith7 years ago

We definitely should not be incrementing the DB version number if the upgrade commands fail, as that prevents the upgrade from working once any MySQL permissions issues are ironed out. Would also be nice to present the errors in a friendly way that points to solutions, but I think the priority here is making sure that once resolved, the upgrade will re-run and bring everything up-to-date.

comment:6 Nazgul7 years ago

  • Milestone changed from 2.3 (trunk) to 2.4 (future)

comment:7 westi6 years ago

  • Keywords early needs-patch added
  • Milestone changed from 2.5 to 2.6

I think it's a bit late to go playing with the upgrade code for 2.5 punting to 2.6

comment:8 follow-up: Scott H6 years ago

  • Cc Scott H added

Any discussion on how this could be implemented? Should failed queries cause the script to bail? This would fix a few of the problems by displaying an error but probably doesn't help the upgrade process and may interfere elsewhere? (I've been trying to think of where a query error shouldn't lead to a fatal error).

comment:9 Scott H6 years ago

  • Cc scott@… added; Scott H removed

comment:10 mrmist6 years ago

Something could probably be done with SHOW GRANTS FOR CURRENT_USER; (mysql 4.1) before the upgrade, but it may be tricky to parse through for the myriad of possibilities.

comment:11 in reply to: ↑ 8 Denis-de-Bernardy5 years ago

Any discussion on how this could be implemented? Should failed queries cause the script to bail?

if so, they should only bail once all of the dbDelta queries have run, but not at any point after -- and this would only really treat a problem in an alter table statement.

keep in mind that you cannot rollback. if dbDelta worked, the rest should work as well... whereas if you're moving the stuff from post_categories into newly created terms tables, and it stops in the middle and refuses to go any further when it runs into an error, your install needs some severe db maintenance before you can recover anything.

imo, if the user knows SQL well enough to configure the permissions to something other than "grant all" or nearly so, surely he can fix his install if something goes wrong. the only thing he does need to see are the db error messages, which are no longer shown since around 2.5 or 2.6.

Denis-de-Bernardy5 years ago

comment:12 Denis-de-Bernardy5 years ago

  • Keywords has-patch dev-feedback added; early needs-patch removed

comment:13 Denis-de-Bernardy5 years ago

  • Milestone changed from 2.9 to 2.8

comment:14 ryan5 years ago

  • Component changed from Administration to Upgrade/Install

comment:15 hakre5 years ago

+1 would like to see messages in there as well. had an issue with WPMU that is compareable. the fix itself is lighweight and will lead to better usability.

comment:16 Denis-de-Bernardy5 years ago

  • Keywords commit added; dev-feedback removed

comment:17 ryan5 years ago

I'm not sure dbDelta runs clean enough to always show errors. Do we still get that duplicate key warning?

DB errors should be logged to the error log. Savvy users should be able to make use of that.

comment:18 Denis-de-Bernardy5 years ago

  • Keywords needs-patch added; has-patch commit removed
  • Milestone changed from 2.8 to Future Release

comment:19 hakre5 years ago

maybe a patch that sets logging to on for that operation for 2.8? afaik wpdb does logging already.

comment:20 Denis-de-Bernardy5 years ago

@ryan: I've two dozen db files with Semiologic Pro sites dating back all the way to WP 1.5. I'll give the existing patch a shot when I test my upgrade scripts.

@hakre: if it's not on already, it would make sense, yes.

comment:21 Denis-de-Bernardy5 years ago

  • Severity changed from critical to normal

comment:22 Denis-de-Bernardy5 years ago

  • Milestone changed from Future Release to 2.9

comment:23 Denis-de-Bernardy5 years ago

  • Milestone changed from 2.9 to Future Release
  • Priority changed from high to normal
  • Type changed from defect (bug) to enhancement
Note: See TracTickets for help on using tickets.