Alter Table Always Expects a COLUMN; Crashes on a CONSTRAINT

[philsown]

Hello,

I'm attempting to activate a plugin I'm developing. The database creation scripts have CONSTRAINTs on them. When I attempt to reactivate, I'm getting this error:

WordPress database error: [You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'CONSTRAINT `mytable_mycol_foreign` FOREIGN KEY (`mycol' at line 1]
ALTER TABLE wp_mytable ADD COLUMN CONSTRAINT `mytable_mycol_foreign` FOREIGN KEY (`mycol`) REFERENCES `myothertable` (`myothercol`)

As you can see the SQL error lies in ADD COLUMN CONSTRAINT.

This is being generated in wp-admin/includes/upgrade.php around line 2392

<?php
// For every remaining field specified for the table.
foreach ($cfields as $fieldname => $fielddef) {
        // Push a query line into $cqueries that adds the field to that table.
        $cqueries[] = "ALTER TABLE {$table} ADD COLUMN $fielddef";
        $for_update[$table.'.'.$fieldname] = 'Added column '.$table.'.'.$fieldname;
}

ADD COLUMN is hardcoded and is creating this SQL error. I googled for a solution but didn't find anything.

I've tried it with the constraints being part of the full table creation statement, and also as a stand alone statement, with the same results.

[pento]

Thanks for the bug report, @philsown!

Could you post the CREATE TABLE statement that's causing this?

[philsown]

Here's the SQL I'm running

<?php
$sql = '
CREATE TABLE `wp_vendor_plugin_activities` (
    `id` int(10) unsigned NOT NULL AUTO_INCREMENT,
    `activity_type_id` int(10) unsigned NOT NULL,
    `title` varchar(255) COLLATE utf8_unicode_ci DEFAULT NULL,
    `description` varchar(255) COLLATE utf8_unicode_ci DEFAULT NULL,
    `created_at` timestamp NOT NULL DEFAULT \'0000-00-00 00:00:00\',
    `updated_at` timestamp NOT NULL DEFAULT \'0000-00-00 00:00:00\',
    PRIMARY KEY (`id`),
    KEY `vendor_plugin_activities_activity_type_id_foreign` (`activity_type_id`),
    CONSTRAINT `vendor_plugin_activities_activity_type_id_foreign` FOREIGN KEY (`activity_type_id`) REFERENCES `wp_vendor_plugin_activity_types` (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci';

dbDelta($sql);

[philsown]

The fix in the meantime has been, of course and unfortunately, to remove the CONSTRAINT.

[costdev]

Pinging @craigfrancis to see if he has any thoughts on this ticket.

[craigfrancis]

Quick first thoughts (not run any code to check, and probably can't until later next week)...

WordPress doesn't currently support Constraints, and doing so might get a bit complicated (especially when it comes to existing table records that don't conform to the constraint).

---

upgrade.php currently does a fairly basic parse of the CREATE TABLE queries (aka $cqueries).

It will run DESCRIBE {$table} to determine if the table currently exists. If that fails, it will simply run that SQL... if it succeeds, the table will be checked, and the table will be ALTERed as necessary (adding fields/indexes).

When doing the ALTER approach, it simply splits the query by line ($flds).

​/src/wp-admin/includes/upgrade.php#L2817

It tries to determine the $fieldname (the first word)... and if it matches 'primary', 'index', 'fulltext', 'unique', 'key', or 'spatial'; it is no longer considered a $validfield (good, because it's not a field), and will not be added to the $cfields array.

Your SQL starts the line with CONSTRAINT, so it's incorrectly added to $cfields, and uses the ADD COLUMN approach (the same would happen if you had a line that started with FOREIGN KEY).

So a new case 'constraint': needs to be added, and probably a few other changes in the rest of that bit of code, where it might end up in $indices... but it won't use the format "index_type index_name (...)".

---

So, do we want to go down the route of supporting Constraints?

They are useful to check the data in the tables, but they can introduce some complexity.

[costdev]

Thanks @craigfrancis!

If I understand correctly, and there's a very good chance I don't:

1. This ticket's issue requires a patch to add a new case 'constraint':, and the other changes you mentioned to stop adding CONSTRAINT to $cfields and resolve this issue.
2. If there is a desire to support Constraints in WordPress, this should be discussed in a separate ticket to get thoughts on complexity, performance, BC, and all the usual considerations.

Is that right, or?

[craigfrancis]

Yep, perfect summary (thanks, one day I’ll learn how to keep it brief).