Clean-up the upgrade path: comment

#1: 898520-upgrade-cleanup-comment.patch queued for re-testing.

1. $setting = variable_get('comment_default_mode_' . $type, 4); if ($setting == 3 || $setting == 4) { -- oh of course! Everyone knows that 3 and 4 are various collapsed and expanded threaded modes ( I needed to read D6 comment module to know. Do you know these off head?).
2. $preview = variable_get('comment_preview_' . $type, 1) ? DRUPAL_REQUIRED : DRUPAL_OPTIONAL -- i understand the case for if-else (when you need to wonder what are the operation precedences) but this is not one.

Now, helpers:

1. drupal_write_record -- i would not, ever dare to use dwr in an update scenario. The work saved compared to a db_insert is miniscule and I do not want to figure out whether the drupal_get_schema (with its static cache, none less) is correct at this point or not.
2. $schema = (array) module_invoke($field['module'], 'field_schema', $field);and His Angel took a Flaming Sword and chased them out of Paradise (or something like that). Aka: no module_invoke for you.
3. + $types = db_query('SELECT type FROM {node_type}')->fetchCol(); would be better if it would be a helper too.

#902264: Move hook_field_schema() to .install files was committed

We have better tests, cleaner upgrade and helpers. This is good to go. More can be done later if someone discovers a problem we did not cover, here.

No longer applies.

#13: 898520-upgrade-cleanup-comment.patch queued for re-testing.

erm, didn't mean for the status to change with that.

_update_field_create_field() : You're kidding, right ?
Will we need to make, debug and maintain special duplicated 'update' versions of every CRUD API in all of core ?

I'm starting to think we should revisit running updates for disabled modules, iirc it was only ever meant to account for the case where you leave a module disabled over two major drupal versions and expect it to still work perfectly with all your old data when it's re-enabled, and was a very, very late change in D6, the amount of developer time spent dealing with this is disproportionate. If we have to have these helper functions in the meantime they should at least be versioned - _update_field_create_field_7_0() or something.

+ how can field.module even be disabled ?

Do you have an actual problematic case ?
text.module is required
I guess there's a possibility of D6-D7 running while taxonomy.module is disabled - but I thought this had been taken care of in #706842: Improve comments for the taxonomy upgrade path ?

We have discussed this in length at DCCPH and the problem in general is that if you do not upgrade disabled modules, you can't reenable them ever. Now, though text and field modules are of course required, the results of firing hooks simply can not be predicted and so we do not want to use any. That's sad... but yes: we will need to replace every CRUD function -- note that already many updates write direct SQL instead of firing APIs. Most of the time those APIs allow flexibility -- but we do not want flexibility during update we want predictability or we end up where we are now where the update path is an infinite source of problems.

Just to clarify, thinking we should revisit it doesn't mean right now/for D7, I just don't like the way this is going overall.

For now I'm fine with duplicating functions if that's what's necessary for a working upgrade path, but please let's version them.

OK discussed this more with chx in irc and he asked me to follow up here too.

The reason I think we need to revisit this for D8 is because we have a lot of hard and painful work with the upgrade path, and this isn't just due to not having tests for it for two years, it's a lot of code to maintain that has to do things right in a single shot for thousands of old crappy databases that have been messed around with by core versions going back to Drupal 1 and thousands of contrib modules (like the locale index issue elsewhere). I'd be very keen in Drupal 8 to try to replace update.php for major version changes with migrate module. Keep it around for minor versions, when there's a new core release, you do a fresh install, set it up how you like (hopefully with some exportables support), then point migrate module at the old database and pull the data over. Migrate has lots of lovely things like rollback, monitoring, uses core apis like node_save() etc. that currently update.php can't and doesn't.

On this patch, the reason we need the helper functions to be versioned is this:

7.3 introduces a schema change for field instances, which requires a change to the API function,

7.8 introduces a minor api change for instances which requires a maintenance update of all instances.

At this point we'll need three copies of the function - field_update_instance(), _update_7_0_update_instance() and _update_7_8_update_instance(). If we never need _update_7_8_update_instance() then we don't lose much accounting for it in the first place.

I guess there's a possibility of D6-D7 running while taxonomy.module is disabled - but I thought this had been taken care of in #706842: "taxo as field" update broken + wipes some node/term associations ?

I think there is still an existing problem with it - see #895386: Clean-up the upgrade path: taxonomy - and so far, the extra helper functions like the ones proposed here are the only idea that anyone has come up with that would solve that :(

At this point we'll need three copies of the function - field_update_instance(), _update_7_0_update_instance() and _update_7_8_update_instance(). If we never need _update_7_8_update_instance() then we don't lose much accounting for it in the first place.

The OP discusses that we should rewrite the comment upgrade path so that we only update once where we finally need to be and skip any intermediate states. I think this is wise for comment and other complex modules.

@27: I think that proposal only works in pre-beta-mode. As soon as we have updates from stable to stable version, we have to support intermediate states. We can't tell them to go back to 7.0 to be able to run the 7.7 -> 7.8 update :)

We don't *have* to do anything. We can decide that such a policy is untenable. Frankly, we are in no position to make such promises. Cutting scope is one of our only weapons for getting D7 released. I don't even think that removing the promise of HEAD => HEAD upgrade path during beta is cutting scope.

@moshe I think we need to support updates from 7.7 to 7.8, please read my comment again.

We have discussed this in length at DCCPH and the problem in general is that if you do not upgrade disabled modules, you can't reenable them ever.

Right. And then Barry posited that perhaps sites have to make a tough decision at upgrade time. They need to get modules out of the disabled state and into active or uninstalled. Disabled is not allowed or at least not supported. Someone then mentioned that this would suck for sites that want to upgrade but one of their modules is not ready for D7 yet. They'd like to upgrade then enable the module at some later date and have their data preserved. This is certainly nice to have, but I don't know that we have to support it. Desperate times, desperate measures.

If folks want to keep working on a 'disabled gets upgraded' upgrade path thats cool. But I'm saying that we have a Plan B that should be easier.

Wish we had. Remember, the problem is that when you fire a hook during update it might hit unupdated modules and it might or might not work. Likely: not. I have even suggested not loading any modules but user/system/filter and even then tread carefully. Such a change would render the disabled/nondisabled distinction moot.

Rerolled against HEAD.

D'oh. I forgot about

-    require $this->databaseDumpFile;
+    foreach ($this->databaseDumpFiles as $file) 

now taxonomy is adjusted and happy.

I still think we need to name the update functions + $types = _update_7_0_node_get_types();, chx seemed to agree in irc, but they're not versioned in this patch.

+  field_cache_clear(TRUE);

I thought we were disabling field module, or not relying on it? Why is it OK to use field_cache_clear() here?

Revisioned utility functions , removed bogus TRUE and added some comments to field.info.inc to make sure field cache clear remains safe.

Removed field_cache_clear() completely.

That was my last objection, as long as we didn't need this to make the tests pass (which I hope we don't, because then something else is odd, but please wait for the bot before commit), this looks ready to go.

