WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 7 weeks ago

#15158 reopened enhancement

wpdb insert & update with null values

Reported by: westi Owned by: sorich87
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0.1
Component: Database Keywords: needs-patch
Focuses: Cc:

Description

From: http://wordpress.org/support/topic/bug-fix-wpdb-insert-amp-update-with-null-values

"Some of you might have noticed but the wpdb's insert & update methods don't work well with null value (if you try to set null on a numeric field you'll end up with 0)"

Attachments (5)

15158.diff (2.0 KB) - added by sorich87 3 years ago.
wpdb_prepare_null.15158.patch (1.5 KB) - added by jbutkus 16 months ago.
Patching wpdb::prepare() to support NULL handling
wpdb_prepare_null.15158.v2.patch (1.5 KB) - added by jbutkus 16 months ago.
Fixed patch for NULLs support in wpdb::prepare()
wpdb_prepare_null.15158.v3.patch (1.7 KB) - added by jbutkus 16 months ago.
Fixed patch for NULLs support in wpdb::prepare(), handling arguments count being lower than expected
wpdb_prepare_null.15158.v4.patch (1.7 KB) - added by jbutkus 16 months ago.
More memory-concious approach towards NULL support in wpdb::prepare()

Download all attachments as: .zip

Change History (45)

comment:1 Denis-de-Bernardy4 years ago

  • Cc Denis-de-Bernardy added

comment:2 hakre4 years ago

I don't think that wpdb->prepare is designed to handle NULL values for numeric fields. Probably that's the cause.

comment:3 jane3 years ago

If someone wants to get this in for 3.1, please post a patch now for community review, as freeze is right around the corner.

sorich873 years ago

comment:4 sorich873 years ago

  • Keywords has-patch added; needs-patch removed
  • Owner set to sorich87
  • Status changed from new to accepted

comment:5 westi3 years ago

  • Keywords needs-testing 2nd-opinion added

Patch looks reasonable.
Have you got some example code which relies on this feature to test with?

comment:6 follow-up: nacin3 years ago

Code appears to act only on the value being === null. If someone is currently passing a value that may end up being null, but is actually passing this to %s or with the intention of it being handled by a zero-length string, then we'll get an off-by-one error. It's also difficult to follow -- the arrays for fields/values and formats should ideally be easier to match up from a code review perspective.

This needs to be explicit, i.e. array( 'field1' => '1', 'field2' => null, 'field3' => 3 ), array( '%s', null, '%d' ).

Both NULL and 'null' as a case-insensitive string should probably be accepted as a format.

comment:7 nacin3 years ago

  • Keywords needs-patch added; has-patch needs-testing 2nd-opinion removed
  • Type changed from defect (bug) to enhancement

comment:8 westi3 years ago

  • Milestone changed from 3.1 to Future Release

Enhancement moving to Future Release - we are focused on bug fixes for 3.1 now

comment:9 TechDagan2 years ago

  • Cc dagan.henderson@… added

Where do we stand with this? As far as I can tell, it wasn't added to 3.2 and we're now in the 3.3 beta.

comment:10 eddieringle2 years ago

  • Cc eddie@… added

comment:11 cyberhobo2 years ago

  • Cc cyberhobo@… added

comment:12 deltafactory2 years ago

  • Cc deltafactory added

comment:13 andkrup2 years ago

  • Cc andkrup added

comment:14 badconker23 months ago

  • Cc aurelien.joahny@… added

comment:15 jbutkus18 months ago

  • Cc butkus.justas@… added

What are the plans, for implementing fully-boiled NULL handling?

comment:16 nacin16 months ago

Seems like a patch is needed to add null support to prepare(), see #12819.

comment:17 in reply to: ↑ 6 miqrogroove16 months ago

Replying to nacin:

Both NULL and 'null' as a case-insensitive string should probably be accepted as a format.

Accepting 'null' as a string equivalent of NULL will break every possible usage of that phrase within WordPress. Are you prepared to deal with that?

comment:18 follow-up: miqrogroove16 months ago

I would also like to remind everyone that NULL is often invalid in MySQL, e.g.

"To search for column values that are NULL, you cannot use an expr = NULL test ... because expr = NULL is never true for any expression" -- MySQL refman

comment:19 in reply to: ↑ 18 ; follow-up: jbutkus16 months ago

Replying to miqrogroove:

I would also like to remind everyone that NULL is often invalid in MySQL, e.g.

"To search for column values that are NULL, you cannot use an expr = NULL test ... because expr = NULL is never true for any expression" -- MySQL refman

The NULL is not invalid.
It is just that there is special operation for NULL checking IS NULL, instead of = NULL.

That is just to be expected, as field value is not NULL. The NULL is just an indicator, that field does not have any value.

But I fully agree with comment 18 by miqrogroove, where 'null' (string) usage in favor of NULL (literary PHP NULL value) is questioned. It would raise much more questions and issues, than come of use to developers.

