WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 11 months ago

#15158 closed enhancement (fixed)

wpdb insert & update with null values

Reported by: westi Owned by: pento
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.0.1
Component: Database Keywords: has-patch needs-docs
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 (6)

15158.diff (2.0 KB) - added by sorich87 6 years ago.
wpdb_prepare_null.15158.patch (1.5 KB) - added by jbutkus 4 years ago.
Patching wpdb::prepare() to support NULL handling
wpdb_prepare_null.15158.v2.patch (1.5 KB) - added by jbutkus 4 years ago.
Fixed patch for NULLs support in wpdb::prepare()
wpdb_prepare_null.15158.v3.patch (1.7 KB) - added by jbutkus 4 years 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 4 years ago.
More memory-concious approach towards NULL support in wpdb::prepare()
15158.2.diff (4.1 KB) - added by pento 11 months ago.

Download all attachments as: .zip

Change History (50)

#1 @Denis-de-Bernardy
6 years ago

  • Cc Denis-de-Bernardy added

#2 @hakre
6 years ago

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

#3 @jane
6 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.

@sorich87
6 years ago

#4 @sorich87
6 years ago

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

#5 @westi
6 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?

#6 follow-up: @nacin
6 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.

#7 @nacin
6 years ago

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

#8 @westi
6 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

#9 @TechDagan
5 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.

#10 @eddieringle
5 years ago

  • Cc eddie@… added

#11 @cyberhobo
5 years ago

  • Cc cyberhobo@… added

#12 @deltafactory
5 years ago

  • Cc deltafactory added

#13 @andkrup
4 years ago

  • Cc andkrup added

#14 @badconker
4 years ago

  • Cc aurelien.joahny@… added

#15 @jbutkus
4 years ago

  • Cc butkus.justas@… added

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

#16 @nacin
4 years ago

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

#17 in reply to: ↑ 6 @miqrogroove
4 years 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?

#18 follow-up: @miqrogroove
4 years 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

#19 in reply to: ↑ 18 ; follow-up: @jbutkus
4 years 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.

#20 in reply to: ↑ 19 ; follow-up: @TechDagan
4 years 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.

#21 in reply to: ↑ 20 @jbutkus
4 years 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.

#22 @jbutkus
4 years 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.

@jbutkus
4 years ago

Patching wpdb::prepare() to support NULL handling

#23 follow-up: @miqrogroove
4 years 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!' );

#24 @miqrogroove
4 years 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.

@jbutkus
4 years ago

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

#25 in reply to: ↑ 23 ; follow-up: @jbutkus
4 years 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?

#26 in reply to: ↑ 25 ; follow-up: @miqrogroove
4 years 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.

#27 follow-up: @deltafactory
4 years 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;

@jbutkus
4 years ago

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

#28 in reply to: ↑ 26 ; follow-up: @jbutkus
4 years 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.

#29 in reply to: ↑ 27 ; follow-up: @jbutkus
4 years 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.

#30 in reply to: ↑ 29 @deltafactory
4 years 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.

#31 in reply to: ↑ 28 @miqrogroove
4 years 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.

#32 @miqrogroove
4 years ago

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

#33 @miqrogroove
4 years 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.

#34 follow-up: @miqrogroove
4 years ago

I think a better strategy would be to run to 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.

Version 0, edited 4 years ago by miqrogroove (next)

@jbutkus
4 years ago

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

#35 in reply to: ↑ 34 @jbutkus
4 years 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?

#36 @miqrogroove
4 years ago

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

#37 @arman.poghosyan
3 years 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?

#38 @westi
3 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

#39 @arman.poghosyan
3 years 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

#40 @son9ne
2 years ago

removed.. sorry

Last edited 2 years ago by son9ne (previous) (diff)

#41 @samwilson
15 months ago

I'm surprised that this ticket has been open for so long. Does anyone have any update on whether it's going to be fixed — or is it just accepted behaviour, and there's too much that relies on it now for it to be changed?

@pento
11 months ago

#42 @pento
11 months ago

  • Keywords has-patch needs-docs added; needs-patch removed
  • Milestone changed from Future Release to 4.4

15158.2.diff is something of a modernisation of the behaviour in 15158.diff. It's actually a port of [backpress 279], which has been running on WordPress.com for years.

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

I'm specifically ignoring the format, though I have no problem with documenting that it should be NULL, for clarity of code. @DrewAPicture, can you give your thoughts on updating the docs for ::insert(), ::replace(), ::update() and ::delete()?

With regards to @jbutkus' patches - adding NULL support to ::prepare() is an interesting problem, but it's way outside of the scope of this ticket. Let's handle it in #12819.

#43 @pento
11 months ago

  • Owner changed from sorich87 to pento
  • Status changed from reopened to assigned

#44 @pento
11 months ago

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

In 34737:

WPDB: Allow null values in the CRUD functions.

Specifically, ::insert(), ::replace(), ::update(), and ::delete() can now set a column to NULL, or add the IS NULL condition to the WHERE clause.

This is based on [backpress 279].

Props pento, nbachiyski, sorich87.

Fixes #15158.

Note: See TracTickets for help on using tickets.