Make WordPress Core

Opened 13 years ago

Closed 10 years ago

Last modified 5 years ago

#16615 closed defect (bug) (fixed)

More inline docs needed to explain DB errors esp. dead_db() and effects of WP_DEBUG

Reported by: jeremyclarke's profile jeremyclarke Owned by: drewapicture's profile DrewAPicture
Milestone: 4.0 Priority: normal
Severity: normal Version:
Component: Database Keywords: has-patch commit
Focuses: docs Cc:

Description

The Problem: DB Error reporting can't be modified

I'm trying to set my sites up for some scheduled downtime of our (separate) MySQL server. I am hoping that I can show cached pages if they are requested, but show a 'We are down for scheduled maintenance' message when a user loads a page that needs the database.

In the process I'm seeing there are many different places in the code that fire depending on exactly how broken the database is. Specifically, there are some scenarios where dead_db(), which is totally different from the rest of the messages, will fire. dead_db() is cool because it promises to let us use a /wp-content/db-error.php file to control output in case of a db error, but currently it is just frustrating because most types of db error (server missing, db missing) don't cause dead_db() to fire, but instead use $wpdb::bail(). These bail()-based errors are used in wp-db.php.

However dead_db() CAN still fire, which I know because our site often has database outages that result in the H2 from dead_db() being shown. I think it's a certain mix of "the mysql server and database seem to exist, but are failing to respond to actual queries". That said, I'm pretty sure that the scenarios where dead_db still fires are ones also covered by some of the $wpdb::bail() situations, and would be better off handled by one consistent system of errors. dead_db() should either be used for all DB related errors or deprecated, otherwise it is just an awkward red-herring for developers.

The Solution: Better filters on $wpdb::bail()

I think that all these messages need to be pluggable somehow and it should be clear how to do so when looking at the code itself. Asking people who want to change a wp_die() message to find it in the code is reasonable, but it should be clear from there how to change it. Ideally there would be a filter in the function that calls bail() that lets you edit the text and/or forward the user to another URL where a maintenance message lives. It should be of of those situations where finding the source of the message also finds the means of changing it.

The simplest answer would be a filter added in $wpdb::bail() that used the $error_code to identify the specific message. In the database errors the $error_code passed to bail() are useful slug-type strings like 'db_select_fail'. Something like:

apply_filters('wpdb_bail', $error_code, $message);
wp_die($message);

This would give people a lot of control, and could easily be referred to in a comment before any given instance of $wpdb::bail().

To make the $error_code easier to find I think it's also worth reformatting the code used to call $wpdb::bail(). Currently it takes this form:

$this->bail( sprintf( /*WP_I18N_DB_CONN_ERROR*/"
...
SUPER LONG HTML MESSAGE
...
"/*/WP_I18N_DB_CONN_ERROR*/, $details['db_host'] ), 'db_connect_fail' );

This makes it hard to notice the 'db_connect_fail' string all the noise. Instead the message perparation and bail() call should be on two lines, one for defining the message and another that only calls bail (with an explanation about the $error_message and filter above it).

Looking at it deeper I imagine I can achieve what I want by hooking into the 'wp_die_handler' filter, checking for the exact HTML generated by the DB error (the $message value in the code above), and doing something based on that, but it's obviously a house of cards for future updates where the text might change. It will also be easily foiled by any translation of the $message which will change its output.

Attachments (3)

16615.diff (367 bytes) - added by ericlewis 11 years ago.
a bit more documentation on wpdb show errors
16616.1.diff (387 bytes) - added by ericlewis 10 years ago.
16615.1.diff (387 bytes) - added by ericlewis 10 years ago.

Download all attachments as: .zip

Change History (14)

#1 @jeremyclarke
13 years ago

  • Component changed from General to Comments
  • Summary changed from Inconsistent and un-pluggable handling of db error messages esp. dead_db() to More inline docs needed to explain DB errors esp. dead_db() and effects of WP_DEBUG

Ugh. I just realized that none of the wpdb::bail() messages show unless WP_DEBUG is set to true. This is my fault for not noticing.

It seems that as long as WP_DEBUG is not true dead_db() does in fact fire and use the wp-content/db-error.php file as described.

Apologies for the noise in the ticket above. I think that all we really need at this point is more comments in the source to make it clearer what is happening and when. The way it's set up now you'd have to dig around forever to realize what's going on.

#2 @ocean90
13 years ago

  • Component changed from Comments to Inline Docs
  • Keywords needs-docs added

@ericlewis
11 years ago

a bit more documentation on wpdb show errors

#3 @c3mdigital
11 years ago

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

#4 follow-up: @SergeyBiryukov
11 years ago

The patch mentions WP_DEBUG twice. It should be WP_DEBUG and WP_DEBUG_DISPLAY.

#5 @nacin
10 years ago

  • Component changed from Inline Docs to Bootstrap/Load
  • Focuses docs added

#6 in reply to: ↑ 4 ; follow-up: @DrewAPicture
10 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from Awaiting Review to Future Release
  • Type changed from enhancement to defect (bug)

Replying to SergeyBiryukov:

The patch mentions WP_DEBUG twice. It should be WP_DEBUG and WP_DEBUG_DISPLAY.

@ericlewis: Care to update the patch? This can go in once we move to the 4.0 milestone.

#7 @samuelsidler
10 years ago

  • Milestone changed from Future Release to 4.0

@ericlewis
10 years ago

@ericlewis
10 years ago

#8 in reply to: ↑ 6 @ericlewis
10 years ago

Replying to DrewAPicture:

@ericlewis: Care to update the patch?

Sure, updated in attachment:16615.1.diff

#9 @SergeyBiryukov
10 years ago

  • Component changed from Bootstrap/Load to Database
  • Keywords has-patch commit added; needs-patch removed

#10 @DrewAPicture
10 years ago

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

In 28243:

Improve inline documentation for the wpdb::$show_errors property.

Note that SQL/DB errors are displayed by default if both WP_DEBUG and WP_DEBUG_DISPLAY evaluate to true.

Props ericlewis.
Fixes #16615.

This ticket was mentioned in Slack in #core by sergey. View the logs.


5 years ago

Note: See TracTickets for help on using tickets.