Make WordPress Core

Opened 18 years ago

Closed 10 years ago

Last modified 10 years ago

#4027 closed enhancement (wontfix)

Upgrade function don't do ANY error checking whatsoever

Reported by: skjaeve's profile skjaeve Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.1.2
Component: Upgrade/Install Keywords: close
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 16 years ago.

Download all attachments as: .zip

Change History (26)

#1 @rob1n
18 years ago

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

Looking.

#2 @rob1n
18 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?

#3 @rob1n
18 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

#4 @rob1n
18 years ago

  • Milestone changed from 2.2 to 2.3

#5 @markjaquith
18 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.

#6 @Nazgul
18 years ago

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

#7 @westi
17 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

#8 follow-up: @Scott H
17 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).

#9 @Scott H
17 years ago

  • Cc scott@… added; Scott H removed

#10 @mrmist
17 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.

#11 in reply to: ↑ 8 @Denis-de-Bernardy
16 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.

#12 @Denis-de-Bernardy
16 years ago

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

#13 @Denis-de-Bernardy
16 years ago

  • Milestone changed from 2.9 to 2.8

#14 @ryan
16 years ago

  • Component changed from Administration to Upgrade/Install

#15 @hakre
16 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.

#16 @Denis-de-Bernardy
16 years ago

  • Keywords commit added; dev-feedback removed

#17 @ryan
16 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.

#18 @Denis-de-Bernardy
16 years ago

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

#19 @hakre
16 years ago

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

#20 @Denis-de-Bernardy
16 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.

#21 @Denis-de-Bernardy
16 years ago

  • Severity changed from critical to normal

#22 @Denis-de-Bernardy
16 years ago

  • Milestone changed from Future Release to 2.9

#23 @Denis-de-Bernardy
16 years ago

  • Milestone changed from 2.9 to Future Release
  • Priority changed from high to normal
  • Type changed from defect (bug) to enhancement

#24 @chriscct7
10 years ago

  • Keywords close added; needs-patch removed
  • Resolution set to wontfix
  • Status changed from new to closed

Both of these functions were deprecated in 2.6 and are no longer used.

#25 @DrewAPicture
10 years ago

  • Milestone Future Release deleted
Note: See TracTickets for help on using tickets.