#39: 898520-upgrade-cleanup-comment.patch queued for re-testing.

#39: 898520-upgrade-cleanup-comment.patch queued for re-testing.

#39: 898520-upgrade-cleanup-comment.patch queued for re-testing.

I fully agree that we need the versioned functions, I just wanted to say that this might produce a problem with contrib/custom modules that are using these.

Let's say, you have a contrib module that converts it's data from a custom table in D6 to a field in D7. You are doing that in whatever_update_7000() and use _update_7_0_field_create_field() for that. Sites are upgrading from D6, execute that function, it works, everything is fine.

Now, Drupal 7.8 comes out and there is an internal change in _update_7_0_field_create_field() so it adds a _update_7_8_field_create_field(). Back to our contrib module, which is still using _update_7_0_field_create_field() even if someone directly upgrades from 6.x to 7.8+. It will try to use _update_7_0_field_create_field() and that might not work anymore because the schema has already been updated to the 7.8 version. Kaboom.

Solutions:
1. All modules using _update_7_0_field_create_field() need to release a new version and either make it conditional to use either 7_0 or 7_8 depending on if the second exists or require 7.8 in their dependencies (yay!!!! for being able to do that). But they need to change existing update functions and we always said that you should never do that.

2. The idea I had is that we could have a _update_latest_field_create_field(). Contrib/Custom modules can use that, and it will use the correct one. In the use case above, it would now simply use _update_7_8_field_create_field(). However, the limitation is that the API must not change between 7_0 and 7_8. But, this is a) a requirement for a stable core version anyway unless there are really really good arguments and this is the same problem for all functions in core, not just the versioned update functions and b) In that case, we can simply fall back to solution 1. I'm not sure if there might be other problems when you are upgrading from 7.0 to say 7.10 together with a bunch of contrib modules at the same time. But I'm not sure if that is supported anyway.

