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

[DD32]

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.

[markjaquith]

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.

[DD32]

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]

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]

changes tested.

[DD32]

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

[DD32]

remains of wp-admin/

[DD32]

/ and /wp-includes/ untested

[DD32]

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]

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]

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

[ryan]

Assigning to me for review and commit.

[ryan]

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

[DD32]

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]

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]

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]

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

[noroute]

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

[ryan]

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

[ryan]

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

[ryan]

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

[Denis-de-Bernardy]

fixed?

[Denis-de-Bernardy]

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

[dd32]

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]

attachment 6836.7.diff added

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

[ryan]

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

[ryan]

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

[hakre]

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?

[hakre]

Those from ​wp-admin/includes/upgrade.php (6836.3.diff )?

[dd32]

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.

[hakre]

Indeed interesting testcase that is.