The upgrade path from Drupal 6 (or from 7.x-1.x) is really broken, suggesting a misunderstanding of how update hooks operate (and how they interact with hook_schema()).

The original problem is that update 7200 installs the most up-to-date schema -- before any of the subsequent (and potentially schema-modifying) updates will execute. This is absolutely wrong. When an update hook installs a schema (as opposed to doing it in hook_install()), that schema must subsequently remain untouched, so that the assumptions made by all the subsequent update hooks will always be valid.

For example, update 7209 adds a new column to the schema (making the reasonable assumption that no such column already exists), but in fact the column does exist, because the latest schema was (incorrectly) installed in 7200, so we get the error "Cannot add field weight_settings.sync_translations: field already exists."

This isn't difficult to fix -- define a copy of the original schema, and manually install that one instead of the current one in update 7200 -- but doing so in this case triggers a new issue, as update 7202 wants to insert into the weight_weights table, but this shouldn't exist yet -- it's not created until update 7205!

Right now I'm not sure what the correct fix is. There are a few different problems, and if you're not careful you run the risk of breaking things for existing users who are currently running some arbitrary earlier release of the module, so the fix needs to cover all releases.

The biggest thing is to make sure you fix the schema used in 7200, as the current approach is just going to cause maintenance pain. Perhaps as long as that is done, and it's certain that all the other existing update hooks will work both with and without the fix, that is sufficient.

The key things to remember about update hooks and schemas are:

1) When you install a module for the first time, none of the update hooks are executed. Drupal simply makes a note of the 'last' update hook in the install file, so it knows where to start from if a new update hook is added. The schema defined in hook_schema() must therefore always be kept up-to-date, as it won't be modified by any of the existing update hooks when it's used in hook_install().

2) Conversely if an update hook installs a schema, that schema definition must permanently remain in its original state, because all the subsequent update hooks will be executed.

3) Once you've added and released an update hook, you generally can't take it back or substantially change it. For every update hook you write, you need to assume that someone has downloaded that version of the module and executed the update hook.

You also need to assume that someone out there is still running that version of the module, as no matter which version of the module someone is running, it is necessary that they can successfully execute all the subsequent update hooks in sequence.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jweowu’s picture

Issue summary: View changes

My comments on 7206 were mistaken -- that one does check for the table before attempting to use it.

jweowu’s picture

Issue summary: View changes

suggestion

jweowu’s picture

Status: Active » Needs review
FileSize
5.52 KB

Okay, I think this patch sorts it out. It's a compromise between the existing code and the normal approach.

Update 7200 now installs the original schema, but also the later weight_weights table which is needed by 7202. The existing update hooks were already checking for the existence of that table, so this means we can leave them as-is.

Update 7209 checks for weight_settings.sync_translations in case there's anyone in an invalid state due to the schema mis-match, but now that we're using the correct schema in 7200, this update hook should execute as normal for anyone upgrading.

Future update hooks won't need to account for variations in weight_schema()

I've also fixed a bug in update 7210, as it was joining weight_weights to node on entity_id = nid only, when it's also necessary that entity_type = 'node'. That query had actually been written twice -- once as a static query and once as a dynamic query -- so I've refactored that code to use the dynamic query in both instances.

jweowu’s picture

Issue summary: View changes

tweaking

davisben’s picture

Status: Needs review » Fixed

Thanks for the patch! Committed to dev.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Also mentioned 7.x-1.x, as the upgrade path is the same there as for 6.x