Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#27240 closed enhancement (fixed)

Add allow_bail Argument for wpdb->check_connection() the Same as for db_connect()

Reported by: drprotocols's profile DrProtocols Owned by: pento's profile pento
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.9
Component: Database Keywords: has-patch commit
Focuses: Cc:

Description

The new wpdb check_connection() function is a great addition and potentially allows plugins to change to using that function rather than having to use their own (especially good now that there is at the mysql/mysqli thing to be considered).

However, as it bails by default (unless the template_redirect action has been done) this causes a problem for plugins that handle the broken and cannot reconnect failure themselves.

The db_connect() function has an allow_bail argument that allows the caller to prevent the bail action and simply have db_connect() return false if a connection failure occurs.

Adding the same argument and functionality to check_connection() (defaulting to true as it does for db_connect()) would allow the caller to to have check_connection() return false on a failure and allow the caller to handle the failure.

This would provide a behavioural consistency between the two functions as in effect they are both trying to connect to the database.

The call to db_connect() from within check_connection() would still call with false regardless of the value that check_connection() was called with.

(I don't believe that the template_redirect action can be guaranteed to have been "done" under every possible scenario where a plugin might want to check a database connection which is why I believe the additional argument and associated logic is required. For example, would template_redirect have been called if a background (cron) task were being executed? This condition seems too uncertain to be relied upon).

Regards

Attachments (1)

27240.diff (934 bytes) - added by pento 10 years ago.

Download all attachments as: .zip

Change History (8)

#1 @pento
10 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Thanks for the suggestion!

So, there are a few different paths for this code.

  • Before template_redirect:
    1. If WP_DEBUG and WP_DEBUG_DISPLAY are set, wpdb::bail() will call wp_die(). This will normally only be the case on dev sites.
    2. If they're not true, dead_db() will be called.
  • After template_redirect:
    1. wpdb::check_connection() will not die, it will return false, which will usually translate into wpdb::query() returning false.

For the first case, while it's possible (using the wp_die_handler filter), I'd suggest that a plugin wouldn't want to change this - wpdb's usage of wp_die() is to give feedback in a dev/testing environment, not in production.

For the second case, dead_db() can be overridden by adding a custom DB template at WP_CONTENT_DIR . '/db-error.php'. This covers the vast majority (I would guess >99%) of cases where the DB is dead, as it dies before WP can even connect to it. It's done this way, because dead_db() can be called before plugins are loaded.

For the third case, this is much trickier to handle, given that it could happen any time during page generation, from the very beginning of the header, right through to the end of the footer. I would imagine any custom handling for this would be very different to the handling of the other two cases, so I'm okay with a plugin having to have different handlers for different failures.

(The extra scenario, as you mentioned, are uses that will never do template_redirect. These include wp-cron, admin-ajax, and anything that loads WP by calling wp-load.php directly. These cases are comfortably covered by wp_die() and dead_db().)

#2 @DrProtocols
10 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

Thanks for your initial analysis!

To cut to the chase and consider wp-cron initiated (background) tasks specifically - as you state template_redirect will never be done so processing will drop through to call $this->bail() with a message.

As you state, if $this->show_errors is false then wp_die() is not called but we return to check_connection() with false - but the return value is not used, instead dead_db() is immediately called.

So a couple of issues here:

  1. Some WP users (however erroneously you may think) will have the necessary conditions set for $this->show_errors to be true even if they are not in a dev environment - so in that case processing descends into wp-die() and the failure condition that we want to (currently do through our own database connection test function) handle quite happily causes WP to just die - and because this is a cron task there is no browser or user to see any death message and so to the user it just looks like the plugin broke - so a plugin support then has to jump through hoops to diagnose a simple issue that could have been seen if the plugin were allowed to handle the failure and log appropriate messages into the plugin log file.
  1. If the wind is in the right direction and bail() does return to the caller of check_connection() it still isn't allowed to handle the failure because the return value of bail() isn't tested but instead dead_db() is unconditionally called.

So if processing drops into dead_db() what happens - either a local custom db error template is loaded and then PHP die() is called OR drop through for a terse error page output to a non-existent browser/user followed by PHP die() again. So what's wrong here:

  1. The custom db error template is just that - a means to send a custom page to a browser, which of course doesn't exist here. Trying to use it for anything else would just be perverting that. And this is just a file - it's perfectly feasible that a theme or a DB related plugin would have already installed such a file which would be much more relevant or functional in the general case so another plugin cannot just come along and toss such a file aside. Possibly if this were some filter or action thing it might work but even then there is no proper context data to handle the failure as it is currently handled.
  1. With no custom db error template and not even any wp_die() to try and force to do something useful it's just a straight drop through to PHP die() - the plugin cannot handle the failure as it currently would, end of story.

As you also state, trying to use the wp_die_handler filter in some way is really not a good way to go and in any case there is no way to force a processing path that would always go through wp_die().

So we are left with there being no way that a wp-cron task could use the nice new check_connection() method to be able to check and potentially reconnect the database connection and itself handle any failure to reconnect as it can simply do at present using it's own method that the check_connection() function is so close to but just falls short of having equal utility to.

So the closing statement in the response above:

These cases are comfortably covered by wp_die() and dead_db().)

