Make WordPress Core

Opened 7 years ago

Last modified 17 months ago

#38936 new defect (bug)

Alter Table Always Expects a COLUMN; Crashes on a CONSTRAINT

Reported by: philsown's profile philsown Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Upgrade/Install Keywords: needs-testing dev-feedback
Focuses: Cc:

Description

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.

Change History (9)

#1 @pento
7 years ago

  • Component changed from Plugins to Upgrade/Install
  • Version 4.6.1 deleted

Thanks for the bug report, @philsown!

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

#2 @philsown
7 years ago

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);

#3 @philsown
7 years ago

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

#4 @desrosj
5 years ago

  • Keywords needs-testing added

This ticket was mentioned in Slack in #core-upgrade-install by costdev. View the logs.


18 months ago

#6 @costdev
17 months ago

  • Keywords dev-feedback added

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

#7 @craigfrancis
17 months ago

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.

#8 @costdev
17 months ago

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?

Last edited 17 months ago by costdev (previous) (diff)

#9 @craigfrancis
17 months ago

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

Note: See TracTickets for help on using tickets.