Opinions?

EDIT: In case it is not clear. This not holding up this patch from being RTBC/commited. For 1, there is nothing we need to do and if solution 2 would work, we could add these latest function in a follow-up.

This was RTBC before the bot fluke.

Ok, just looking at this for the first time. Refactoring and renumbering those update hooks, while really nice from a code optimization/organization POV, is going to cause some major headaches for everyone with a D7 site so far without a manual query to update system.schema for all of these modules.

I know we don't support HEAD to HEAD upgrades yet, but this seems like an unnecessary change that causes more problems than it solves. Furthermore, it seems like this is going to unnecessarily push off beta until every single module is fixed. Not interested in that.

I don't know why we don't just leave comment_update_7013() in there as a blank stub function, which is consistent with how we've handled other missing update hooks when code is moved around in the past. See http://api.drupal.org/api/function/system_update_7008/7 for example. So let's fix that.

I'd also like +function _update_7_0_field_create_field(&$field) { and +function _update_7_0_field_create_instance($field, &$instance) { and the like to be surrounded by a consistent Docblock @defgroup that explains what these versioned functions are for.

The series of hook_update_N() functions currently are surrounded by:

 /**
  * @defgroup updates-6.x-to-7.x System updates from 6.x to 7.x
  * @{
  */

...

/**
 * @} End of "defgroup updates-6.x-to-7.x"
 * The next series of updates should start at 8000.
 */

...so something like that. Maybe @defgroup update-api-6.x-to-7.x ? I leave it up to you folks.

That brings up another question, which is how did we settle on a 7_0 naming convention as opposed to a 7000+ one? Browsing through system.install I see functions like http://api.drupal.org/api/function/system_schema_cache_7054/7 that seem to use the same naming convention as hook_update_N(). Seems like it would be more consistent this way?

At the very least, we need some docs in the source code on why it's done the way it is. Probably at the top of system.install. There's a lot of discussion in this issue that's not captured in the source code, and this is bad.

Finally, it's not at all clear why this stuff:

-  var $databaseDumpFile = NULL;
+  var $databaseDumpFiles = array();

...is part of this patch?

Here's another, addressing the concerns above.

I can't reproduce that at all. I ran the upgrade tests and got 1533 passes, 0 fails, 0 exceptions. Here is a repost.

the same to fails..

#50: 898520-upgrade-cleanup-comment.patch queued for re-testing.

I have totally cleaned up and now found that the other test created a new node test file which got unpatched somewhere. D'oh! Back to RTBC because webchick's comments are addressed :)

Great stuff! Committed to HEAD. Yay < 20 criticals! :D

Oh well, I was in the midst of reviewing this when it was committed.

I don't really understand why this issue was marked critical. Unless something was actually broken in a real life instance of the comment upgrade path, it shouldn't be. This is more about theoretical correctness and code cleanup, etc (which are good, but not critical).

In any case:

1.  /**
   + * Migrate data from the comment field to field storage.
   + */
   +function comment_update_7013() {
   + // Drupal 7 alpha 6 contained updates until 7013 so this stub is left here
   + // to make comment schema versions consistent.
   +}
   

   I disagree with this and think we should revert it. Speaking personally, but also as someone working on Drupal Gardens (which is running D7 updates now), I don't think it's necessary. Either Drupal officially supports the upgrade path or it doesn't, and currently it doesn't, so there's no need to pretend it partially does. Plus, this particular issue should be easy for us to work around, either via a custom update function or using Drush to modify the schema record... There have been way, way, WAY harder issues for us to work around before :)

   It would be OK I guess if there were no consequences, but there are - these update functions are visible in the admin UI. So currently, someone updating D6 to D7 will see this if they open the fieldset to look at the list of updates:

   * 7000 - Rename comment display setting variables.
   * 7001 - Change comment status from published being 0 to being 1
   * 7002 - Rename {comments} table to {comment} and upgrade it.
   * 7003 - Split {comment}.timestamp into 'created' and 'changed', improve indexing on {comment}.
   * 7004 - Upgrade the {node_comment_statistics} table.
   * 7005 - Create the comment_body field.
   * 7006 - Migrate data from the comment field to field storage.
   * 7013 - Migrate data from the comment field to field storage.
   

   The repetition and misnumbering in the last two are confusing and make it look like something is wrong with the update. I suggest we kill the second function.

2. +    // - 1 was 'required' previously, convert into DRUPAL_REQUIRED (2).
   +    // - 0 was 'optional' previously, convert into DRUPAL_OPTIONAL (1).
   +    $preview = variable_get('comment_preview_' . $type, 1) ? 2 : 1;
   +    variable_set('comment_preview_' . $type, $preview);
   

   The previous version of this code used DRUPAL_REQUIRED and DRUPAL_OPTIONAL directly. Why did you replace it with integers instead?

3. --- modules/field/field.install	2010-06-25 17:47:22 +0000
   +++ modules/field/field.install	2010-09-11 20:06:34 +0000
   @@ -167,3 +167,133 @@ function field_schema() {
    
      return $schema;
    }
   +
   +
   +/**
   + * @defgroup update-api-6.x-to-7.x Update versions of API functions.
   + * @{
   + * Functions similar to normal API function but not firing hooks.
   

   Why is the generic defgroup defined in the field module? Shouldn't it be defined in system.install instead?

4. +/**
   + * Utility function: create a field directly to the database.
   + *
   + * This function creates fields according to the schema version 7000.
   + */
   
   ...
   
   +/**
   + * Utility function: create a field instance directly to the database.
   + *
   + * This function creates field instances according to the schema version 7000.
   + */
   

   I found this confusing; seems like "directly to the database" should be "by writing it directly to the database", and "according to the schema version 7000" should be more like "via a process that is valid on sites that are at field module schema version 7000 or higher"?

5. +  // Create storage for this field.
   +  module_load_install($field['module']);
   +  $schema = (array) module_invoke($field['module'], 'field_schema', $field);
   ...
   +  module_invoke($field['storage']['module'], 'field_storage_create_field', $field);
   

   The module_load_install() is unnecessary here - the code is guaranteed to be loaded.

   Also, earlier in this function we already explicitly declared that the field module must be 'field_sql_storage', so it might be more readable to call the functions directly rather than via module_invoke().

6. +/**
   + * @defgroup field-updates-6.x-to-7.x Field updates from 6.x to 7.x
   + * @{
   + */
   

   This defgroup needs to be closed at the bottom of the file.

7.  /**
   + * Utility function: fetch the node types directly from the database.
   + */
   +function _update_7010_node_get_types() {
   +  return db_query('SELECT type FROM {node_type}')->fetchCol();
   +}
   

   Why 7010? Seems like it is valid much earlier than that. Also, this function should be added to the update-api-6.x-to-7.x defgroup.

   I am also very tempted to say we don't need this function at all. It's true that node_type_get_types() invokes a hook, but it's a standard info-style hook, so the chances of it not working seem very very slim. I guess for consistency we might, though. Grumble.