simply doesn't hold true for the case where the caller of check_connection() would want to handle the failure condition itself without having to cater for various possible processing paths entailing creating files that may conflict with other use cases or complex handler function processing which is simply pointless when check_connection() could simply be called with a parameter to request it to not move on to bailout handling in the event of failure but instead simply return a false value so the caller can know the reconnection failed and handle the failure itself.

Of course if I have misinterpreted or misunderstood anything and there is something very simple that can be done so that check_connection() can be called from a plugin cron task handler and it can be guaranteed to return false to the plugin in the event of reconnection failure then I would of course be very happy to know :-)

I would note that this function is a new function (and a nice, long overdue one at that) and that the purpose of this stage of the development process is for new functionality to be exposed to a wider audience of developers who can the look at it and say "...that's really nice, but if you just made this small update then I could really use that and I could junk my own version and we'd have a much cleaner and consistent code base..."

The change required is no more than the current logic within db_connect() which if the caller has passed in false to prohibit bailing in the event of a connection failure will simply return false to the caller.

The change required in check_connection() is simply to provide the same argument (which would have a default value of true as is the case with db_connect()) and if it is set true by the caller then after a reconnection failure simply return false rather than continue with bailout processing.

Since in essence check_connection() and db_connect() actually do exactly the same thing functionally - they connect to the database if possible - I can see no reason why they should not have the same functional logic in this respect?

So I would respectfully request that this be reconsidered as a very useful and functional enhancement to the check_connection() function in order to make it very much more useful and allow it to be used in more scenarios than it would currently be usable and thus allow for a simplification of users code bases and a more consistent handling of the check/reconnect operation.

Regards

#3 @SergeyBiryukov
10 years ago

  • Milestone set to Awaiting Review

#4 @johnbillion
10 years ago

  • Keywords 2nd-opinion added
  • Milestone changed from Awaiting Review to 3.9

Moving this to 3.9 (as the method was introduced in 3.9) so we can give it a yay or nay.

@pento
10 years ago

#5 @pento
10 years ago

  • Keywords has-patch commit added; 2nd-opinion removed
  • Owner set to pento
  • Status changed from reopened to accepted

Thanks for the extra feedback! This is a solid argument in favour of adding $allow_bail argument to check_connection().

attachment:27240.diff adds this.

#6 @nacin
10 years ago

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

In 27925:

Database: Add $allow_bail argument to wpdb::check_connection() to match the connect method.

props DrProtocols, pento.
fixes #27240.

#7 @DrProtocols
10 years ago

Thanks guys, every little helps :-)

Note: See TracTickets for help on using tickets.