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

[jeremyclarke]

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.

[jeremyclarke]

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.

[ericlewis]

a bit more documentation on wpdb show errors

[SergeyBiryukov]

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

[DrewAPicture]

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.

[ericlewis]

Replying to DrewAPicture:

@ericlewis: Care to update the patch?

Sure, updated in attachment:16615.1.diff​

[DrewAPicture]

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.