WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 12 months ago

#12819 reopened enhancement

wpdb::prepare support for null

Reported by: roxaz Owned by: ryan
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Database Keywords: needs-patch
Focuses: Cc:

Description

now we can not submit null values using wpdb::prepare. if we lets say have datetime field that can be null, and execute query prepared with $wpdb->prepare("update table set date_time_field = %s", null) - it sets value of the field to 0, which results in 0000-00-00 00:00:00 date. what is happening here - null is quoted and passed as a string, that should not be the case.

Change History (12)

comment:1 scribu4 years ago

  • Component changed from General to Database
  • Keywords needs-patch added
  • Milestone changed from Unassigned to 3.1
  • Owner set to ryan
  • Type changed from defect (bug) to enhancement

Makes sense.

comment:2 miqrogroove4 years ago

  • Keywords needs-patch removed
  • Milestone 3.1 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

OP is based on a misunderstanding of wpdb->prepare(), which is not intended to handle data types other than strings and numbers.

See also #11622

comment:3 scribu4 years ago

  • Keywords dev-feedback added
  • Milestone set to Future Release
  • Resolution wontfix deleted
  • Status changed from closed to reopened

The fact that it currently only supports strings and numbers doesn't inherently mean it shouldn't also support null values. From the doc:

Prepares a SQL query for safe execution. Uses sprintf()-like syntax.

comment:4 miqrogroove4 years ago

scribu, you are preaching to the choir. prepare() got beat to death during 3.0 alpha and the core devs do not want to change it while PHP 4 is still supported. This ticket duplicates #11622 and should not be left open.

comment:5 scribu4 years ago

  • Keywords dev-feedback removed
  • Milestone Future Release deleted
  • Resolution set to duplicate
  • Status changed from reopened to closed

Yeah, I guess this is just one of the enhacements that #11622 would cover.

comment:6 mark-k16 months ago

  • Resolution duplicate deleted
  • Status changed from closed to reopened

Just ran into a related question on stackexchange http://wordpress.stackexchange.com/questions/76742/how-to-pass-null-in-where-array-for-wpdb-update.

IMHO even if there will be a move to PDO in 3.6, especially if the PDO based DB class will solve this issue, it is time to fix it in what will be the legacy code. It will be bad if PDO code is not fully compatible with legacy code.

comment:7 SergeyBiryukov16 months ago

  • Milestone set to Awaiting Review

comment:8 nacin16 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

Seems like this would fix #15158. I agree with re-opening this.

comment:9 follow-up: jbutkus16 months ago

  • Cc butkus.justas@… added

What would you think, on using two-step aproach?

  1. Iterate over all values (args) and replace PHP NULL values with generated random hashes (at least 264 in complexity) - remembering all replacements;
  1. Perform current vsprintf operation and replace former NULLs:

2.1. First by unquoting possibly quoted values (as NULL may be textual or numeric column): ~= s/'$hash'/$hash/g) ,

2.2. Then by actually replacing them with NULLs: ~= s/$hash/NULL/g).

Why I propose replacement-based solution is that other way round would be implementing fully-fledged string parsing, to decode positional information, and one would have to take into account all sorts of possibilies (i.e. INSERT INTO foo ( col1, col2, col3 ) VALUES ( %2$s, %3$d, %s ) ON DUPLICATE KEY UPDATE SET col3 = %3$s, col2 = %s ).

With replacement the only side effect may be replacing some actual value for NULL. What is the possibility with key of 264? We can easily minify the possibility, by increasing it to 2256, as hashing functions are fast enough, and simply concatenating output of several of these may resolve most of errors.

Of course, parsing string would be better, but it is way more complicated and more errors may creep-in.

I may opt-in to provide sample solution. Just that some thoughts on this are more than welcome.

comment:10 in reply to: ↑ 9 ; follow-up: miqrogroove16 months ago

Replying to jbutkus:

Why I propose replacement-based solution is that other way round would be implementing fully-fledged string parsing, to decode positional information, and one would have to take into account all sorts of possibilies (i.e. INSERT INTO foo ( col1, col2, col3 ) VALUES ( %2$s, %3$d, %s ) ON DUPLICATE KEY UPDATE SET col3 = %3$s, col2 = %s ).

That syntax is neither used nor supported by wpdb::prepare, thankfully. What we're discussing is a simple matter of replacing the three supported placeholders with a literal NULL. The only syntactical complication is that this operation must avoid the regexp quoting of strings for the null args.

Random hashes are not needed here, because in a worst-case replacement strategy it would be adequate to use a static placeholder string that is invalid in SQL.

comment:11 in reply to: ↑ 10 jbutkus16 months ago

Replying to miqrogroove:

That syntax is neither used nor supported by wpdb::prepare, thankfully. What we're discussing is a simple matter of replacing the three supported placeholders with a literal NULL. The only syntactical complication is that this operation must avoid the regexp quoting of strings for the null args.

Well, I may have missed the documentation part on this one, but, actually, it works and gives expectable results:

$wpdb->prepare(
    '
        INSERT INTO foo ( val1, val2, val3 )
        VALUES ( x\'%2$s\', %3$d, \'%1$s\' )
        ON DUPLICATE KEY UPDATE SET val2 = %3$d
    ',
    'foo-\'bar\'-baz',
    'ae4f',
    12345
);

gives

INSERT INTO foo ( val1, val2, val3 )
VALUES ( x'ae4f', 12345, 'foo-\'bar\'-baz' )
ON DUPLICATE KEY UPDATE SET val2 = 12345

Of course, the wpdb::prepare() does not support quoting of positionally-referenced strings, thus one has to add quotes on it's own.

Replying to miqrogroove:

Random hashes are not needed here, because in a worst-case replacement strategy it would be adequate to use a static placeholder string that is invalid in SQL.

It is hard to say, what is "invalid in SQL".

What if I would write such statement:

$wpdb->prepare(
    'INSERT INTO foo ( val1, val2 ) VALUES ( %s, %s )',
    'this is `"invalid"` SQL `DROP TABLE students` statement',
    NULL
);

What value would you have to choose, as invalid SQL, which would correctly match only actual NULLs, and not anything else, that user may input?

The key may not be purely random - it may be fixed for every replacement (i.e. 1kb long string based on curent NONCE_SALT, etc.).
Just that I would think about it being "long", more than "invalid in SQL". And by long - anything, that might only be generated as correct input to statement, as 1 in 100 billions, or something. I.e. more likely user must target that intentionally, than have it as a valid input (by writing documentation, for example).

Of course, a patch may include modifying "escape_by_ref", to NOT target specific values, i.e. "this is 'NULL' placeholder". But that may cause an error, if one is to write just this value.

comment:12 netweblogic12 months ago

  • Cc msykes@… added
Note: See TracTickets for help on using tickets.