[wp-trac] [WordPress Trac] #27240: Add allow_bail Argument for wpdb->check_connection() the Same as for db_connect()

[WordPress Trac]

#27240: Add allow_bail Argument for wpdb->check_connection() the Same as for
db_connect()
-------------------------+-----------------------
 Reporter:  DrProtocols  |       Owner:
     Type:  enhancement  |      Status:  reopened
 Priority:  normal       |   Milestone:
Component:  Database     |     Version:  trunk
 Severity:  normal       |  Resolution:
 Keywords:               |     Focuses:
-------------------------+-----------------------
Changes (by DrProtocols):

 * status:  closed => reopened
 * resolution:  wontfix =>


Comment:

 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.

 2. 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.

 2. 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

--
Ticket URL: <https://core.trac.wordpress.org/ticket/27240#comment:2>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform