Add hooks to wpdb's insert(), update(), delete() and similar methods

[borekb]

The wpdb class currently offers a query filter in its query() method which supplies unstructured data to its handlers (a string with an SQL query). In our plugin we need to work with structured data provided to the insert(), update() and similar methods. It would be great if these methods provided hooks both before and after the actual work happens.

For instance, the insert() method would become:

function insert( $table, $data, $format = null ) {

    do_action( 'db_before_insert', $table, $data, $format );
    $result = $this->_insert_replace_helper( $table, $data, $format, 'INSERT' );
    do_action( 'db_after_insert', $table, $data, $format, $result );
        
    return $result;      
}

[johnbillion]

We had a chat about this in IRC prior to borekb opening this ticket. ​Chat log here.

My only concern with the proposed actions/filters is that not everything uses the wpdb::insert() and wpdb::update() methods. If a plugin on your site performs an INSERT or UPDATE using wpdb::query(), then the proposed filters won't see that query. This means, for example, that if you're logging INSERTs using the proposed new filter then you'll also need to use the existing query filter to guarantee you don't miss any INSERTs, thus rendering the proposed filters redundant.

While the filters are a good idea in theory, I don't think they provide any actual benefit. I'm open to being swayed though.

[borekb]

No it is a good point. Our general problem is that we need to work with structured query data and the current 'query' filter makes this task quite hard. Clean WordPress installation strictly uses insert(), update() and similar methods but external plugins might use the generic query() method, that's true.

I'll discuss this further with my colleague to see if this ticket would actually help us or not. Thanks.

[borekb]

Missed something obvious, sorry. The 'query' filter would not work for us because it is fired before the query executes. For the insert scenario, we need to do our work after the query was run.

Let me ask the other way around - what would be the reasons not to include the proposed hooks? One reason that comes to mind is that list of available hooks would grow. Is there anything else I'm possibly missing? These hooks would still greatly help us, even if there might still be situation where we'll need to do something extra.

[nacin]

Replying to borekb:

Let me ask the other way around - what would be the reasons not to include the proposed hooks? One reason that comes to mind is that list of available hooks would grow. Is there anything else I'm possibly missing? These hooks would still greatly help us, even if there might still be situation where we'll need to do something extra.

I have a strong aversion to adding low-level hooks in strange places, and intuition here has paid great dividends down the line as we make further adjustments. Placing filters on queries means we can't tweak a query without possibly breaking filters — this is a nightmare that we continue to deal with in a number of areas. The "dumb" query filter offers absolutely zero expectations for the developer as to what will be received.

These are proposed transactional actions, rather than filters. This is a little better, but still awkward. The actual SQL for a particular query is a terrible way to identify it, as it could easily change in a future version. This means what you can actually do is limited. What do you actually want to do here? Parse the query with regex? That is fraught. It encourages bad developer behavior and is pretty much guaranteed to break in the future.

You'd be better served looking one level up. Nearly all DB queries in WordPress, such as those performing CRUD operations for posts, comments, etc., have transactional hooks both before and after queries. These were mostly added back in 2.9 and 3.0, and happen to be used pretty extensively by VaultPress.

[borekb]

I fully appreciate that going low-level is risky but I guess everyone who handles the 'query' filter or anything that low-level understand what they are doing. For our plugin (VersionPress) there's not much else we can do because while WordPress itself might offer suitable hooks, some complex 3rd party plugins don't and capturing their data on the DB level is the best we can try.

The question is how do we hook into that low level. The 'query' filter runs before the query itself (so we don't know, for example, what was the ID of the inserted entity, and there are other reasons why we don't want our code to run before the database query itself) and is hard to work with anyway (the query is just a string). And after the DB query finishes, I don't think there is a generic action that we could hook into at all, is there? So we ended up creating our own wpdb implementation that is set in the db.php file but that just feels terrible - we are not a db driver, we have conflicts with other plugins that decided to use db.php etc. We'd just like to avoid this if at all possible.

All I'm asking in this ticket is to have some extensibility point for devs like us who need to go low-level. johnbillion had a good point about how all those insert and update hooks might not be enough if something is using the query() method directly so I'm not 100% sure my request should be taken literally. But I'm glad that we're discussing low-level extensibility - even something like a single 'after_query' action would immensely help us.

[borekb]

After discussing this further with my colleague, the 'after_query' action hook seems to be the only crucial thing for us at the moment. The other hooks might be useful in some scenarios but considering some of the points discussed above, the single 'after_query' hook might be the best solution.

At this point I'd like to ask:

1. What do you think about the proposed 'after_query' hook and our arguments for it?
2. If this is something you would consider adding, should I create a new ticket or just update the text of the original suggestion?

Thanks.

[pento]

I'd be more comfortable with an after_query action - it's explicitly read-only, so we won't be encouraging plugins to modify the query on the fly.

If it's at the end of wpdb::query(), any plugin that wants to use it can read the various wpdb members set in query() (insert_id, rows_affected, last_result) to get the information they need, without us needing to pass things in the action that we may want to change at a later date.

[borekb]

Do you guys know how people commonly use the query filter, for what purpose? I tried some code search services and it seems that, first, it is used quite rarely, second, it is usually used to replace the WHERE part of the SELECT query.

I'm wondering all that to find out if we would need access to the query before it was modified by the filter (i.e., as it was passed into the function) or not. If adding the query filter is rare and it only modifies the SELECT queries than we are probably fine with the modified query (the one stored as $wpdb->last_query). Otherwise, it would be useful for us to have access to the original query in some way as it captures the original intent of the caller of the query() method.

[borekb]

Hi guys, is there any chance this would be considered for 4.1? Thanks.

[pento]

I'm still not wild about the after_query action - it feels like a very edge-case action, which usually means it's just going to get in the way as we try to maintain WPDB.

Have you considered creating a db.php drop-in that inherits WPDB? Something like what @johnbillion has done ​here would suit what you're after.

[borekb]

Replying to pento:

Have you considered creating a db.php drop-in that inherits WPDB?

That's what we do at the moment but it's a major problem if the site owner wants to use more db.php plugins. You can basically have only one plugin using db.php so in that sense, db.php is a "bottleneck" while actions / filters are not.

