WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#6836 closed defect (bug) (fixed)

Prepare queries using INSERT/UPDATE should use wpdb::insert() or wpdb::update()

Reported by: DD32 Owned by: ryan
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.6
Component: Administration Keywords: has-patch
Focuses: Cc:

Description

Recently some commits were made to introduce more prepared queries, eg: [7645](However theres probably more in the last 7 months). There are also areas where the insert/update raw SQL's have not been moved over yet.

See [6221] and [6238] for the renamed areas.

Attachments (13)

6836.diff (3.3 KB) - added by DD32 8 years ago.
6836.2.diff (2.3 KB) - added by DD32 8 years ago.
changes tested.
6836.3.diff (21.7 KB) - added by DD32 8 years ago.
Tested with a New Install only; Upgrade functions not tested, Could do with an extra eye-over
6836.4.diff (2.5 KB) - added by DD32 8 years ago.
remains of wp-admin/
6836.5.diff (8.8 KB) - added by DD32 8 years ago.
/ and /wp-includes/ untested
6836.5-2.diff (9.2 KB) - added by DD32 8 years ago.
6836.full.diff (38.0 KB) - added by DD32 8 years ago.
also includes a logic inversion in the original patch for wp-includes/comment.php
insert.type.diff (1.1 KB) - added by DD32 8 years ago.
wordpress-trunk_20090124_sqlannotations.diff (130.9 KB) - added by noroute 8 years ago.
My Patch to marks/annotates all uses of inline SQL outside of wp-db. This patch includes the cases where a raw SQL update/insert is used instead of the existing methods in wp-db (search for "@RawSQLUse, method_exists"). Also see the discussion on wp-hackers (search for "Making WP more secure the evolutionary way").
wordpress_sqlannotations_simple.diff (76.5 KB) - added by noroute 8 years ago.
Simple annotation version to mark all the remaining raw uses of INSERT/UPDATE (+simple cases of DELETE and SELECT)
wordpress_delete_wpdb.diff (1.9 KB) - added by noroute 8 years ago.
PoC Implementation of an abstraction of DELETE for wp-db.php
6836.6.diff (8.1 KB) - added by dd32 7 years ago.
6836.7.diff (8.1 KB) - added by dd32 7 years ago.

Download all attachments as: .zip

Change History (39)

#1 @markjaquith
8 years ago

Cheers! I think the best way to do it is one file at a time. That way a lot of people can participate without having to all share one big monster diff.

#2 @DD32
8 years ago

I think the best way to do it is one file at a time.

Yep, I agree with that. I might make a start over the weekend on some of them.

@DD32
8 years ago

#3 @DD32
8 years ago

attachment 6836.diff added.

Note: There was a escaping issue with adding meta/updating meta, ' and " were being double escaped. Patch as such removes the double escpaping.

Also renamed a few variables so as to use compact('meta_value') instead of array('meta_value' => $metavalue,..). Just makes it look a bit more clean and keeps fields named the same throughout.

All Meta changes were tested working, The last chunk(Change to _relocate_children()) hasnt been tested, But i see no reason it should fail.

@DD32
8 years ago

changes tested.

@DD32
8 years ago

Tested with a New Install only; Upgrade functions not tested, Could do with an extra eye-over

@DD32
8 years ago

remains of wp-admin/

@DD32
8 years ago

/ and /wp-includes/ untested

#4 @DD32
8 years ago

Just noticed this: http://trac.wordpress.org/browser/trunk/wp-includes/functions.php#L527

532: $type = $wpdb->escape( $headers['content-type'] );
}}

I assume it was probably left over from when the raw query was there.. removed it in 6836.5-2.diff

@DD32
8 years ago

#5 @DD32
8 years ago

  • Keywords has-patch needs-testing added

#6 @DD32
8 years ago

About to add a full refreshed patch.

I've been running my local install patched like this for awhile, and not had any issues with escaped data.

@DD32
8 years ago

also includes a logic inversion in the original patch for wp-includes/comment.php

#7 @ryan
8 years ago

  • Owner changed from anonymous to ryan

Assigning to me for review and commit.

#8 follow-up: @ryan
8 years ago

insert() and update() don't do prepare(). I would like to pass format specifiers to them somehow before moving everything over.

#9 in reply to: ↑ 8 @DD32
8 years ago

Replying to ryan:

insert() and update() don't do prepare(). I would like to pass format specifiers to them somehow before moving everything over.

How about something similar to this:

insert ('test_table', array('ID' => 's', 'post' => 'test'), array('ID' => '%d'));
creating:
INSERT INTO test_table (`ID`,`post`) VALUES ('0','test')

(See patch)

@DD32
8 years ago

#10 @DD32
8 years ago

that could be changed to this to make it more compact:

foreach( (array)$types as $key => $type )
	if( isset($data[$key]) )
		$data[$key] = sprintf($type, $data[$key]);

@noroute
8 years ago

My Patch to marks/annotates all uses of inline SQL outside of wp-db. This patch includes the cases where a raw SQL update/insert is used instead of the existing methods in wp-db (search for "@RawSQLUse, method_exists"). Also see the discussion on wp-hackers (search for "Making WP more secure the evolutionary way").

@noroute
8 years ago

Simple annotation version to mark all the remaining raw uses of INSERT/UPDATE (+simple cases of DELETE and SELECT)

@noroute
8 years ago

PoC Implementation of an abstraction of DELETE for wp-db.php

#11 @ryan
7 years ago

  • Milestone changed from 2.9 to 2.8

#12 @ryan
7 years ago

(In [10730]) Use wpdb::insert() and update(). Props DD32. see #6836

#13 @ryan
7 years ago

Not everything from the patch applied cleanly, but most of it did.

#14 @ryan
7 years ago

(In [10731]) Use wpdb::insert() and update(). see #6836

#16 @Denis-de-Bernardy
7 years ago

  • Keywords needs-patch added; has-patch needs-testing removed

guess not. there are still a few $wpdb->query("INSERT ... and $wpdb->query("UPDATE ... around.

#17 @ryan
7 years ago

  • Milestone changed from 2.8 to 2.9

#18 @ryan
7 years ago

  • Milestone changed from 2.9 to Future Release

@dd32
7 years ago

#19 @dd32
7 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 3.0

attachment 6836.6.diff added

Adds the final few UPDATE/INSERT conversions that are not using complex SQL.

Most of the last areas that use it is wp-admin/includes/upgrade.php which i've been avoiding as i'm not going to be able to test the changes properly there.. So i'm tempted to leave that code be for now while its in working condition, Its prepare()'d, just not update()'d

@dd32
7 years ago

#20 @dd32
7 years ago

attachment 6836.7.diff added

  • Removed a useless => %d specification on a query
    • $wpdb->field_types takes care of those fields.

#21 @ryan
7 years ago

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

(In [12652]) Use update and insert. Props DD32. fixes #6836

#22 @ryan
7 years ago

Let's skip the ones in upgrade. They're fine and retesting them would be a pain.

#23 @hakre
7 years ago

I don't get ryan's last comment. Does it mean this ticket is partially fixed and some queries in upgrade related functions were not changed? If so, which were they?

#25 @dd32
7 years ago

Does it mean this ticket is partially fixed and some queries in upgrade related functions were not changed? If so, which were they?

It means any changes to the upgrade scripts in the supplied patch were not commited, And will not be commited.

The only reason is that testing DB changes made to the upgrade scripts is a serious undertaking, Its hard to test every case of every SQL branch, the current implementation is working and uses proper escaping, so its best left running as-is.

#26 @hakre
7 years ago

Indeed interesting testcase that is.

Note: See TracTickets for help on using tickets.