Error updating table for class during setup advanced

[wuuti]

Bug report

Summary

When I run setup advanced i get multiple errors like this shown in the installation summary:

Error updating table for class MODX\Revolution\modDashboard
Array
(
[0] => 42S21
[1] => 1060
[2] => Duplicate column name 'customizable'
)

Step to reproduce

Not sure how to exactly reproduce it. Happened with the github based revo installation for 3.x

Observed behaviour

Also there are errors shown in the summary, it seems those errors did not prevent a successful installation. You can then click the "Next" button and continue. You do not know, if the shown errors now are a problem or not.

Expected behavior

• there should not be errors like this in general
• if the error is more a warning, we should output that as a warning, not as an error
• if the error does not affect the installation, we may consider not even to show it, as it will only unsettle people
• if the error affects the installation, then the upgrade imho should have stopped immediately, rolled back and not shown as being successful

[JoshuaLuckers]

The list of errors I get:

Error updating table for class MODX\Revolution\modDashboard
Array
(
[0] => 42S21
[1] => 1060
[2] => Duplicate column name 'customizable'
)
Error updating table for class MODX\Revolution\modDashboardWidget
Array
(
[0] => 42S21
[1] => 1060
[2] => Duplicate column name 'permission'
)
Error updating table for class MODX\Revolution\modDashboardWidget
Array
(
[0] => 42S21
[1] => 1060
[2] => Duplicate column name 'properties'
)
Error updating table for class MODX\Revolution\modDashboardWidgetPlacement
Array
(
[0] => 42S21
[1] => 1060
[2] => Duplicate column name 'user'
)
Error updating table for class MODX\Revolution\modDashboardWidgetPlacement
Array
(
[0] => 42S21
[1] => 1060
[2] => Duplicate column name 'size'
)
Error updating table for class MODX\Revolution\modContentType
Array
(
[0] => 42S21
[1] => 1060
[2] => Duplicate column name 'icon'
)

[JoshuaLuckers]

When doing the advanced setup to change the database configuration it will run the upgrade scripts for 3.0.0 again.
One of them is this one: https://github.com/modxcms/revolution/blob/9a527c6b4ac51c9a49c765dc04d93202ce0ca43e/setup/includes/upgrades/common/3.0.0-dashboard-widgets.php

It tries to add fields that already exist. I'm not sure if xPDO should check if a field already exists or that we should do this in the upgrade script.

[Jako]

Is this a version check issue like in #14396? The upgrade script should not run again. But maybe this does occur, since we don't have a final 3.0.0.

[JoshuaLuckers]

Yes, because the current version is 3.0.0-alpha3 and the version for the upgrade script is 3.0.0 and the version compare will return true:

revolution/setup/includes/modinstallversion.class.php

Lines 132 to 134 in 803ebb4

	if (version_compare($this->version,$sc,'<')) {
	$scripts[] = $path.$sc.'.php';
	}

[Mark-H]

@Jako @JoshuaLuckers Are you really concerned about these errors?

As far as I'm concerned they're harmless and expected while upgrading between different releases with the same version number. I'm inclined to "wontfix" unless there are good reasons we shouldn't.

--

As for the expected behavior @wuuti describes:

there should not be errors like this in general

Agreed, however...

if the error is more a warning, we should output that as a warning, not as an error

... it technically is an error, as a migration failed.

It is harmless because as the error indicates, the column it tried to add is already there.

Unless we're going to do pattern matching to determine when "Error updating table for class ..." is safe to ignore and when it isn't, I'd rather leave the interpretation to a human being. If necessary we can add common cases into the upgrade documentation to assist humans with the interpretation.

if the error does not affect the installation, we may consider not even to show it, as it will only unsettle people

In this case, it doesn't affect it and is entirely safe to ignore.

However, there may be other cases where it can't update a table that do indicate a problem (perhaps an incompatible SQL mode) and hiding it would let that slip through without the user being prompted to review the problem.

As per the previous point, this evaluation is in my opinion best left to a human, as the error message may be subtly different between mysql flavors, versions or modes.

if the error affects the installation, then the upgrade imho should have stopped immediately, rolled back and not shown as being successful

Given the ambiguity as to wether or not a table not getting upgraded is an error, rolling back in such a case would prevent people from upgrading between nightlies or alpha builds. The MODX setup also doesn't currently support rolling back, as migrations only go "up" and not "down".

A separate issue to track adding "down" migrations could be useful, but I have some doubts enough people are interested in downgrading regularly enough to warrant the effort of that being implemented and for all future migrations to be written both ways too.

For the rare cases a downgrade is necessary, reverting to a backup of the database and files would be more efficient.

--

If we do want this addressed, we'd need to be more specific in the version numbering on the migrations, i.e. have separate migrations for 3.0.0-alpha1, 3.0.0-alpha2, and 3.0.0-alpha3 so only the appropriate ones are executed. And then separate migrations for every beta and rc in the future, too.

After that we'd still see the same errors in nightlies, because those don't use separate version numbers...