[wp-trac] [WordPress Trac] #30006: wpdb API refactoring

Thu Oct 16 14:32:32 UTC 2014

#30006: wpdb API refactoring
-------------------------+-----------------------------
 Reporter:  aercolino    |      Owner:
     Type:  enhancement  |     Status:  new
 Priority:  normal       |  Milestone:  Awaiting Review
Component:  Database     |    Version:  4.0
 Severity:  normal       |   Keywords:
  Focuses:               |
-------------------------+-----------------------------
 I wanted to simplify the task of reading and understanding code of wpdb,
 and noticed that that pattern `if is_X then do_X else do_Y` (which affects
 many wpdb methods) was distracting my focus onto differences between APIs.
 It was also causing some code duplication. I found out that that pattern
 was being used in 4 different ways (patterns themselves), so I made this
 refactoring by adding a bit of structure to code.

 This contains no dependency injection (#9953), no interfaces (#21055), no
 database drivers (#21663) but only a refactoring of code introduced in
 #21663. (starting from comment 135)
 * First I got the list of mysql_* functions at
 http://php.net/manual/en/ref.mysql.php and manually searched for their
 correspondent mysqli_* functions into http://php.net/. (I hope I didn’t
 miss any.)
 * Then I selected mysql* uses in wpdb and extracted them into a new
 wpdb_API class, which wpdb now extends.

 '''Original title''': wpdb API refactoring

 (sorry for the length of this description; I tried to document and justify
 all bits)

 === Patterns detected into wpdb ===

 '''Pattern A''', suitable when mysql_* and mysqli_* functions have the
 same arguments, in the same order. (I applied this wherever I see fit)
 {{{
     extract($this->api_vars(‘function’));  // oops, extract!! Keep
 reading...
     $mysqli_function(...);
 }}}

 '''Pattern B''', suitable when mysql_* and mysqli_* functions have the
 same arguments, in the same order or not. (I applied this wherever I see
 fit)
 {{{
     $this->api_call( 'mysqli_function', ... )
 }}}
 The reason for having A while B would have been enough, you might wonder,
 is twofold.
 On one side, A is needed for doing things like
 {{{
     if ( function_exists( $mysqli_function ) ...
 }}}
 On the other side, A is quite nice to use right now for this patch,
 instead of B, because it allows to replace the `if is_mysqli then
 mysqli_do else mysql_do` branching simply with `$mysqli_do`. Variable
 functions are so PHP!

 '''Pattern C''', suitable when the difference between mysql and mysqli
 branches is a bit more than calling a correspondent function (with or
 without an “i” in the middle). It consists of extracting the branching
 into its own wpdb_API protected method. (I applied this once)

 Example: wpdb::set_sql_mode. It had a piece of code that, after applying
 the B refactoring, still needed like 25 lines to get the current SQL
 modes. So I extracted that code into its own protected method
 wpdb_API::db_modes.

 '''Pattern D''', suitable in any other case, i.e. when the difference
 between mysql and mysqli branches is quite big. It consists of extracting
 the three pieces of the branching (api_check, mysql_branch and
 mysqli_branch) into their own wpdb_API protected methods. (I applied this
 once)

 Example: wpdb::db_connect. It was quite big and complicated, including a
 recursion step under certain circumstances. I cut the code into different
 pieces, and moved them up into wpdb_API in their own protected methods
 wpdb_API::db_connect, wpdb_API::mysql_connect and
 wpdb_API::mysqli_connect. With this separation the need for a recursion
 step vanished.

 === Notes (see also documentation in the code) ===

 '''PHP extract function'''
 WP has become a very challenging system: Many bits of code have a big
 ticket behind. (I don’t say I don’t like challenges, because I do)

 In this case: #27881. It seems that extract is getting discouraged in WP
 for improving HHVM compatibility. In this refactoring, we could just use B
 instead of A, or we could also use `$function[‘mysqli’][‘name’]` instead
 of `$mysqli_name` but it looks much uglier.

 If the problem with extract is that we can’t easily spot where variables
 in the current scope come from (#22400), that is easy to solve by adding
 `/** @var $mysqli_* */` annotations soon after calling extract, as I did
 here.

 So, for the time being, I’ve left extract in place.

 '''Put into wpdb_API: $dbuser, $dbpassword, $dbname, $dbhost, $dbh'''
 I thought that these connection data made more sense into wpdb_API than
 wpdb, so I moved them. They make more sense not because of the API part
 per se but because of the presence of wpdb_API::db_connect, which uses
 them.

 '''Added wpdb::filter_query, wpdb::filter_row'''
 A code comment for the `apply_filters(‘query’, $query)` call in the
 wpdb::query method says "Some queries are made before the plugins have
 been loaded, and thus cannot be filtered”. That was one of the reasons for
 writing my Full UTF-8 plugin as a drop-in.

 Due to the lack of those two methods, I had to override wpdb::query (and
 recently also _do_query… see below). Now they are defined to echo back
 whatever they get and are called immediately before querying and
 immediately after retrieving the results.

 With these two methods in place, my drop-in gets zero duplication. (I
 tried it out) If you are concerned about performances slow down, I can
 tell you that my plugin blindly escapes all the query and un-escapes all
 the results and I don’t notice it. (used for 4 years) Lately I tried to
 benchmark it precisely (also using 200KB "lorem ipsum" posts) but didn’t
 get any consistent trend. Some times it was slower and some other times
 even faster with my plugin activated. I think the reason is that in the 40
 queries needed for one editing page generation, all these PHP time
 differences disappear in comparison to lags for moving info to and from
 the database.

 This is a small change that will allow less duplication in drop-ins.
 (nasty duplication, because plugins developers do not constantly monitor
 WP code to promptly apply needed patches).

 '''Deprecated wpdb::$use_mysqli'''
 To open the path to new APIs in the future, I thought that a string would
 better fit this scenario than a boolean: `‘mysqli’ == wpdb_API::$api`.

 Given to how property overloading methods are implemented in wpdb, even if
 wpdb::$use_mysqli was declared private, it’s not possible to just remove
 it, because other developers could have used it through that backdoor.

 So wpdb::$use_mysqli is now deprecated (and read only). See below for
 deprecated property functionality.

 '''Deprecated wpdb::$has_connected'''
 While perusing wpdb::db_connect I saw that the fallback flow could be made
 more explicit, cleaner, and we could avoid 1-step-recursion and get rid of
 wpdb::$has_connected.

 $has_connected was a bit buggy, in my opinion. If I’m not wrong, it was
 introduced for telling apart the second execution (recursion step) from
 the first one, but the check was ill placed, causing $has_connected to
 never be checked at run time.

 In fact, suppose mysqli_connect is called because $use_mysqli is true but
 fails for some reason and a fallback is attempted. In that case the
 recursion step is triggered after putting $use_mysqli to false. During the
 second execution, mysql_connect is called.
 * In case of success, $dbh is true-y, so $has_connected becomes true and
 the execution ends returning true to the caller.
 * In case of failure, $dbh is false-y and either we bail or not, but the
 execution ends returning false to the caller.
 BUT the caller is the first execution, so the returned value is discarded
 and, AFAICT, we’ll again execute the same code we just did. It works “OK”
 because all that code must be repeatable (somehow) and the original caller
 ends getting the correct return value because $dbh didn’t change after
 coming back from the second execution to the first one.

 Given to how property overloading methods are implemented in wpdb, even if
 wpdb::$has_connected was declared private, it’s not possible to just
 remove it, because other developers could have used it through that
 backdoor.

 So wpdb::$has_connected is now deprecated (and read only). See below for
 deprecated property functionality.

 '''Added wpdb_API::$fallback'''
 The wpdb::db_connect local variable $attempt_fallback was based on the
 same checks used for wpdb::$is_mysqli so I introduced a
 wpdb_API::$fallback property, which can have four states: impossible,
 possible, success, failure.

 Initially it’s null. wpdb_API::init_api_to_use (called from the
 constructor) sets it to either “possible” or “impossible”. And
 wpdb_API::db_connect sets it to either “success” or “failure”, only if a
 fallback was possible and has been attempted.

 '''Added deprecated properties (separate patch)'''
 Given that property overloading methods are already implemented in wpdb,
 it was natural to use them for signaling deprecation of wpdb::$use_mysqli
 and wpdb::$has_connected.

 I added all the bits (hopefully) for the functionality along the lines of
 function and argument deprecations. I added a test to make sure the
 deprecation error is triggered.

 '''wpdb extends wpdb_API'''
 Drop-ins could extend wpdb_API instead of wpdb, taking advantage of proven
 code for connecting and interfacing the API directly, thus helping reduce
 duplication in drop-ins.

 '''Refactoring of wpdb::set_sql_mode'''
 The foreach that computed $modes minus $incompatible_modes is now
 array_diff($modes, $incompatible_modes).

 '''Fixed 'deprecated_file_included' in tests'''
 File deprecation had a couple of documentation errors coming from copying
 and pasting argument deprecation.
 * In src/wp-includes/functions.php, we call
 {{{
     do_action( 'deprecated_file_included', $file, $replacement, $version,
 $message );
 }}}
 * In tests/phpunit/tests/functions/deprecated.php
  * we did bind the action to ‘deprecated_file’ but unbind it from
 ‘deprecated_argument’.
  * we defined
 {{{
     public function deprecated_file( $file, $version, $replacement,
 $message ) {
 }}}
 I assumed this was another error due to copy and paste so I fixed the
 signature of the handler (deprecated_file) to have the arguments in the
 same order as they appear in the trigger (do_action) and then I unbound it
 from 'deprecated_file_included'.

 '''Should be protected wpdb::_do_query'''
 In the last update to my Full UTF-8 drop-in not only I had to override
 wpdb::query (like I used to) but also wpdb::_do_query for no other reason
 than that it was declared private. This is another occurrence of what I
 called before “nasty duplication” BUT I don’t understand how to fix it
 yet.

 I tried to change it to protected and it worked perfectly, except for a
 fatal error (sigh, it’s not a joke). The fatal error was due to having
 made it protected in wpdb but having it still private in my drop-in. Which
 means we now can’t change it without breaking stuff.

 '''@since X4.2'''
 Wherever I introduced something, I marked it with this annotation, to be
 able to do a search and replace.

 '''Tests clean for wordpress-develop on October 16, 2014 at 13:15 UTC.'''
 I updated from svn and ran phpunit and I had no failures due to this
 refactoring.

--
Ticket URL: <https://core.trac.wordpress.org/ticket/30006>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform