WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 8 days ago

#20263 new defect (bug)

Backticks in dbDelta cause warning and actually causes a query to alter all columns and indexes to run even if none have changed

Reported by: Driskell Owned by:
Milestone: Future Release Priority: normal
Severity: minor Version: 1.5
Component: Database Keywords: has-patch early
Focuses: Cc:

Description

Backticks in a CREATE TABLE in a dbDelta call are causing the below warning, and running a query to alter all columns and indexes with backticks even if they haven't changed:
Notice: Undefined offset: 1 in /home/backuptechnology/public_html/wp-admin/includes/upgrade.php on line 1544

Seems this started a long long time ago after this was fixed:
http://core.trac.wordpress.org/ticket/8014

Seems there is a match on line 1587 of WordPress 3.3.1 wp-admin/includes/upgrade.php:

               // For every field in the table
                foreach ($tablefields as $tablefield) {
                        // If the table field exists in the field array...
                        if (array_key_exists(strtolower($tablefield->Field), $cfields)) {
                                // Get the field type from the query
                                preg_match("|".$tablefield->Field." ([^ ]*( unsigned)?)|i", $cfields[strtolower($tablefield->Field)], $matches);

The preg_match bit is what fails to match because it is looking for "fieldname" instead of "fieldname".
So this is where the warning occurs, and why it assumes the type of the field is wrong.
This means the previous patch 3 years ago probably meant when you have backticks in your CREATE TABLE, when you run dbDelta it actually runs an ALTER TABLE for every single column in the table, and then an ALTER TABLE ADD INDEX / PRIMARY for every single KEY and PRIMARY (there's more than just the above code that needs adjusting), causing duplicate key errors, but ignored. Guess it kinda works though...

Also seems during the DESCRIBE it doesn't use backticks so if someone has a table with a reserved name (stupid I know) it would break dbDelta completely, but that's me digressing I just think getting rid of the warnings for CREATE TABLES with backticks will be good.

But explains why my plugin takes a few seconds to activate hahaha. :-)

I'll provide a patch at some point this week or next unless someone has already done so.

Attachments (2)

20263.patch (677 bytes) - added by kurtpayne 3 years ago.
Remove backticks when comparing fields and indexes
20263.diff (791 bytes) - added by davidmosterd 10 days ago.

Download all attachments as: .zip

Change History (11)

comment:1 @johnbillion3 years ago

  • Cc johnbillion added

@kurtpayne3 years ago

Remove backticks when comparing fields and indexes

comment:2 @kurtpayne3 years ago

  • Cc kpayne@… added
  • Keywords has-patch added

I can replicate the original problem. Here's the sample plugin I used to test this:

https://gist.github.com/2151282

Just activate the plugin and look for errors. Deactivate the plugin to reset.

comment:3 @nacin3 years ago

  • Version changed from 3.4 to 1.5

Definitely not new, setting version to 1.5 when dbDelta came around.

Adding ` to the list of characters to trim() might be enough, yes?

comment:4 @kurtpayne3 years ago

Adding ` to the trim list won't fix it because a backtick can occur in the middle of the string:

`user_ip` int(11) DEFAULT 0,

comment:5 @timfs3 years ago

What's so complicated? Took me 10 seconds:

preg_match("|`?" . preg_quote($tablefield->Field) . "`? ([^ ]*( unsigned)?)|i", $cfields[strtolower($tablefield->Field)], $matches);

This also fixes problems with tablenames that contain "special" characters, in MySQL for example foo.bar.[b-a-z] is a perfectly valid tablename, besides using non-sanitized external input is always a very bad idea.

However, this Regex is pretty fragile anyways, it can't handle multiple spaces, it can't handle tabs, it can't handle spaces between the parentheses enclosing the column length, etc... someone should really rewrite that whole stuff, it might explode by a slight sneeze.

Version 0, edited 3 years ago by timfs (next)

comment:6 @dd324 months ago

  • Component changed from Upgrade/Install to Database

comment:7 @andddd3 months ago

It's been couple of years. Can this be finally fixed because my plugins spits a ton of errors on each update. @timfs proposed a reasonable patch.

Last edited 3 months ago by andddd (previous) (diff)

@davidmosterd10 days ago

comment:8 @davidmosterd10 days ago

Little context: Here with a mentor (Mark Jaquith) on contributor day WordCamp London 2015 and just looking to get my first commit going, found this ticket :)

We did this assumption: When you supply a CREATE TABLE statement, dbDelta() should handle what MySQL gives you when you export the CREATE TABLE statement

I tried to simplify this case by first removing the first appearance of the column name from the query, and then ltrim() from the remainder any remaining spaces and an optional backtick (in case the column name was enclosed in backticks). The string should now start with the data type, maybe having " unsigned" behind it. The remaining preg_match() is quite simple now.

comment:9 @markjaquith8 days ago

  • Keywords early added
  • Milestone changed from Awaiting Review to Future Release
Note: See TracTickets for help on using tickets.