Reduce duplication in $wpdb

[scribu]

Currently, update(), insert() and soon delete() [see #18948] use the same code to generate the WHERE clause.

It should be moved into a helper method.

[scribu]

I'm wondering if we should mark these new methods with an actual "protected" keyword.

[nacin]

Replying to scribu:

I'm wondering if we should mark these new methods with an actual "protected" keyword.

I would say absolutely.

[wonderboymusic]

This is a much better version of this

[nacin]

Patch applies fine (aside from load.php, which was committed elsewhere).

The by-reference $format so we may unshift it seems a bit odd. I understand why it was done, though. Looks like some docs were updated to $formats — arguments and variables should be updated just the same, in that case.

Avoiding the table clashes is a noble idea, but this isn't foolproof. It only checks the tables property, while there are also global and multisite global tables. If we check against both $wpdb->tables( 'all' ) and $wpdb->tables( 'ms_global' ) ), that seems like a lot of unnecessary checking. Not terrible, but it also wouldn't catch queries done against WP tables with another prefix. I don't think that is something core does these days (we opt to switch_to_blog() instead) but something to think about.

I'd rather leave that out for now and revisit it in its own ticket.

[wonderboymusic]

Refreshed the patch, gets rid of tons of code - I ran Unit Tests on every change.

[wonderboymusic]

New patch with one less method

[nacin]

Looks great. Will want to do a closer inspection to make sure it doesn't break anything.

[nacin]

I think this is slightly over-abstracted. _insert_replace_helper is the only situation where we need values only. Let's keep that logic there, then have the new helper function drop the $values aspect. This would still then abstract out update() and delete().

An alternative would be for the helper to return an array of formatted values, keyed by field. Then the formats stuff is abstracted, and each function just do a foreach with $field => $value. It might be ugly when put together, but it's an idea.

[wonderboymusic]

these are all marked 3.7-early

[atimmer]

Refreshed ​wpdb-condense.3.diff.

[nacin]

Replying to nacin:

I think this is slightly over-abstracted. _insert_replace_helper is the only situation where we need values only. Let's keep that logic there, then have the new helper function drop the $values aspect. This would still then abstract out update() and delete().

An alternative would be for the helper to return an array of formatted values, keyed by field. Then the formats stuff is abstracted, and each function just do a foreach with $field => $value. It might be ugly when put together, but it's an idea.

[nacin]

This was done in 4.2.