WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#10864 closed enhancement (fixed)

Add replace method to wpdb

Reported by: aaroncampbell Owned by: ryan
Milestone: 3.0 Priority: normal
Severity: normal Version:
Component: Database Keywords: has-patch needs-testing
Focuses: Cc:

Description

The wpdb class has an insert method, but it would be really nice to have a replace method as well. You can run them directly through the query method, but the added convenience would seem to be worth the minimal additional code.

Attachments (3)

10864.001.2.diff (2.9 KB) - added by aaroncampbell 4 years ago.
10864.001.diff (2.9 KB) - added by aaroncampbell 4 years ago.
10864.002.diff (3.0 KB) - added by aaroncampbell 4 years ago.
With INSERT/REPLACE sanity check

Download all attachments as: .zip

Change History (12)

comment:1 aaroncampbell5 years ago

In the attached patch I created a new method in wpdb called _insert_replace_helper, which is meant to be a private method. Both wpdb::insert and the new wpdb::replace use it. If whomever is in charge prefers it to be done another way, just let me know and I'd be happy to make a new patch.

comment:2 hakre5 years ago

the $type parameter needs to be optional as well (because the one before it is).

is $type validated against all allowed input values?

comment:3 aaroncampbell5 years ago

  • Cc aaroncampbell added

$type isn't validated against allowed values. The main reason is because this should be considered a private method only used by other wpdb methods (Until we move to PHP 5 though, I can't actually make it private). Also __checked_selected_helper, which does the same basic thing, doesn't do any validation.

I suppose it could do some minor validation, but I don't see it being necessary (just my opinion). I doesn't really add any additional security, since anything able to use that method would also be able to use query directly and avoid the validation.

comment:4 westi5 years ago

  • Milestone changed from 2.9 to 3.0

This sounds like a good idea but I don't think it should go into 2.9

Moving to the 3.0 milestone for early consideration.

comment:5 follow-up: Denis-de-Bernardy4 years ago

Suggesting wontfix, personally. If memory serves, REPLACE INTO will change the ID of the original row.

Better use INSERT ... ON DUPLICATE statements where appropriate.

comment:6 in reply to: ↑ 5 aaroncampbell4 years ago

Replying to Denis-de-Bernardy:

Suggesting wontfix, personally. If memory serves, REPLACE INTO will change the ID of the original row.

REPLACE will alter an auto-increment primary key *if* you use a different unique key to specify the row to replace. Most of the time you're using the primary key though, in which case it stays.

Even so, I'm not suggesting that we start using it all over the place, just that we add it to wpdb since I think it's something that's used pretty commonly. It would just make wpdb more flexible with only 6 additional lines of code.

comment:7 hakre4 years ago

Please add a defensive check for the $type parameter to only contain 'INSERT' or 'REPLACE'. Something like:

  if ( ! in_array( $type, array( 'INSERT', 'REPLACE' ) )
    return false;

aaroncampbell4 years ago

aaroncampbell4 years ago

aaroncampbell4 years ago

With INSERT/REPLACE sanity check

comment:8 aaroncampbell4 years ago

10864.001.2.diff was a mistake, so it's the same as 10864.001.diff, which is just a refresh of the original patch to match the current trunk.

I also added 10864.002.diff which includes the sanity checks that hakre wants. I'm still of the opinion that they are pointless. The method should be considered private (even though we can't use the private keyword yet), and only used from wpdb methods.

However, it's not a huge deal either way.

comment:9 nacin4 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [13454]) Add wpdb::replace() for replace queries. props AaronCampbell fixes #10864

Note: See TracTickets for help on using tickets.