8. function _update_user_role_grant_permissions($rid, array $permissions, $module) {
   

   Shouldn't this function (previously introduced in user.install) also have been added to that defgroup and given a function name with 7xxx in it, following the convention introduced in this issue?

9. + * During update, it is impossible to judge the consequences of firing a hook
   + * as it might hit a module not yet updated. So simplified versions of some
   + * core APIs are provided.
   

   I think this needs more explanation. My first inclination is that modules could use hook_update_dependencies() to get around that. Probably we shouldn't make them do that, but I think it needs a more detailed writeup. Possibly this can be handled in #794192: Documentation of hook_update_N() should explain that certain functions cannot be called from there since hook_update_N() is really where the documetation is most urgently needed.

10. function comment_update_dependencies() {
    ...
      // Comment update 7012 creates the comment body field and therefore must run
      // after text module has been enabled and entities have been updated.
      $dependencies['comment'][7012] = array(
        'system' => 7021,
      );
    

    This is now out of date since comment_update_7012() does not exist anymore. Perhaps it can just be removed - I don't think it's declaring any new dependencies that the earlier code in comment_update_dependencies() isn't already implicitly declaring.

11. /**
     * Moved to comment_update_7011().
     */
    function system_update_7030() {
    }
    

    This reference to comment_update_7011() is now out of date.

Hm, it looks like by mistake @webchick didn't actually commit the whole patch here, just some of the test files: http://drupal.org/cvs?commit=419962

So, let's address my comments above before committing the rest :)

Thanks for the review, David!

On the "leaving of legacy stub update functions there" issue, I'm just conforming to what core does already elsewhere in system.install et al (which I think dates back to D6 but not positive). I agree that we're lying to people in the UI currently, since the function doesn't in fact do anything like what it says (though it is doing these things elsewhere), but again, we're following core's lead here.

We should probably file a separate issue to discuss whether or not to retain these stub functions, but personally I think they don't really hurt anything. If anything, I'd change the PHPDoc above them to note that this is a stub function rather than misleading end users.

We should clean up the rest, though.

Oh, also, I think the "critical" nature of this was establishing a pattern we can re-use in other updates. I agree that unless there's a known "in the wild" issue with directly calling API functions, these sorts of clean-up patches are not critical.

Thanks for the quick turnaround.

(2) We don't use constants for the same reasons that we don't use API functions: they can change.

After we have a stable upgrade path, we aren't planning to change the API unless there's a really really good reason to. I think the chances that we will ever again change the values of these constants in D7 are extremely slim and probably zero. And if we did, there is no reason we couldn't go back and change the update function to use explicit integers at that point.

(5) You are confusing the field module for the field storage module.

Partially I was. But this part I was still correct:

+  module_invoke($field['storage']['module'], 'field_storage_create_field', $field);

We are always guaranteeing in the code above it that $field['storage']['module'] will be 'field_sql_storage'.

And in this part:

+  module_load_install($field['module']);
+  $schema = (array) module_invoke($field['module'], 'field_schema', $field);

As long as $field['module'] is installed, the module_load_install() is not necessary (since the update system already loads all installed modules' .install files, regardless of whether they are enabled). And I assume we can't support adding a field for a module that has never been installed. The module_invoke() is OK though, agreed.

(9) That's for another patch.

Agreed.

To specifically address David's remaining concerns:

1. We do not use constants to discourage contrib from doing so. Just because these two constants happen to be defined all the time, most constants will be in modules which we do not know to be loaded.
2. We load the module install to make sure it's loaded. For contrib, It very well might or might not be again. Even if it's overkill, there is no harm in doing so. I like overkill in case of updates. You can't be too cautious.

As this contain the upgrade path utility functions can't understand how this became noncritical.

@chx: http://api.drupal.org/api/function/drupal_load_updates/7 loads all the .install files, doesn't matter if it's core or contrib. So unless we support this for uninstalled modules, we don't need to load it explicitly. Anyway, this is a minor point; I agree that in general the patch is RTBC. I'm still finding the "This function is valid for a database schema version 7000" PHPDoc a bit confusing, but perhaps we can improve on it elsewhere.

I think from #60 @webchick preferred that we add back the stub comment_update_7013() function here and discuss removing it elsewhere, so maybe we should do that? (Although core is not consistent with this currently - #898536: Clean-up the upgrade path: dblog went in very recently and did not leave any such stub functions behind.) I went ahead and created the general issue to discuss that: #909272: Remove stub update functions and renumber the remaining ones

The reason this was downgraded from critical is that the comment upgrade path was not actually broken. The utility functions could also have been added in other, critical issues if they are needed there. Anyway, whatever. Let's just get this committed :)

David, text module is not installed in D6 system_update_7027 enables it what if it's the same situation with a contrib? So yes: we might need to support this for previously nonexisting modules.

Thanks. This looks like a good clean-up to the previous patch. If both Acquians and Examinerians are in agreement on #909272: Remove stub update functions and renumber the remaining ones, then I guess who am I to argue? I just ask that we get this clean-up done quickly so it doesn't unnecessarily postpone a beta release.

Committed to HEAD.