WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#30006 closed enhancement (wontfix)

wpdb API refactoring

Reported by: aercolino Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.0
Component: Database Keywords: has-patch
Focuses: Cc:

Description

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.

Attachments (2)

add_deprecated_property.patch (10.0 KB) - added by aercolino 3 years ago.
extract_wpdb_API.patch (30.7 KB) - added by aercolino 3 years ago.

Download all attachments as: .zip

Change History (5)

#1 @aercolino
3 years ago

  • Keywords has-patch added

#2 @voldemortensen
3 years ago

I like this, but it adds a number of extract() functions that would be against the efforts of #22400.

Somebody more important that I would have to make that call though.

#3 @pento
3 years ago

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

While I appreciate the effort that you've put into this, we're not interested in refactoring WPDB at this time.

As a general rule, WordPress avoids refactoring code in large chunks, as it inevitably introduces new bugs, and makes the mission of maintaining backwards compatibility quite difficult.

We greatly appreciate contributions, but in the future, I'd recommend you open a ticket before you write such a huge amount of code. :-)

Note: See TracTickets for help on using tickets.