Make WordPress Core

Opened 17 years ago

Closed 15 years ago

Last modified 15 years ago

#6836 closed defect (bug) (fixed)

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

Reported by: dd32's profile DD32 Owned by: ryan's profile 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 17 years ago.
6836.2.diff (2.3 KB) - added by DD32 17 years ago.
changes tested.
6836.3.diff (21.7 KB) - added by DD32 17 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 17 years ago.
remains of wp-admin/
6836.5.diff (8.8 KB) - added by DD32 17 years ago.
/ and /wp-includes/ untested
6836.5-2.diff (9.2 KB) - added by DD32 17 years ago.
6836.full.diff (38.0 KB) - added by DD32 16 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 16 years ago.
wordpress-trunk_20090124_sqlannotations.diff (130.9 KB) - added by noroute 16 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 16 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 16 years ago.
PoC Implementation of an abstraction of DELETE for wp-db.php
6836.6.diff (8.1 KB) - added by dd32 15 years ago.
6836.7.diff (8.1 KB) - added by dd32 15 years ago.

Download all attachments as: .zip

Change History (39)

#1 @markjaquith
17 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
17 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
17 years ago

#3 @DD32
17 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
17 years ago

changes tested.

@DD32
17 years ago

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

@DD32
17 years ago

remains of wp-admin/

@DD32
17 years ago

/ and /wp-includes/ untested

#4 @DD32
17 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
17 years ago

#5 @DD32
16 years ago

  • Keywords has-patch needs-testing added

#6 @DD32
16 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
16 years ago

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

#7 @ryan
16 years ago

  • Owner changed from anonymous to ryan

Assigning to me for review and commit.

#8 follow-up: @ryan
16 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
16 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
16 years ago

#10 @DD32
16 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
16 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
16 years ago

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

@noroute
16 years ago

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

#11 @ryan
16 years ago

  • Milestone changed from 2.9 to 2.8

#12 @ryan
16 years ago

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

#13 @ryan
16 years ago

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

#14 @ryan
16 years ago

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

#16 @Denis-de-Bernardy
15 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
15 years ago

  • Milestone changed from 2.8 to 2.9

#18 @ryan
15 years ago

  • Milestone changed from 2.9 to Future Release

@dd32
15 years ago

#19 @dd32
15 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
15 years ago

#20 @dd32
15 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
15 years ago

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

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

#22 @ryan
15 years ago

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

#23 @hakre
15 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
15 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
15 years ago

Indeed interesting testcase that is.

Note: See TracTickets for help on using tickets.