(To be fair to db.php, it's not really its problem; there should be only one DB driver so it's probably OK to allow just one plugin to occupy it. The problem is the lack of other extensibility points in this area, so things that are not DB drivers like various debug plugins or our VersionPress often resort to it just because there's no other easy way. See also the IRC discussion linked in comment 2.)

[ghoush]

Replying to johnbillion:

I would personally really like to see this happen. It's been 6 months since this was last responded to. But I love VersionPress.

It actually surprises me that there isn't an after_query already. That seems pretty useful. Putting your own db.php together is just a recipe for collisions with other plugins who do the same.

[karlbecker]

Another vote here, since this is important to improving version control on wordpress (specifically, ​http://versionpress.net , but seems useful in general for this goal).

To allow wordpress to be a viable platform to integrate with developers who rely on version control for various systems, version control for Wordpress needs to be well-supported.

[SergeyBiryukov]

Replying to borekb:

(To be fair to db.php, it's not really its problem; there should be only one DB driver so it's probably OK to allow just one plugin to occupy it. The problem is the lack of other extensibility points in this area, so things that are not DB drivers like various debug plugins or our VersionPress often resort to it just because there's no other easy way. See also the IRC discussion linked in comment 2.)

This works for me in a plugin without using the db.php drop-in:

class wpdb_after_query extends wpdb {

	function query( $query ) {
		$return_val = parent::query( $query );

		if ( false !== $return_val ) {
			/**
			 * Fires after the query is executed.
			 *
			 * @param string $query Database query.
			 * @param wpdb   $wpdb  wpdb instance.
			 */
			do_action( 'after_query', $query, $this );
		}

		return $return_val;
	}

}

function wp29710_replace_wpdb() {
	global $wpdb;

	$wpdb = new wpdb_after_query( DB_USER, DB_PASSWORD, DB_NAME, DB_HOST );

	wp_set_wpdb_vars();
}
add_action( 'plugins_loaded', 'wp29710_replace_wpdb' );

[borekb]

Replying to SergeyBiryukov:

That would still allow only one plugin / drop-in to hook into the query if I understand it correctly. We tried to take your idea a bit further and replace inheritance with composition by creating a proxy class and replacing $wpdb with that. It worked but there were still some issues with that approach:

• If the proxy was created manually, it was quite a bit of work
• New releases of WordPress might break the proxy, e.g., if there are new public members in wpdb
• The proxy can be generated on-the-fly using a library like ​ProxyManager but that has its own set of issues:
  • The external dependency is quite large (look at the source code of ProxyManager)
  • It is PHP 5.5+ only and there's not a lot of choice amongst those libraries

New built-in hooks / filters still seem like a vastly preferable solution.

[SergeyBiryukov]

Replying to borekb:

That would still allow only one plugin / drop-in to hook into the query if I understand it correctly.

I think my point was that it's still possible to hook into the query even if there's a drop-in from another plugin in use (unless I'm missing something).

[SergeyBiryukov]

It might be possible to create several classes extending most popular drop-ins, and then instantiate one depending on the drop-in currently in use.

Otherwise, you're right, the idea isn't that useful.

[borekb]

Replying to SergeyBiryukov:

I think my point was that it's still possible to hook into the query even if there's a drop-in from another plugin in use (unless I'm missing something).

There are ways and we've actually played with three different working solutions already, however, they are all pretty ugly or at least inconvenient. To the extent that plugin authors typically don't bother with them and just use the DB drop-in, creating the aforementioned bottleneck.

With that being said, I understand that this issue might be perceived as relatively unimportant to some, and as we absolutely needed a fix for VersionPress we found one. If there's not enough demand from other plugin authors and/or WP users who run into issues because of conflicting plugins, I'm not going to push this further.

[dd32]

Replying to borekb:

That would still allow only one plugin / drop-in to hook into the query if I understand it correctly. We tried to take your idea a bit further and replace inheritance with composition by creating a proxy class and replacing $wpdb with that. It worked but there were still some issues with that approach:

I would do exactly that, I don't realistically think these hooks this low are useful to most people (And won't really work with DB dropins - some of them may need to use their own method), any changes to items should be hookable higher up - and if they're not, tickets should be opened to add hooks to those locations.

wpdb-injector.php​ is a example of a magic-based class that can wrap $wpdb no matter what dropin is in use, transparently acting as a proxy, any future methods or args added just work, and well.. it works (I just did it as a mu-plugin, but it'll just as happily work in a regular plugin).
Other plugins could come along and register the exact same type of class (with a different classname..) and both plugins would happily co-exist, one plugin proxying the other plugin which proxies the DB connection.
This only intercepts direct function calls to $wpdb, it doesn't catch internal $wpdb calls (ie. update() calling prepare() / query()

[borekb]

Replying to dd32:

That will work until some plugin uses type hinting / checking on $wpdb. Not sure how common that is but the whole site will break then.

[dd32]

Replying to borekb:

Replying to dd32:

That will work until some plugin uses type hinting / checking on $wpdb. Not sure how common that is but the whole site will break then.

I personally don't expect any plugins/sites to ever do that, for the simple sense that db dropins don't have to extend wpdb (although it's encouraged). If they're acting that strict, chances are it'll break on plenty of other common plugin methods used.

[borekb]

Replying to dd32:

db dropins don't have to extend wpdb (although it's encouraged)

Interesting, I didn't know that. Do you know of examples of db drop-ins that do not extend wpdb? And is this documented somewhere? (Just curious.)

Overall, if the Adapter pattern can be used without issues then this ticket can probably be closed as it's a good enough solution. Thank you all for your input and suggestions.

[johnbillion]

Wontfixing as per discussion above.