Make WordPress Core

Opened 5 years ago

Closed 5 months ago

Last modified 3 months ago

#20263 closed defect (bug) (fixed)

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: ocean90
Milestone: 4.6 Priority: normal
Severity: normal Version: 1.5
Component: Database Keywords: has-unit-tests has-patch
Focuses: Cc:

Description (last modified by ocean90)

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: #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 (10)

20263.patch (677 bytes) - added by kurtpayne 5 years ago.
Remove backticks when comparing fields and indexes
20263.diff (791 bytes) - added by davidmosterd 20 months ago.
20263.2.diff (1.6 KB) - added by ocean90 6 months ago.
20263.3.diff (1.5 KB) - added by ocean90 5 months ago.
20263.4.diff (3.9 KB) - added by ocean90 5 months ago.
20263.5.diff (8.8 KB) - added by ocean90 5 months ago.
20263.6.diff (11.3 KB) - added by ocean90 5 months ago.
moar tests
20263.7.diff (11.7 KB) - added by pento 5 months ago.
20263.8.diff (14.6 KB) - added by ocean90 5 months ago.
20263.9.diff (14.6 KB) - added by ocean90 5 months ago.

Download all attachments as: .zip

Change History (41)

#1 @johnbillion
5 years ago

  • Cc johnbillion added

5 years ago

Remove backticks when comparing fields and indexes

#2 @kurtpayne
5 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:


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

#3 @nacin
4 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?

#4 @kurtpayne
4 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,

#5 @timfs
4 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.

Last edited 4 years ago by timfs (previous) (diff)

#6 @dd32
2 years ago

  • Component changed from Upgrade/Install to Database

#7 @andddd
23 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 23 months ago by andddd (previous) (diff)

20 months ago

#8 @davidmosterd
20 months 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.

#9 @markjaquith
20 months ago

  • Keywords early added
  • Milestone changed from Awaiting Review to Future Release

#10 @dd32
16 months ago

#30655 was marked as a duplicate.

This ticket was mentioned in Slack in #glotpress by ocean90. View the logs.

8 months ago

#12 @ocean90
8 months ago

  • Keywords needs-unit-tests added

We noticed the same with GlotPress which has a colum "references", see https://github.com/GlotPress/GlotPress-WP/pull/343.

It seems to work with BackPress because BP_SQL_Schema_Parser::delta() supports backticks, see https://backpress.trac.wordpress.org/browser/trunk/includes/class.bp-sql-schema-parser.php?marks=118,130,144#L117.

#13 @jrfoell
7 months ago

Just joining the conversation here, hoping this gets resolved since it seems like an easy fix. Still an issue in WP 4.4 :(

6 months ago

#14 @ocean90
6 months ago

20263.2.diff adds a simple unit test but it probably needs a few more.

@davidmosterd: I noticed that the strtolower() for $tablefield->Field got removed in 20263.diff. Not sure if that was intended or not.

#15 @ocean90
5 months ago

  • Description modified (diff)
  • Keywords has-unit-tests added; needs-unit-tests removed
  • Milestone changed from Future Release to 4.6
  • Owner set to ocean90
  • Severity changed from minor to normal
  • Status changed from new to reviewing

#16 @pento
5 months ago

Rather than doing the ltrim() magic, I'd be inclined to add the `s to the regex. That's the same as how wpdb::get_table_from_query() behaves, as well as the BackPress example that @ocean90 linked to.

#17 @ocean90
5 months ago

In 37538:

Database: Support backticks around field names when parsing a query for the field type.

Avoids an undefined index PHP warning by dbDelta().

Props davidmosterd, ocean90.
See #20263.

5 months ago

5 months ago

#19 @ocean90
5 months ago

  • Keywords needs-patch added; has-patch early removed

Turns out the approach in 20263.patch doesn't work because it would break a query where backticks are required, like for reserved keywords. 20263.4.diff includes a test for that.

In 20263.4.diff I tried to escape every field in our custom tables which doesn't quite work because of the array_search() call.

I'll take another look tomorrow and will probably copy parts of BP_SQL_Schema_Parser:: get_index_definition() and https://backpress.trac.wordpress.org/browser/trunk/includes/class.bp-sql-schema-parser.php?marks=118-143#L117 to wpdb.

This ticket was mentioned in Slack in #glotpress by ocean90. View the logs.

5 months ago

#21 @ocean90
5 months ago

  • Keywords has-patch added; needs-patch removed

20263.5.diff normalizes the index definitions so they match with the result of SHOW INDEX FROM in https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/upgrade.php?rev=37538&marks=2274-2337#L2274.

The regular expressions are copied from BackPress, slightly changed to have named capture groups.

This patch fixes also #34959, #34871, #34873 and #34869. #34874 would be possible too.

Last edited 5 months ago by ocean90 (previous) (diff)

5 months ago

5 months ago

moar tests

5 months ago

#22 @pento
5 months ago

In 20263.7.diff:

  • Updated the patch to apply cleanly after [37574].
  • Added SPATIAL to the index preg_match() and tidied it up a little.
  • Merged two trim() calls at the start of foreach ( $flds.... (The sequential trims didn't operate quite the same as previous usage.)

I like the direction of this patch. My primary concern is that the new index matching chunk of code is fairly dense, it could do with some whitespace, and maybe even some comments (forbid the thought!). I don't relish the idea of trying to remember how this all works when our next dbDelta() maintenance spree comes around.

Oh, and I forgot to fix it it my patch - after foreach ( $index_columns... we need to unset( $index_column ).

5 months ago

#23 @ocean90
5 months ago

20263.8.diff is 20263.7.diff plus whitespace and a few more comments. It also includes the missing unset() part.

5 months ago

#24 follow-up: @ocean90
5 months ago

In 20263.9.diff I changed the regex to match a column/index name from \w to (?:[0-9a-zA-Z$_-]|[\xC2-\xDF][\x80-\xBF])+ which is also used by wpdb::get_table_from_query().

Technically, MySQL supports more characters. From the docs:

Permitted characters in unquoted identifiers:

  • ASCII: [0-9,a-z,A-Z$_] (basic Latin letters, digits 0-9, dollar, underscore)
  • Extended: U+0080 .. U+FFFF

Permitted characters in quoted identifiers include the full Unicode Basic Multilingual Plane (BMP), except U+0000:

  • ASCII: U+0001 .. U+007F
  • Extended: U+0080 .. U+FFFF

Should we support all characters?

#25 in reply to: ↑ 24 @pento
5 months ago

Replying to ocean90:

Should we support all characters?

Let's split that out into a new ticket, because it may turn into a rabbit hole, and we'll need to change WPDB to add the same support.

#26 @ocean90
5 months ago

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

In 37583:

Database: Normalize index definitions in dbDelta().

dbDelta() compares the index definitions against the result of SHOW INDEX FROM $table_name. This requires a specific format so indices are not unnecessarily re-created. This format wasn't ensured, until now.

  • Parse the raw index definition to extract the type, name and columns so a normalized definition can be built (#20263, #34873).
  • Standardize on uppercase types (#34871) and on 'KEY'. 'INDEX' is only a synonym for 'KEY'.
  • Escape index names with backticks (#20263).
  • Normalize columns: Ignore ASC and DESC definitions (#34959), remove whitespaces (#34869) and escape column names with backticks (#20263).
  • Add backticks to all index change queries (#20263).

Props ocean90, pento, kurtpayne.
Fixes #20263, #34869, #34871, #34873, #34959.

#27 @davidmosterd
5 months ago

@ocean90 Cool :)

So, in the end my solution was not used at all? I was not able to do more on this; wish I could. I did not get props I see, which I get if my code was not good enough, just want to make sure it's no mistake ;)

#28 @ocean90
5 months ago

@davidmosterd You got props in [37538] for the initial patch. :) The patch was good, thanks for that, but as mentioned in comment:16 adding the backtick to the regex directly is a way less magical.

#29 @ocean90
5 months ago

  • Keywords needs-dev-note added

#30 @ocean90
4 months ago

  • Keywords needs-dev-note removed

#31 @ocean90
3 months ago

#37418 was marked as a duplicate.

Note: See TracTickets for help on using tickets.