And if nacin was talking about format string purely (i.e. use 'null' instead of '%s') - I can hardly understand the purpose of this. If I would like to use NULL for field, I would just skip that field from SQL statement, instead of adding 'null' format mask.

comment:20 in reply to: ↑ 19 ; follow-up: TechDagan16 months ago

Replying to jbutkus:

If I would like to use NULL for field, I would just skip that field from SQL statement, instead of adding 'null' format mask.

In the case of an INSERT, that would work fine. But a field cannot be updated to become NULL, only to contain an empty string or, in the case of an integer, 0. Boolean fields are completely unmanageable.

I can see why the original version(s) of wpdb did not bother to include null-parsing logic, but it's not really that complicated to do. I'll take a look at the code and, time willing, see about submitting a patch.

comment:21 in reply to: ↑ 20 jbutkus16 months ago

Replying to TechDagan:

In the case of an INSERT, that would work fine. But a field cannot be updated to become NULL, only to contain an empty string or, in the case of an integer, 0. Boolean fields are completely unmanageable.

I would go for one of:

  • do not provide a field (in format, or values list) if you don't want it to be touched;
  • provide field with it's indended format (be it %s, or %d, etc.) and a value of NULL, in case you want to INSERT/UPDATE field with NULL.

comment:22 jbutkus16 months ago

  • Keywords has-patch added; needs-patch removed

Please find my patch attached.

I have tested it to work with such cases:

Test1:

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

gives:

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


Test2:

$wpdb->prepare( 'UPDATE foo SET val1 = %s, val2 = %d, val3 = %f, val4 = %s', 'foo-\'bar\'-baz', NULL, 2013.12, 'Happy Holidays' );

gives:

UPDATE foo SET val1 = 'foo-\'bar\'-baz', val2 = NULL , val3 = 2013.120000, val4 = 'Happy Holidays'


Test3:

$wpdb->prepare( 'INSERT INTO foo ( val1, val2, val3, val4 ) VALUES ( %s, %d, %s, %s ) ', 'My First WP post', '1010', NULL, 'Hello, World!' );

gives:

INSERT INTO foo ( val1, val2, val3, val4 ) VALUES ( 'My First WP post', 1010, NULL , 'Hello, World!' )

I see this as one of the possibilities.

jbutkus16 months ago

Patching wpdb::prepare() to support NULL handling

comment:23 follow-up: miqrogroove16 months ago

Here are a couple of test cases to break your patch. If you can get the bugs worked out, I'll take another look at it. :)

Patch breaks the last arg:

prepare( 'INSERT INTO foo ( val1, val2, val3, val4, val5 ) VALUES ("\'%%s\' is where strings go",%s,%d,%s,%s) ', 'My First WP post', '1010', NULL, 'Hello, World!' );

Patch returns bool(false) and throws a PHP Warning:

prepare( 'INSERT INTO foo ( val1, val2, val3, val4, val5 ) VALUES ("\'%%s\' is where strings go",%s,%s,%d,%s) ', NULL, 'My First WP post', '1010', 'Hello, World!' );

comment:24 miqrogroove16 months ago

There is also a case where too few arguments are passed to prepare() and it is supposed to be recognized as an error. You can fix that by checking is_null(), which is currently missing in the patch.

jbutkus16 months ago

Fixed patch for NULLs support in wpdb::prepare()

comment:25 in reply to: ↑ 23 ; follow-up: jbutkus16 months ago

Replying to miqrogroove:

Here are a couple of test cases to break your patch. If you can get the bugs worked out, I'll take another look at it. :)

Thank you for pointing these out. :-)
That was rather trivial to get (solved in regexp), but would have bad effects.

Please see new attachment for details.

And maybe you could explain, what would you check via is_null for not being present? To check what vsprintf currently checks for?

comment:26 in reply to: ↑ 25 ; follow-up: miqrogroove16 months ago

Replying to jbutkus:

And maybe you could explain, what would you check via is_null for not being present? To check what vsprintf currently checks for?

I suggest developing some test cases that have arguments missing so that you can see the patch causes prepare() to return a value, which it should not do.

comment:27 follow-up: deltafactory16 months ago

While I support the addition of NULL support, I wonder if it would break some plugins that rely on wpdb::prepare() to convert null to zero or an empty string. Would it make sense to support nulls when explicitly enabled, for example:

$wpdb->enable_nulls = true;

jbutkus16 months ago

Fixed patch for NULLs support in wpdb::prepare(), handling arguments count being lower than expected

comment:28 in reply to: ↑ 26 ; follow-up: jbutkus16 months ago

Replying to miqrogroove:

I suggest developing some test cases that have arguments missing so that you can see the patch causes prepare() to return a value, which it should not do.

I would vote for adding it in a different commit, as it is a separate feature (which was not checked for in the past), but here is my attempt upon this question.

It sounds logical to resolve it within this very same issue, as now it is easy to add.

