WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 19 months ago

#19019 new enhancement

Reduce duplication in $wpdb

Reported by: scribu Owned by:
Milestone: Future Release Priority: normal
Severity: minor Version:
Component: Database Keywords: needs-refresh 3.7-early
Focuses: Cc:

Description

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.

Attachments (4)

wpdb-condense.diff (4.9 KB) - added by wonderboymusic 3 years ago.
wpdb-condense.2.diff (7.6 KB) - added by wonderboymusic 3 years ago.
This is a much better version of this
wpdb-condense.3.diff (10.7 KB) - added by wonderboymusic 3 years ago.
wpdb-condense.4.diff (10.7 KB) - added by atimmer 20 months ago.

Download all attachments as: .zip

Change History (18)

comment:1 @wonderboymusic3 years ago

  • Keywords has-patch added; needs-patch removed

comment:2 follow-up: @scribu3 years ago

  • Milestone changed from Future Release to 3.5
  • Severity changed from normal to minor

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

comment:3 in reply to: ↑ 2 @nacin3 years ago

Replying to scribu:

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

I would say absolutely.

@wonderboymusic3 years ago

This is a much better version of this

comment:4 @nacin3 years ago

  • Keywords needs-refresh added

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.

comment:5 @wonderboymusic3 years ago

  • Keywords needs-refresh removed

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

comment:6 @wonderboymusic3 years ago

New patch with one less method

comment:7 @nacin3 years ago

  • Keywords commit added

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

comment:8 @nacin2 years ago

  • Keywords 3.6-early added
  • Milestone changed from 3.5 to Future Release

comment:9 @wonderboymusic2 years ago

  • Milestone changed from Future Release to 3.6

comment:10 follow-up: @nacin2 years ago

  • Keywords 3.7-early added; 3.6-early removed
  • Milestone changed from 3.6 to Future Release

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.

comment:11 @wonderboymusic21 months ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

comment:12 @wonderboymusic20 months ago

  • Keywords needs-refresh added; has-patch commit removed

@atimmer20 months ago

comment:14 in reply to: ↑ 10 @nacin19 months ago

  • Milestone changed from 3.7 to Future Release

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.

Note: See TracTickets for help on using tickets.