WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 7 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 21 months ago.
wpdb-condense.2.diff (7.6 KB) - added by wonderboymusic 21 months ago.
This is a much better version of this
wpdb-condense.3.diff (10.7 KB) - added by wonderboymusic 19 months ago.
wpdb-condense.4.diff (10.7 KB) - added by atimmer 7 months ago.

Download all attachments as: .zip

Change History (18)

comment:1 wonderboymusic21 months ago

  • Keywords has-patch added; needs-patch removed

comment:2 follow-up: scribu21 months 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 nacin21 months ago

Replying to scribu:

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

I would say absolutely.

wonderboymusic21 months ago

This is a much better version of this

comment:4 nacin19 months 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 wonderboymusic19 months ago

  • Keywords needs-refresh removed

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

comment:6 wonderboymusic19 months ago

New patch with one less method

comment:7 nacin19 months ago

  • Keywords commit added

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

comment:8 nacin18 months ago

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

comment:9 wonderboymusic17 months ago

  • Milestone changed from Future Release to 3.6

comment:10 follow-up: nacin12 months 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 wonderboymusic9 months ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

comment:12 wonderboymusic7 months ago

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

atimmer7 months ago

comment:14 in reply to: ↑ 10 nacin7 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.