comment:29 in reply to: ↑ 27 ; follow-up: jbutkus16 months ago

Replying to deltafactory:

While I support the addition of NULL support, I wonder if it would break some plugins that rely on wpdb::prepare() to convert null to zero or an empty string. Would it make sense to support nulls when explicitly enabled, for example:

$wpdb->enable_nulls = true;

I think that ($wpdb->enable_nulls = true;) would introduce more trouble, than anything else. As somebody would set it, and then forget to reset, while others would expect it to be in one position.

Either it must be another method, that would allow something like this (i.e. create method wpdb::prepare_with_nulls( x, y )), or just attempt to ignore.

How a developer might expect this to be true, when there is no use for NULL values, if wpdb is used regularly, so far?

Albeit it is hard to guess, what a developer might expect.

I, for once, expect wpdb::prepare( x, y ) to work correctly with positional arguments ('INSERT INTO foo SET bar = %2$s, baz = %d'), despite the fact, that it is clearly stated otherwise. And I think there are people who use this, thus I tried to add support for such cases in NULL checking.

So, yes, this is an oldish ticket, which needs some political resolution, I suspect.

comment:30 in reply to: ↑ 29 deltafactory16 months ago

Replying to jbutkus:

I think that ($wpdb->enable_nulls = true;) would introduce more trouble, than anything else. As somebody would set it, and then forget to reset, while others would expect it to be in one position.

Of course, you're right. My general idea was this: At this point, developers should be required to intentionally choose to use nulls since every bit of code out there was written with the understanding that a null couldn't be inserted. As you help point out, if it's dangerous to enable it within a single page load, it would be disastrous to make the global default.

comment:31 in reply to: ↑ 28 miqrogroove16 months ago

Replying to jbutkus:

I would vote for adding it in a different commit, as it is a separate feature (which was not checked for in the past), but here is my attempt upon this question.

I took a closer look at this for you. I suggest going back to v2.patch and change this line:

if ( ! isset( $values[$loc_index] ) ) {

... with my suggested code ...

if ( $loc_index < count($values) and is_null( $values[$loc_index] ) ) {

... now there is no need for the extra variables.

comment:32 miqrogroove16 months ago

Disregard previous comment. The call to unset() messes up the array count.

comment:33 miqrogroove16 months ago

Instead of calling unset(), you could reset $args=array(); at the top of your first if block and then foreach { $args[] = $values[$loc_index] }

Otherwise, $args and $values have the same information being duplicated unnecessarily anyway.

comment:34 follow-up: miqrogroove16 months ago

I think a better strategy would be to run two separate loops: foreach($args) to create a list of which args were NULL and unset them, followed by foreach($positions[0]) to modify the query string.

This strategy would avoid duplicating the $args array, which is potentially extremely expensive when working with large queries.

Last edited 16 months ago by miqrogroove (previous) (diff)

jbutkus16 months ago

More memory-concious approach towards NULL support in wpdb::prepare()

comment:35 in reply to: ↑ 34 jbutkus16 months ago

Replying to miqrogroove:

I think a better strategy would be to run two separate loops: foreach($args) to create a list of which args were NULL and unset them, followed by foreach($positions[0]) to modify the query string.

This strategy would avoid duplicating the $args array, which is potentially extremely expensive when working with large queries.

Why I propose array_values is that it is faster than foreach by some 65% (several timings running plain foreach with value-copy, vs. array_values, on 10.000 elements array). Thus having two foreach loops shall not give viable performance impact. And we get rid of unnecessary keys.

I have uploaded a new patch, which may be treated as more memory-concious. Here I got rid of extra $values variable.

What do you think of this approach?

comment:36 miqrogroove16 months ago

I'm not suggesting a value-copy strategy. With two loops the original $args array can be used.

comment:37 arman.poghosyan3 months ago

  • Keywords needs-patch added; has-patch removed
  • Resolution set to invalid
  • Status changed from accepted to closed

Still no any PDO support in Wordpress and this patch is not in core either.
I tested the patch in different scenarios and while it's working perfectly in INSERT and UPDATE statements, this patch breaks ->prepare for other statements that were working without patch,
for example:

SHOW FULL COLUMNS FROM `table_name`;

I haven't tested it with SELECT statements either. But I still think that we need support for NULL in $wpdb->prepare, as many plugins solely rely on wordpress database functions and they can't operate with NULL values simply because wordpress doesn't support them in prepare, or they must forget about prepare and use other functions directly, than what's the purpose of prepare for security?

comment:38 westi3 months ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

comment:39 arman.poghosyan3 months ago

OK, so other tests showed also that it doesn't work with UPDATE statements either, e.g.

UPDATE `table_name` SET `var`=%s;

is replaced with

UPDATE `table_name` SET `var` NULL ;

it basically strips = sign

comment:40 son9ne7 weeks ago

removed.. sorry

Last edited 7 weeks ago by son9ne (previous) (diff)
Note: See TracTickets for help on using tickets.