WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 5 years ago

Last modified 5 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 7 years ago.
6836.2.diff (2.3 KB) - added by DD32 7 years ago.
changes tested.
6836.3.diff (21.7 KB) - added by DD32 7 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 7 years ago.
remains of wp-admin/
6836.5.diff (8.8 KB) - added by DD32 7 years ago.
/ and /wp-includes/ untested
6836.5-2.diff (9.2 KB) - added by DD32 7 years ago.
6836.full.diff (38.0 KB) - added by DD32 7 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 7 years ago.
wordpress-trunk_20090124_sqlannotations.diff (130.9 KB) - added by noroute 6 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 6 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 6 years ago.
PoC Implementation of an abstraction of DELETE for wp-db.php
6836.6.diff (8.1 KB) - added by dd32 5 years ago.
6836.7.diff (8.1 KB) - added by dd32 5 years ago.

Download all attachments as: .zip

Change History (39)

comment:1 @markjaquith7 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.

comment:2 @DD327 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.

@DD327 years ago

comment:3 @DD327 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.

@DD327 years ago

changes tested.

@DD327 years ago

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

@DD327 years ago

remains of wp-admin/

@DD327 years ago

/ and /wp-includes/ untested

comment:4 @DD327 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

@DD327 years ago

comment:5 @DD327 years ago

  • Keywords has-patch needs-testing added

comment:6 @DD327 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.

@DD327 years ago

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

comment:7 @ryan7 years ago

  • Owner changed from anonymous to ryan

Assigning to me for review and commit.

comment:8 follow-up: @ryan7 years ago

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

comment:9 in reply to: ↑ 8 @DD327 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)

@DD327 years ago

comment:10 @DD327 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]);

@noroute6 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").

@noroute6 years ago

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

@noroute6 years ago

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

comment:11 @ryan6 years ago

  • Milestone changed from 2.9 to 2.8

comment:12 @ryan6 years ago

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

comment:13 @ryan6 years ago

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

comment:14 @ryan6 years ago

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

comment:16 @Denis-de-Bernardy6 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.

comment:17 @ryan6 years ago

  • Milestone changed from 2.8 to 2.9

comment:18 @ryan6 years ago

  • Milestone changed from 2.9 to Future Release

@dd325 years ago

comment:19 @dd325 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

@dd325 years ago

comment:20 @dd325 years ago

attachment 6836.7.diff added

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

comment:21 @ryan5 years ago

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

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

comment:22 @ryan5 years ago

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

comment:23 @hakre5 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?

comment:25 @dd325 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.

comment:26 @hakre5 years ago

Indeed interesting testcase that is.

Note: See TracTickets for help on using tickets.