WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#34482 closed defect (bug) (invalid)

wp-db.php incorrect usage of strip_invalid_text() in process_fields()

Reported by: fhwebcs Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.2
Component: Database Keywords:
Focuses: Cc:

Description

Using $wpdb->update I'm stumbling upon errors while updating multiple fields.

Source: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/wp-db.php#L2083

In my opinion the result of strip_invalid_text() within process_fields() is used incorrectly:

    $converted_data = $this->strip_invalid_text( $data );

    if ( $data !== $converted_data ) {
        return false;
    }

Comparing $data (multiple nested array) with returned value (array|WP_Error) from strip_invalid_text() makes no sense. So, it should be replaced by:

    $data = $this->strip_invalid_text( $data );

    if ( is_wp_error( $data ) ) {
        return false;
    }

For another example see usage of $this->strip_invalid_text() in strip_invalid_text_from_query() (https://core.trac.wordpress.org/browser/trunk/src/wp-includes/wp-db.php#L2887)

Change History (9)

#1 @TobiasBg
4 years ago

  • Milestone changed from Awaiting Review to 4.4
  • Version changed from trunk to 4.2

Thanks for the report! Indeed, this looks like arrays are being compared.

Introduced in [30345] for #21212.

#2 @TobiasBg
4 years ago

  • Keywords reporter-feedback added

Comparing arrays like this is however not a problem (see the PHP docs).
From the function's documentation, it's the desired operation. If text has been stripped from the $data array, the function shall return false so that the query is rejected.

Checking for that WP_Error does not make sense here, as that would only trigger if the invalid text could not be stripped from the text for some reason.

Could you therefore please explain what errors you are seeing and what input/query is triggering them?

#3 @fhwebcs
4 years ago

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

You are right! I'v got lost because of missing error messages (see https://core.trac.wordpress.org/ticket/32167 for related debugging improvement). In my case, the string got truncated during strip_invalid_text() and -as documented- the result value becomes false.

So, my issue can be closed. Thank you.

#4 @TobiasBg
4 years ago

  • Milestone 4.4 deleted

No problem! Good to hear that you found the cause now!

#6 @TiagoGouvea
4 years ago

Ok.. more detailed information about problem here:

I'm trying to update a record, setting a field to null (but it already null), and the $converted_data are stripping the 'NULL' to 2 characters because the field are a char(2)... and then, it result in false due the $data is different of the $converted_data.

Here the dumps:

// $data
array (size=1)
  'end_estado' => 
    array (size=4)
      'value' => string 'NULL' (length=4)
      'format' => string '%s' (length=2)
      'charset' => string 'latin1' (length=6)
      'length' => 
        array (size=2)
          'type' => string 'char' (length=4)
          'length' => int 2

// $converted_data
array (size=1)
  'end_estado' => 
    array (size=4)
      'value' => string 'NU' (length=2)
      'format' => string '%s' (length=2)
      'charset' => string 'latin1' (length=6)
      'length' => 
        array (size=2)
          'type' => string 'char' (length=4)
          'length' => int 2

What you think?
Thanks

#7 @dd32
4 years ago

Hi @TiagoGouvea,

Can you just confirm whether you're passing NULL or "NULL" to the function?

Given the data you've provided, it looks like you're passing it as a string (which isn't supported, and MYSQL would attempt to set it to a literal string "NULL").

#8 @TiagoGouvea
4 years ago

Cheking here, I'm passing 'NULL' on array.. but, I have a filter that fix it. Then, in the final query the NULL will not be a string.

add_filter('query', 'wp_db_null_value');
function wp_db_null_value($query)
{
    return str_ireplace("'NULL'", "NULL", $query);
}

Ok.. I'm realizing that I did so much to force the situation here. When I leave the null value, null, the final query turn it to when updating.

What can be a best approach to set a field to null in a generic wpdb->update() ?

Thanks

#9 @dd32
4 years ago

  • Keywords reporter-feedback removed

Answered in the original ticket: comment:3:ticket:34703

Note: See TracTickets for help on using tickets.