Make WordPress Core

Opened 12 years ago

Closed 9 years ago

#19019 closed enhancement (fixed)

Reduce duplication in $wpdb

Reported by: scribu's profile scribu Owned by:
Milestone: 4.2 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 12 years ago.
wpdb-condense.2.diff (7.6 KB) - added by wonderboymusic 12 years ago.
This is a much better version of this
wpdb-condense.3.diff (10.7 KB) - added by wonderboymusic 11 years ago.
wpdb-condense.4.diff (10.7 KB) - added by atimmer 10 years ago.

Download all attachments as: .zip

Change History (19)

#1 @wonderboymusic
12 years ago

  • Keywords has-patch added; needs-patch removed

#2 follow-up: @scribu
12 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.

#3 in reply to: ↑ 2 @nacin
12 years ago

Replying to scribu:

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

I would say absolutely.

@wonderboymusic
12 years ago

This is a much better version of this

#4 @nacin
11 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.

#5 @wonderboymusic
11 years ago

  • Keywords needs-refresh removed

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

#6 @wonderboymusic
11 years ago

New patch with one less method

#7 @nacin
11 years ago

  • Keywords commit added

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

#8 @nacin
11 years ago

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

#9 @wonderboymusic
11 years ago

  • Milestone changed from Future Release to 3.6

#10 follow-up: @nacin
11 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.

#11 @wonderboymusic
11 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

#12 @wonderboymusic
10 years ago

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

#14 in reply to: ↑ 10 @nacin
10 years 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.

#15 @nacin
9 years ago

  • Milestone changed from Future Release to 4.2
  • Resolution set to fixed
  • Status changed from new to closed

This was done in 4.2.

Note: See TracTickets for help on using tickets.