Make WordPress Core

Opened 6 years ago

Last modified 4 years ago

#43578 new defect (bug)

Unexpected MYSQL data format

Reported by: loranrendel's profile loranrendel Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.9.4
Component: Database Keywords: 2nd-opinion has-patch has-unit-tests dev-feedback
Focuses: Cc:

Description

When I use field user_id in $wpdb->insert it set value to Integer, but the table I add data into has user_id text field.
It works normally only if format parameter specified.

Example:

<?php
$wpdb->query("CREATE TABLE {$wpdb->prefix}_test (`id` INT, `user_id` VARCHAR(16))");
$wpdb->insert("{$wpdb->prefix}_test", ['id' => 1, 'user_id' => 'stringKey']);
print_r($wpdb->get_row("SELECT * FROM {$wpdb->prefix}_test WHERE id = 1"));

Result: stdClass Object ( [id] => 1 [user_id] => 0 )

Attachments (1)

43578.diff (5.4 KB) - added by soulseekah 6 years ago.

Download all attachments as: .zip

Change History (5)

#1 @dd32
6 years ago

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

Hi @loranrendel and welcome back to Trac.

This happens because WordPress has mapped the user_id column internally as a numeric field and applies %d to the values through insert(), replace() and update() on $wpdb.

For reference, the query generated by the $wpdb->insert() call is this:

INSERT INTO `wp_test` (`id`, `user_id`) VALUES (%s, %d)
->
INSERT INTO `wp_test` (`id`, `user_id`) VALUES ('1', 0)

Possible options:
a) Expected behaviour, don't do this, and if you do, specify the data-types
b) Extend the field_types to have a table specifier, so that the fields have to be defined on a per-table basis.
c) Treat custom tables as custom tables, and only ever use %s unless specified otherwise in the call. However $wpdb->field_types is public and plugins could theoretically update it.

#b + #c has a back-compat issue with existing code which is receiving %d because it's writing to a custom table with similar column names would start writing with %s instead, which then uses MySQL's data-type conversion instead.
I should say though, that I don't see that as a downside, just a side effect. The benefits would outweigh the downsides.

#2 @loranrendel
6 years ago

I think option b) is the greatest one. It doesn't have compatibility issues and would not break anything, because for Wordpress' own tables in this case it will work properly and will get %d format, and for any other table it will also receive correct format based on table structure.

I think, stop receiving %d for other tables wouldn't cause back-compat issue because if field is integer it will receive %d as well, and conversion of string to %d is incorrect operation.

I encountered this error when converting some old code to WP plugin and got rid of old mysqli wrapper. Some data was lost when added to DB, but I recovered it from logs.

@soulseekah
6 years ago

#3 @soulseekah
6 years ago

  • Keywords has-patch has-unit-tests dev-feedback added; needs-patch removed

Interesting case.

43578.diff makes sure that only built-in/core tables abide by the field_types rules. Custom tables will always default formats to %s, unless the developer specifically fiddled with $wpdb->field_types in some way.

How it works, the first time $wpdb->process_field_formats() is called two internal (static) lists are built - one with the current tables (which is almost certainly going to only contain core tables), and one with the current field types (which is also almost certainly going to only containt core field types).

process_field_formats() now accepts a third ghost parameter (since it's protected we cannot just add it as a regular parameter since warnings are going to be thrown). A _doing_it_wrong message is sent out to all subclasses not supplying the third parameter. The third parameter is the $table.

When the field formats are processed, we make sure that $table is a built-in table before letting one of the field_types supply the default format for a field. We also allow custom field overrides to be added to field_types, these will be respected, except in cases where the field name matches a built-in field.

// without patch
"INSERT INTO `wp_test` (`id`, `user_id`) VALUES (%s, %d)"

// with patch regular
"INSERT INTO `wp_test` (`id`, `user_id`) VALUES (%s, %s)"

// with patch custom field
$wpdb->field_types['balance'] = '%d';
"INSERT INTO `wp_test` (`id`, `user_id`, `balance`) VALUES (%s, %s, %d)"

// with patch custom field collission
$wpdb->field_types['user_id'] = '%d'; // not much sense, since it already exists
"INSERT INTO `wp_test` (`id`, `user_id`) VALUES (%s, %s)" // note the %s, drawback?

It is probably a bad idea to reformat $wpdb->field_types, since it is public and can be changed or read by developers. Keeping it as is is crucial. One idea to overcome this limitation is make $field_types an object that implements ArrayAccess, and has an add_field_type( $field, $format, $table ) method. This is probably over-engineering, though.

Thoughts?

#4 @loranrendel
4 years ago

Maybe something like this for non-wordpress tables:

<?php
$fields = ($wpdb->get_results("DESC {$wpdb->prefix}regsystem_applications2"));

We will have types for any field

    [0] => Array
        (
            [Field] => id
            [Type] => int(11)
            [Null] => NO
            [Key] => PRI
            [Default] => 
            [Extra] => auto_increment
        )

    [1] => Array
        (
            [Field] => form_id
            [Type] => int(11)
            [Null] => YES
            [Key] => 
            [Default] => 
            [Extra] => 
        )

    [2] => Array
        (
            [Field] => record_id
            [Type] => int(11)
            [Null] => YES
            [Key] => 
            [Default] => 
            [Extra] => 
        )

    [3] => Array
        (
            [Field] => user_id
            [Type] => varchar(64)
            [Null] => YES
            [Key] => 
            [Default] => 
            [Extra] => 
        )

Note: See TracTickets for help on using tickets.