Updates run in unpredictable order

KarenS: modules which are not *enabled* are updated, not modules, which are not *installed*. I don't remember having updates running in predictable orders anytime.

I wonder how dependencies will be resolved for disabled modules (which could depend on modules which are not even in the file tree, so we cannot track their dependencies).

OK I have no idea how to fix this issue in core, but maybe this might be a potential workaround for cck.

There was a unique index introduced after RC1 that had to be rolled back and replaced. Because we can't know what the schema is in core and had to deal with updates with two potential schemas, a variable_set() was put into the first update, then subsequent behaviour was dependent on that. This effectively makes one update dependent on another. If you know exactly which module requires the update to have been run already, the respective update function could also delete that variable. If it's one update dependency for multiple dependent modules, then I guess the worst that happens is a stale variable gets left in the table.
Patch and discussion is here upwards: http://drupal.org/node/164532#comment-679172

Also, I don't see that this is that much of a different situation to the 4.7 cck to 5.x core content type creation (whch iirc relied on users following instructions correctly), or 5.x update status to 6.x core update module - which requires a full uninstall run on the contrib before upgrading. Obviously it's better not to have to worry about users following instructions, but both came to mind reading this.

Well, we never had ordering / dependency support for updates, and this is indeed sad to be missing. But we did live without it so far and being a totally new feature to implement I'd suggest you depend on some existing technology to implement it when you need:

- http://drupal.org/node/208602 is not committed, so #abort-ing updates of one module is possible
- you can check the schema version of a module in your update functions, to ensure that a module you depend on already run its updates *to the point you require them*
- you need to have your users run update.php multiple times to get through this, or we can build in something automated at the end of update.php to look again for update functions to run, but that would possibly go into an infinite loop with #abort-ed updates being tried to run again.

Looks like you can do this with the current tools, and no dependency ordering would help you require specific schema versions, which you might not get with #abort taking effect even with some kind of consistent update ordering.

Or to put it differently, if module B updates run after module A updates, but module A aborted some of its updates, you are screwed again. You need to look for the schema version anyway, which requires some specific code on your part, but leads to the expected result instead of more failures in this case.

gabor - you correctly say that #abort can cause problems but i think this will be very rarely used if we guarantee the order in which updates are run. modules with pending updates should be considered unreliable until their updates are run. we are reducing the meaning of dependency to mean "except when running update.php".

my inclination is to fix this for 6, even at this late hour.

Moshe: so you would not run updates for modules which depend on modules for which updates were not run / are not runnable? I think would be just a small step in the right direction, since often we make assumptions about a specific schema version of a module being already there, but I am looking forward to non-disruptive ways of solving dependencies in the update as well. There is no such feature to mark this as a bug of, so at best, this can be made a task.

"There is no such feature to mark this as a bug of, so at best, this can be made a task." - one could say it is a bug in the dependency feature.

I'll ask a naive question - how come we can guarantee module run order during install and not during update. i would think they are the same problem. karen says "the dependency must be observed even when the modules aren't installed". whats hard about that? dependencies are declared in .info file which is always safe to load.

The difference IMHO is that in install we expect a clean slate, while in update, we always go from one version to another, so the beginning of the road is also "unknown" which results in more error conditions. Lamenting on whether this is a bug or not, does not move this forward anyway. Dependencies were never a feature in updates, so I would not call them a bug, but I am not interested in arguing about it, so let it be.

Moshe redirected me to this issue from #220945: patch: drush mm enable/disable/dot/uninstall where I try to enable/disable modules. It was a too quick patch for drush, but atm i'm reverse enginering D5 to make it work ;-)

Afaik this dependencies stuff is about directed graph.That is walk through the graph start to end or the other way around (depending on dependency or reverse dependency) according to a 'Topological Sorted List' and halt on failure.

Whether installing modules or enabling or updates (backwards) disabling/uninstalling (forward) them. For a graph example see the aforementioned issue or http://build2be.com/files/build2be.com/ryan.png (ubercart).

I want to contribute to this issue but don't know how to start. So some feedback is welcome.

Regards,
Clemens Tolboom

Talked @ FOSDEM to Gabor and Gerhard. Their advise was to first patch D7
... so i will try to deliver a patch for D7
... guess i have to provide this for install.php, update.php and module.inc
... that will take some time :-/

Hm, there is a graph implementation already in the install code as chx pointed out (he wrote the code). Look into what happens when you enable a module which has dependencies, all dependencies are nicely installed as well on the web interface in Drupal 6. So drush could use that in Drupal 6 or even backport that code for Drupal 5 for use. The same is not run on running updates though, and this is what this issue is about. I am not sure how the drush operations relate to update ordering.

Update.php function update_batch() has a simple for each in preparing the batch. This should not be the case!

$operations[] should contain the batches in a topological sorted list (TSL) order. This way a dependant module gets the 'right' updated dependency when updating itself.

In order to do this some changes should be made to modules.inc to provide a TSL of all modules choosen to update.
Ie function module_get_tsl( $modules = array()) which will according to 'dependencies' builds the right order out of the given list.

I've done this in my drush patch for mm for enabling/disabling/uninstalling and it works great.

I'm in preparation to document this idea.

I documented my thoughts http://build2be.com/content/module-management-drupal without getting much response.

Just talk to chx a little about DFS, TSL and yes I could try to provide a few patches. But what I want is a little more: rewrite install.php, update.php, module.inc and system.inc to streamline all module related code into module.inc. And I guess that's to far fetched to do.

I made my drush patch for D5 a real module drush_mm (not only) to make it easier to test this issue against.

@KarenS What is the recipe to reproduce this issue?

We ended up using the #abort solution for cck field modules - all contrib fields should do the same :-( See #304813: CCK module developers read this!!

In #310898: Making module_enable and module_disable API functions module_enable and module_disable do not run in a unpredictable order. I implemented a TSL based on the modules dependency graph. Maybe this can be used here too?

If you need a workaround to ensure that a module like CCK has its updates run first (without requiring other dependent modules to add any code of their own), you could just do that via hook_form_alter. For example:

function content_form_alter(&$form, $form_state, $form_id) {
  if ($form_id == 'update_script_selection_form') {
    // Preserve the Drupal core behavior where system.module's updates run first.
    $form['start']['system']['#weight'] = -100;
    // Make sure CCK's updates run next, before any others.
    $form['start']['content']['#weight'] = -99;
    // (You could clean up the above code to make this more robust, but it's probably good enough as is, since other modules will default to #weight = 0.)
  }
}

It's really ugly, but as far as I can tell from testing it, it works fine. Seems like it might be a much better approach than #304813: CCK module developers read this!! (is it too late to matter)? You could also probably use unset() statements in the above code to prevent dependent modules from running updates if they are not enabled (the other issue that CCK is facing).

(I've been banging my head against this all day for Acquia, where we are considering in general how we might have to deal with complicated update dependencies in the Acquia Drupal distribution, and so far this is the solution I have.)

Presumably a real solution would go in Drupal 7 now.

In thinking about a way to fix this for D6 that involves minimal changes, one possibility is that Drupal could continue to run updates exactly as it does now, but just before running them, it could simply invoke something like drupal_alter() on the list of module updates. This would allow CCK (or any other module) to implement a function in its install file that reorders the updates, enforces dependency checking, etc. For an example, see the patch I posted at #304813: CCK module developers read this!!... which more or less does this already for CCK, but since there is no hook it needs to use a very sketchy method to "intercept" the list of module updates, and that is not really good.

Any thoughts on whether something like this kind of drupal_alter() could make it into Drupal 6 as a bugfix for the issue here? It would be completely nonintrusive (would not change Drupal core's behavior at all), but it would be adding a new hook to a stable release... not sure if that's a no-no or not. (Note by the way that it couldn't use "drupal_alter" exactly, since the hook would need to be called for disabled modules also. But that's not really a big deal.)

From the complex upgrade experiences we have it looks like it would indeed be a good idea to try and fix this in a backwards compatible way. We should however sketch up a few scenarios. How would multiple modules "interact" on the chain of alter calls to form a list which is fine for all? To me it sounds like the modules know rules that "I should come before these other modules" (and the list of those other modules is dynamic depending on the site, eg. all field modules); or "I should come after this module" (eg. after a base module of a module suite). If we only provide a simple alter, modules altering the rules might do changes which go against requirements set by previous modules possibly, right?

Could we perhaps use the dependency system here - so core runs first, then modules with no dependencies, then modules which depend on those modules, then modules which depend on them etc. etc. - Are there any situations where that pattern would be inappropriate?

Yes, if modules run updates on Drupal 5 database tables for example on a Drupal 6 site. Take image module. It does not have any 5.x-2.x table release, but it does include db updates for 5.x-2.x in the Drupal 6 version. So you are likely used 1.x stable and now update to 6.x, but all your images will be lost, since the image module tries to run updates which should be run BEFORE system module's updates (system module updates the core tables). Although system module does not depend on image module in any way :)

How would multiple modules "interact" on the chain of alter calls to form a list which is fine for all? ... If we only provide a simple alter, modules altering the rules might do changes which go against requirements set by previous modules possibly, right?

Well, to some extent, this is a potential issue with many Drupal hooks, right? Any module that implements something like hook_nodeapi() or hook_form_alter() can do a whole lot of damage to other modules if it chooses to :) So maybe the solution is just to say that any module implementing this hook should be careful to only affect modules it actually cares about, and if it doesn't, it should be filed as a bug against the module implementing the hook...

For example, the code I proposed in #304813: CCK module developers read this!! is definitely too "greedy" in that it forces system.module to always run first and content.module to always run second. It should instead be rewritten so that it checks module dependencies and only sorts CCK to be in front of modules that depend on it, rather than in front of any module at all.

Perhaps to facilitate this process, the results of module_rebuild_cache() could be passed along with the hook, so that modules have an easy reference to figure out module dependencies they might need to know, etc.

Well, the consequences of wrong update function order are a bit farther reaching then the consequences of nodeapi hooks not runnning well. Anyway, let's see a dependency based update function order.

Marked #168018: Module dependencies not enforced on update as a duplicate, subscribe, and IMHO this is critical.

Not sure this is "we cannot ship without this fixed", but it is really close.

For me the solution looks quite trivial :0 Let me illustrate this by an example (guess that's not complete so please reply)

Say we have just _installed_ a new filefield and enabled it and _afterwards_ installed a new drupal version _and_ updated views. Futhermore the *.install have a new

<?php
 function hook_install_dependencies 
?>

<?php
function filefield_install_dependencies() {
  return array(
    // ie patch for new views schema change
    "6003" => array(
      "views" => array(
        "after" => "6005"
        "before" => "6006"
      ),
    ),
  );
}
?>

<?php
function views_install_dependencies() {
  return array(
    // ie patch for new node teaser short field
    "6005" => array(
      "node" => array(
        "before" => "6025"
      ),
    ),
  );
}
?>

By laying out the dependencies named in hook_install_dependencies as 'module' - 'schema-version' together with the current and latest version and adding all updateable modules we get a graph like below (http://drupal.org/files/issues/update_0_0.png)

Having a graph we can run update.php on it. This example shows a problem though. FileField was _installed and enabled_ before the updated views version was installed. To solve this extra complication the function hook_install_dependencies() must be used with installing new modules too. That way the filefield module wasn't even enabled before the new views arrived.

For module dependencies _without_ a hook_install_dependencies we could use the module dependencies graph which is illustrated with the taxonomy and forum module.

A better hook name would be hook_schema_dependencies because it's about schema (version) dependencies.

Because we already use the metadata from the docblock during the update process, why not putting @requires tags in that:

/**
 * Upgrade FileField to Drupal 7.
 *
 * @requires content:7001
 */
function filefield_update_7001() {
  // Do your stuff
}

In #27 the image is broken so here is a new version.

@29 Good point but we need more then require right? Hope the image helps

I like the docblock suggestion. But do we need #393068: Make the registry class hierarchy, interface and docblock aware for that?

Anyway: a given module must tell _both_ a require and a _reversed_ require. Better words are maybe: depends and rdepends. It is ie not up to views to tell filefield what schema(s) to use. See #27

Anyway ... I'll try to device a test for this first. graph.inc does not have a Topological Sorted List TSL) but a weight assigned. That's kinda random TSL :p

@clemens: graph.inc does generate a topological order (as soon as the graph is not cyclic), or something close if the graph is cyclic. What makes you think otherwise?

The update path is broken because of this (see #583226: Update broken: Call to undefined function field_attach_create_bundle()). We really need to sort this one out.

This patch implements @requires and @blocks directives to the update functions. I tested the logic locally, but we need to add automated tests to that.

Added an example dependency between system_update_7038 (enable the field modules) and comment_update_7005 (create bundles for comments).

A quick review first:

I did not think it was that easy :) Cool.

@requires versus @blocks
- I think blocks is a weird term.
- In #27 I popped before/after which are at least each opposites.
- Maybe depend/rdepend (debian) is better
- Your code doc states: forward/backward
-- I like before/after

Because the functions are taken from the doxygen we need at least a function_exists? But guess batch API solves that?

The requires are nicely listed on 'pending updates' ... there is something with newlines. My try with two looks like

7007 - Create comment Field API bundles. @requires system_update_7038
@requires ystem_update_7038 

I'm writing a test atm. Hope to finish soon.

I get errors array key 'module' does not exists.

The comment example update @requires system_update_7038 leads to _adding_ 'system_update_7038' to the graph _and_ into the batch which is not required! It should be just a claim 'I need system in schema version 7038 _or higher_'

That explains the error. So only the modules in the batch $start should be in the final batch?

I'm unsure about the exact input $start but

Do we have to test for $updates === FALSE ?

  // Set the installed version so updates start at the correct place.
  foreach ($start as $module => $version) {
    drupal_set_installed_schema_version($module, $version - 1);
    $updates = drupal_get_schema_versions($module);

====
This test $version > $max_version cannot happen right? Unless it's shortcutting FALSE?

    $max_version = max($updates);
    if ($version > $max_version) {
      continue;
    }

====
afaik we cannot choose anymore what version to upgrade to so $update < $version will never happen.

Hmmm are we missing the latest version?

    while ($update = array_shift($updates)) {
      if ($update < $version) {
        continue;
      }

@clemens.tolboom: no, there is no issue there, a module that is not installed cannot be updated.

afaik we cannot choose anymore what version to upgrade to so $update < $version will never happen.

There is no UI to select the version by default, but the underlying code does and should still support this functionality. The idea is that a contrib module could restore the confusing select boxes.

I moved the graph functionality into its own function so we can write a test for it without having to run the batch.

I added too many TODOs :(

We now have a ordered schema update but still not a correct update scheme. A @requires system_update_7038 is now just used as an ordering principle but not as a validation to "Is this update really possible". But that is up to update.php form(s)

Furthermore in writing this comment I realize we need to add _all_ versions of the included modules through @required / @blocked into the graph.

Added some tests for function _update_calculate_graph.

I atm don't know how to run the batch directly.

Can we change @requires and @blocks?

I really am still puzzled about these. I propose

@before system_update_7004
@after views_update_7003

I guess #521838: Clean up drupal_get_schema_versions() got this broken.

Not sure what yet. #40 ran successful once :(

A new patch for #521838: Clean up drupal_get_schema_versions() is committed again.

#40 works fine now

Code style fixes, need to add validation in update.php

missed some tabs

This changes to @before and @after for clarity/consistency.

I'm also not really sure what validation is needed, unless we modify update_get_update_list() to build the tree and sort dependencies there?

Here's a patch that adds an extra stage after the selection (non)form. This will show any incompatible updates and then allow you to apply remaining updates. Please see the screencaps for a better example. I've cleaned the strings up a little bit since taking the screencaps.

Images are missing somehow for me.

i don't think the test failures are related....

mikey_p: What do you think about the feasibility of testing for missing or circular dependencies? When I tested it at the Drupalcamp PDX sprint, these cases threw exceptions. (mikey_p pointed out this is really an edge case within an edge case, and I agree updates don't need to "work" in this case, just that it should fail in a predictable way if graph.inc can't create an update plan).

On the whole this code seems to work as advertised. Tagging for API docs.

The function I added ( update_check_dependencies() )to check the dependencies and invalidate update functions whose dependencies are not met, will definitely end up in an infinite loop on circular dependencies.

To properly test this out, add some additional update functions to an enabled module, check that update.php picks these up and then add an unmet dependency to one of them, such as @after system_update_8001 and try to submit the batch in update.php it should pop up a page similar to this:

Of course it should still run remaining updates, but not anything with unmet dependencies.

This could definitely use some review of the strings and such.

tagging

Patch contains:
- two new test cases for wrong update scenarios with update_test_3/4 modules. The test are commented out. But I think we should have these as working tests.
- bug fix of update_test_2.module
- moved some documentation from function _update_calculate_plan back into update_batch

We still have a few TODO's which should either get explained or removed.

Furthermore I don't thinks circular dependencies are an edge case. It is just 3 contrib modules away :p

My quick effort in checking for circular dependencies based on http://drupal.org/files/issues/module_enable_disable_310898.patch (isCircularMember) failed. It throws always an exception :(

  // First we need all participants
  $participant=array();
  foreach( array_keys($updates_graph) as $from) {
    $result[$from]=1;
    if (isset($updates_graph[$from]['edges'])) {
      foreach( $updates_graph[$from]['edges'] as $to) {
        $participant[$to]=1;
      }
    }
  }
  $has_circular_member=FALSE;
  foreach( array_keys($updates_graph) as $from) {
    if (isset($participant[$from])) {
      unset($participant[$from]);
    }
    else {
      $has_circular_member=TRUE;
      break;
    }
    if (isset($updates_graph[$from]['edges'])) {
      foreach( $updates_graph[$from]['edges'] as $to) {
        if (isset($participant[$to])) {
          unset($participant[$to]);
        }
        else {
          $has_circular_member=TRUE;
          break;
        }
      }
    }
    if ($has_circular_member) {
      break;
    }
  }

  if ($has_circular_member) {
    throw new DrupalUpdateException("Circular updates.");
  }

I don't think this needs circular dependency checking. Any such dependency would be a bug in the code of items that form such a dependency IMO.

This is great. However, when I run it with #563106-32: Cannot upgrade from Drupal 6 to Drupal 7 - meta issue, the comment update is ran first, before the system update which should be the first one to run.

comment module
Update #7000

    * Comment order settings removed.

system module
Update #7007

    * Inserted into {role_permission} the permissions for role ID 1, Inserted into {role_permission} the permissions for role ID 2
...

The patch attached fixes some notices during the update process.

@scor: I'm not sure what you mean. This seems to be by design. The only constraint in that patch (we probably need more) is that comment_update_7005() needs to run after system_update_7038().

subscribing

the comment update is ran first, before the system update which should be the first one to run.

Yeah if I'm understating this system properly, there is absolutely *no* default ordering, other than updates that specifically specify they need to run before or after other functions. So system.install running first is not guaranteed in any way.

This patch adds a few more @after commands so that (in my testing) core can be upgraded when combined with #563106-41: Cannot upgrade from Drupal 6 to Drupal 7 - meta issue. Is there a reason why the tests from #56 were dropped?

Actually at the top of update_get_update_list() we attempt to set all system.module updates to run first. We also set this up to be displayed first in update.php in the validate function, I think also wherever the updates are listed.

@quicksketch : We definitely need these test. See ie http://drupal.org/node/521838#comment-2081246 and further down.

Hope someone can put these back in :)

the comment update is ran first, before the system update which should be the first one to run.

as mikey_p said in #64, I was referring to the comment at the begining of update_get_update_list()

  // Make sure that the system module is first in the list of updates.
  $ret = array('system' => array());

which should be consistent with the order in which the updates are actually run (unless there are before/after commands which move some update before any system.install update function).

oops, the tests dropped of the patch, I forgot to add them to bzr. I've added them to this patch.

I tried the patch #563106-41: Cannot upgrade from Drupal 6 to Drupal 7 - meta issue with this one on two Drupal 6 sites update with success! I got some failed updates due to the upload update but this dealt with in #563000: Provide upgrade path to file and does not block this patch.

This patch is still green. Does it just need review? we need to sort the upgrade path soon.

The resolution is quite advanced and impressing. The code style needs some work though.

However, I've also read through the entire issue and it doesn't look like we really explored the option of adding the - in reality presumed - dependencies between required core modules to .info files:

system <- field
            ^- node
                 ^- comment
                 ^- ...
            ^- user
                 ^- openid
            ^- file
            ^- ...

This issue is not about running ALL the updates of a module in certain orders (we can already do that with the module weight), but about running individual updates in certain orders. It's entirely possible to have two modules that need to have some updates run earlier and other updates run later than a certain module. Fore example system module might need to run upgrades 7000 to 7030, then run the node upgrade 7000, then run system update 7031. Unless we're interested in making an entire list of dependencies in the .info file every time we need such a update, then the .info file won't be very suitable.

This issue in about *.install

We still have ill tests ie testCircularDependencies is commented out.

There are still TODOs in the current patch. These should be resolved/removed.

Tried to upgrade a site from D6 -> D7 which failed, led me here. Subscribe.

We still have to accept/remove TODOs and validate our tests

I'd really like to see this isue fixed before the first alpha.

However, I really do not like using PHPdoc properties for this; it's totally unlike anything else we do in Drupal. I realize that we pull the description from PHPDoc (another thing I'm not totally crazy about), but this is merely a descriptive property; the code here ends up embedding actual functionality into documentation, which everywhere else in core is optional.

In Drupal, we provide metadata in _info() hooks, so we should do so here as well.

I'm super stoked though to see the tests for the upgrade system as part of this patch!

#563106: Cannot upgrade from Drupal 6 to Drupal 7 - meta issue is a good use case for this issue, and also one which is dependent on this very issue...

Running update.php with #66 and the #563106-53: Cannot upgrade from Drupal 6 to Drupal 7 - meta issue gives this error:
The node_update_7000 update can not be run. The following dependencies could not be met: node_update_7008, node_update_7007, node_update_7006, node_update_7005, node_update_7004, node_update_7003, node_update_7002, node_update_7001, node_update_7000, taxonomy_update_7002.

@joshuarogers Are you planning on adding back the validation step from update.php?

What happened between patch #66 and #80 ?

The only change needed was about remove doxygen code right? Where is ie drupal_depth_first_search call gone?

For the hook example I hope this is a bad example? We have a better one in http://drupal.org/node/211182#comment-1755092 (#27)

@webchick: frankly, I would love you to stop derailing this issue. Putting that stuff in the PHPDoc is the correct way to do that, and is perfectly in line with what we *already do* for descriptions.

@Joshua - you have ignored the 'already do' part of Damien's statement. Doxygen for update functions is now mandatory in D7. We already display the function description in the UI. Now we'll make use of more elements like @after. This has the advantage of keeping related information in one place. Further, I can't think of a hook in Drupal where we determine sort order. So, the hook approach is not completely consistent with rest of Drupal either.

In any case, this is bickering over equally valid approaches. We need a working upgrade path more than anything.

I agree with @webchick and @JoshuaRogers.

While it does make sense to keep all the metadata about these update hooks in the same place, I think that's more an argument for calling this hook_update_info() and then adding a 'description' key to it -- and then stop parsing the PHPDoc altogether.

I don't believe the current behavior of Drupal 7 makes much sense in this regard. Why would we assume that the PHPDoc code comments (intended to be viewed by programmers) are appropriate text to display in the user interface (intended to be viewed by site administrators)?

Well we only show them instead of giving people a select list of function names to choose from. I don't think anyone wants to write both PHPdoc and a user-facing description for hook_update_N().

I don't believe the current behavior of Drupal 7 makes much sense in this regard. Why would we assume that the PHPDoc code comments (intended to be viewed by programmers) are appropriate text to display in the user interface (intended to be viewed by site administrators)?

@David_Rothstein: I'm not sure what your point is. The PHPDoc description describes what the function does. It is *exactly* what we want to show to the administrator. We don't put technical details in here (those are for inline comments).

The fact is, Documentation blocks are an integral part of the PHP language. We are not doing anything weird or out-of-band here. PHP natively parses the documentation block of each class and function at the parsing stage. We are using *PHP core functions* to get those documentation blocks: we are not parsing the files directly.

We use standard Doxygen property format in that documentation block (@[property name] [property value]) to store the information we need in that documentation block, and use a very simple regular expression to get that.

By the way, adding meta-data to the code is not at all a new idea. It's exactly what Java annotations and .Net Attributes are (there are even examples in Ruby, for example Thor).

Agreed with DamZ. This is a concise and efficient way to do what we need, it's much more grokkable than the proposed info hook syntax, and it's not a bizarre Drupalism (at least not compared to some other things we do). Also this is something very few modules need to do, ever, so adding a new hook_update_info() or hook_update_dependencies(), when it's only going to mention a couple of hooks, and only be defined by a couple of modules (or duplicate information in the phpdoc for no good reason) doesn't seem to help anything.

The attached text file shows what the update descriptions that are shown to the site administrator (in the UI) currently look like. Many of these make little sense in this context.

They are generated via this code:

  // The description for an update comes from its Doxygen.
  $func = new ReflectionFunction($module . '_update_' . $update);
  $description = str_replace(array("\n", '*', '/'), '', $func->getDocComment());

I do think this is an improvement over D6 (where no descriptions are available at all). But I also don't think it makes a whole lot of sense as it is, so it seems wrong to use it as an argument to add even more things (and this time, actual functionality) to the PHPDoc as well.

We had this discussion already when we first added descriptions to the UI. If you make it a separate hook, developers won't consistently implement it or will curse while doing so. We took an existing practice, and made the code take advantage of it. Thats the highest likelyhood of adoption. It is true that comments for developers might differ slightly in content from comments for end users but its a marginal case IMO. Developers can use inline comments if they care to distinguish the messages.

A separate hook here reminds me of the annoying hook_theme(), where all you really want to do is write your theme function and let Drupal deal with the rest.

I agree that a separate hook for all descriptions would be irritating, and re-using the PHPDoc description, while goofy, is an acceptable workaround for this since it's an optional property (the update will still function properly with or without PHPDoc above it, it'll just lack a description in the UI).

This @before @after stuff though, is actual functionality, which will cease to work properly if the comments aren't formatted properly, or are missing. This is a huge WTF because everywhere else in Drupal, these comments are both optional, and do not control any actual program flow. The fact that other languages that most PHP programmers haven't used may also do this doesn't make it any less of a WTF.

But aside from that, I also think it's better from a DX perspective to have one single list per module all of the update dependencies that exist, so that if I require one from another module, I can see at a glance the dependency tree for that module and choose the proper before/after, rather than sifting through 100+ update functions (in case of system.install) trying to parse out the tree in my head.

cross posted from #563106: Cannot upgrade from Drupal 6 to Drupal 7 - meta issue

am i the only person who feels it is patently ridiculous to hold up a MAJORLY CRITICAL piece of functionality like this (being able to upgrade drupal) on something as silly as a 'boil the ocean' acyclic directed graph approach to granular hook weighting?

we're holding up the 4 line solution to the one occurrence of a problem we have solved in similar ways before on the possibility that we can develop a complex replacement algorithm, less than a week before we are meant to release the alpha ?

we should not be adding new hooks at this point.

adrian: I've tested the graph code and it works, and graph.inc is already in core. I don't think that's holding up this issues. The current discussion seems to be whether to use magic comments or a new hook.
Can you explain more about the "4 line solution"?

Maybe I'm wrong (committed elsewhere) but tests from earlier patches are gone :(

The four line solution is to return abort from the update hook if either a variable isn't set, or a schema version isn't incremented to where you want. That bails out of your update, requiring it to be run again. It's the approach mainly taken in the D5-6 CCK upgrade path. The disadvantage is there's no API for it, and you have to re-run update.php to get the aborted updates to proceed (possibly more than once if there's a couple of levels of dependency).

Personally, I think we should fix the HEAD upgrade path with the variable method first, to stop further regressions getting committed, and to allow contrib modules to test their own updates. Since this issue is only going to be an API addition, rather than a change, and it only affects a handful of modules, and it's still a critical bug, then it's a prime candidate for a minor 'alpha freeze' exception. At the moment it looks like we'll have no viable patch here, and no working upgrade path either, by Jan 15th, which is the worst of both.

Cross-posting from #563106: Cannot upgrade from Drupal 6 to Drupal 7 - meta issue.

I think I'm going to give this issue until Thursday the 14th to get fixed, and if it isn't, we're going to go with Adrian's solution, and document it as a workaround. I agree with him that this is ridiculous.

And actually, reading catch's reply, that sounds incredibly sane. Un-postponing the other issue. If this one gets done in time, great. You can remove Adrian's hack. Otherwise, upgrades can continue to suck in D7 just as they did in D5 and D6, and you can argue with the D8 maintainer about comments vs. arrays.

I give up. This issue has been blocked for no real reasons. This patch has been perfectly working (and tested!) since #34 (September 21, 2009 - 15:06). The rest has just been bike-shedding (first on the name of the parameters, then on the mere idea of storing parameters in the docblocks).

Let's just ship with the ugly work-around.

We absolutely need this functionality to upgrade to D7. Mind you: Core took over the namespace of at least one contributed module. This will lead to problems we can't even imagine yet.

Let's go back to #66, use the Doxygen approach, and be done with it. Just needs a couple of tweaks and RTBC.

I agree with Damien and sun, there is nothing blocking the working patch from #66. It provides a working before/after syntax, and also prevents updates from being run unless their dependencies exist (i.e. Updates with missing dependencies cannot be run from update.php UI) and it has unit tests.

Do we need tests for the UI validation?

Not sure I understand the latest drama here... seems like all we need to do to get this done is take the best parts of #66 and #80 and combine them?

Looking carefully at the code, it also seems possible to make it significantly cleaner and simpler (while still keeping the graph approach and UI validation, and fixing the circular dependency problem).

Having a simpler patch to review will hopefully help get this in much quicker.

I will be working on this, and will have a patch to post this weekend.

"Core took over the namespace of at least one contributed module. This will lead to problems we can't even imagine yet." Actions module did the same in 6.x. Nobody died.

Thanks, David, for taking on this patch!

Here's a start at rewriting this, but more needs to be done - and I also need to put the tests back in. Will work on it more later.

I do agree with Damien Tournoud @ #100 but would prefer #66 over #34

What bothers me apart from Damiens points is that nobody seems to appreciate the tests which were (back) in #66

I do understand core is changed meanwhile but would appreciate these tests where put back :)

@clemens.tolboom: As I said in the comment right above yours: "I also need to put the tests back in" :) The patch is still in progress...

Note that I based this mostly off the patch in #66, but had to make a number of changes in addition to switching from @before and @after to a normal Drupal hook. For example:

• The code for the UI was almost an exact duplicate another part of update.php, I think? So instead of that, I merged the logic into the existing form. I also tried to simplify the UI so we don't provide the site administrator too much information up front - mostly, I want to make sure that this patch doesn't get stuck too much on the UI :)
• None of the previous patches appeared to deal with the fact that update_do_one() has the ability to abort an update and needs to then be able to prevent all dependent updates (previously defined as "all subsequent updates for the same module", but now more complicated) from running. So I had to change some things to fix that.
• The previous patch in #66 had a couple instances where code like explode('_update_', $function) was used to parse a function name, and other things that needed to be cleaned up (as per @todo's in the comments, etc), so I tried to consolidate the code to fix those. That part I still need to work on - but at least I have it down to a single instance of explode(). We need that to be zero :) I think the fact that we are using a Drupal hook might save us here, since we can easily make the hook return data keyed by module and version, rather than by the name of the update function itself.
• Note that moving to a Drupal hook also made things simpler in another way: We no longer need separate 'before' and 'after' declarations, but rather can use a single 'dependencies' key (i.e., the same way module .info files do it). I think that's an improvement.

Anyway, I plan to work more on this patch tonight - that's just a summary of the current status.

@David_Rothstein: we don't need both @before and @after, only one of them is necessary.

OK. This version of the patch is now functionally complete - and heavily documented.

Plus, it seems to work. Granted I've only tried it with an empty D6 database, but I can successfully update from Drupal 6 to Drupal 7 without getting any failures.

I still need to update the tests and put them back in, but the rest of the patch should be reviewable now, since I don't know of anything in particular that still needs to change - unless the tests wind up telling me otherwise :) So I'm putting it at "needs review", but it's still "needs work" for the tests.

Some additional notes:

1. As per discussion in #59 through #66, I did not add any code to ensure that system module's updates always run first. I can see the argument either way, to be honest, so figured we should just leave it as is for the time being. On the one hand, it might be convenient for the D6->D7 upgrade since a lot of modules might depend on certain system module updates, but on the other hand, if we are moving to a dependency system then it probably makes sense not to have "magic" (undeclared) dependencies in the background.
2. I decided not to put in any code for circular dependency checking, since it's really a separate issue that shouldn't hold up this one. We don't appear to be doing it for module installation either (regression from D6?), so if we add it back, it should be for both. At the very least, it seems worth having a followup to add that capability to the Drupal graph API (which seems like it is simple to do), even if we decide we don't want to use it because we don't babysit broken code.
3. You'll notice the patch contains this code:
   

    function system_update_7027() {
      $module_list = array('text', 'number', 'list', 'options');
   -  db_delete('system')
   -    ->condition('type', 'module')
   -    ->condition('name', $module_list)
   -    ->execute();
      module_enable($module_list, FALSE);
    }
   

   which was necessary to allow the update to proceed. The origin of that code is #490074: 6/7 upgrade path broken where the intention was to allow people upgrading from D6 with CCK modules still in their filesystem to successfully upgrade and pick up the D7 modules. I'm not sure that code ever worked, though, and it certainly doesn't now (due to changes in the module installation API - not related to this patch). For now, we can live without it, IMHO. The workaround (if necessary) is presumably to just delete CCK from your filesystem before upgrading.

Please review! Tests coming later.

I'm attaching screenshots showing the UI in the case where there are missing dependencies. (This is very similar to what @mikey_p had in #54, just a bit simpler.)

OK, here is a patch complete with the tests. I've also made a few changes to the API docs for hook_update_info(), based on feedback from @hunmonk in IRC.

This one now actually deserves its "needs review" status :)

gave this a fair walkthrough. code style looks good. the code that builds the dependency array works well. after applying this patch (and the patch at #684084: node_update_7006() broken after 'node titles as fields' rollback) i can upgrade an existing 6.x site to 7.x without any major explosions ;)

also when i add a hook_update_info() invocation to a module that passes an unfulfilled dependency, the selection form properly flags it, the update isn't actually run, and no output regarding the update is present on the results page.

david and i discussed a few enhancements to the documentation which i believe he'll roll into an updated patch.

couple of other things:

1. i think that we should eliminate any references to dependencies on the system module in hook_update_info(). IIRC, system module updates are always run first, so this seems redundant. instead, just indicate this truth in the doc for the hook.
2. '#value' => $incompatible_updates_exist ? 'Apply remaining updates' : 'Apply pending updates', <-- i think this might be overkill. we already provide plenty of context in the error messages at the top.
3. i don't understand where the updates are aborted once the processing actually starts. it looks like here:
   

     // If this update was aborted in a previous step, or has a dependency that
     // was aborted in a previous step, go no further.
     if (!empty($context['results']['#abort']) && array_intersect($context['results']['#abort'], array_merge($dependency_map[$function], array($function)))) {
       return;
     }
   

   but that never seemed to get hit. can you clarify this part of the workflow a bit more?

4. maybe it would be a good idea, instead of completely skipping the output for the aborted update on the results page, to put it in, and highlight it as skipped? might provide nice consistency.

it makes me a little nervous to see a brand new hook just a few days before ALPHA1 ;) however, this patch solves a long standing problem in a way that contrib authors will really appreciate, and this change is happening in the update system, which is completely isolated from the rest of core, so there's no fear about busting anything or throwing a big curve ball this late. frankly, the update path always seems to be one of the last things we get to fixing before a release, so it makes sense that some big changes would happen about now :)

Thanks!

david and i discussed a few enhancements to the documentation which i believe he'll roll into an updated patch.

They were actually already in #111, but they're in the attached patch too :)

i think that we should eliminate any references to dependencies on the system module in hook_update_info(). IIRC, system module updates are always run first, so this seems redundant.

That's the way it works now, but with this patch, system module updates aren't guaranteed to run first - so you do need to specify the explicit dependency. See discussion in #59 through #66 and #109. From a pure "philosophical" standpoint this makes sense, but from a practical standpoint it's harder to say. We could presumably write code to enforce that system module goes first whenever it can, but it might actually be tricky to write (we'd need to make sure not to cause infinite loops in the dependency chain while imposing this).

Perhaps if there's no consensus this decision could be made later? Adding it later won't break anyone's code - all it might do is make some of their existing dependency declarations redundant.

# '#value' => $incompatible_updates_exist ? 'Apply remaining updates' : 'Apply pending updates', <-- i think this might be overkill. we already provide plenty of context in the error messages at the top.

Agreed. I made this change in the attached patch.

i don't understand where the updates are aborted once the processing actually starts... can you clarify this part of the workflow a bit more?

There are two different things going on here.

The "abort" code is existing functionality, where update functions can dynamically throw an exception to bail out. I didn't change that except to make it respect the new dependencies (rather than assuming the only dependencies were within the same module, as it did before). Exactly how this gets called is a little messy to explain, I think, since it happens within the batch API processing. I actually tried to write a test for this, but found it didn't work because I couldn't figure out how to start a new batch from within a test, which is itself running in a batch :) I've tested it by hand, though, and it does work.

The second is the new functionality added by this patch, where you skip an update before it ever runs (if you know beforehand that there is a missing dependency). The logic for that is in update_batch() and is simpler to understand - we just skip adding those operations to the batch before the batch is even started.

maybe it would be a good idea, instead of completely skipping the output for the aborted update on the results page, to put it in, and highlight it as skipped? might provide nice consistency.

Hm, yeah, perhaps. I haven't added that for the time being. That output screen is a bit weird looking right now in Drupal 7 anyway (without this patch), so I'm not that excited about trying to modify it as part of this issue :) Also, this situation (missing update dependencies) probably isn't anything that most site admins will ever see.

Ran through the upgrade on with an older version of webchick.net's database, since that's all core except for Mollom, and we could see what happened with people upgrading from older point releases. Here was my experience:

1. On the first screen after cvs up -dPCA I got:
   

   PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'webchick.cache_bootstrap' doesn't exist in /Users/webchick/Sites/webchick.net/includes/database/database.inc on line 1940
   

   That's fine; there's always something. ;)

2. I therefore couldn't log in to my uid 1 account, so when I tried to run update.php I got prompted to set $update_free_access = TRUE; So far so good.
3. I was informed I had 111 updates pending.
4. The progress bar part went off without a hitch. No errors, nothing. Seemed damn fast, too!
5. At the end I saw I had only two (!) failures:
   filter module: Update #7003
   * Failed: SQLSTATE[42000]: Syntax error or access violation: 1091 Can't DROP 'fmd'; check that column/key exists

   system module: Update #6051 (format field for signatures)
   Failed: SQLSTATE[42000]: Syntax error or access violation: 1064 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 'ADD `users` s(s, s) unsigned NOT NULL auto_increment COMMENT 's', ADD' at line 1

6. I got real hopeful until I went to index.php and got:

   PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'webchick.blocked_ips' doesn't exist in /Users/webchick/Sites/webchick.net/includes/database/database.inc on line 1940

   Call stack:

   0.0009 59844 1. {main}() /Users/webchick/Sites/webchick.net/index.php:0
   0.0102 416404 2. drupal_bootstrap() /Users/webchick/Sites/webchick.net/index.php:21
   0.0124 425744 3. _drupal_bootstrap_page_cache() /Users/webchick/Sites/webchick.net/includes/bootstrap.inc:1519
   0.0262 1253300 4. drupal_block_denied() /Users/webchick/Sites/webchick.net/includes/bootstrap.inc:1589
   0.0262 1253508 5. drupal_is_denied() /Users/webchick/Sites/webchick.net/includes/bootstrap.inc:1445
   0.0262 1253844 6. db_query() /Users/webchick/Sites/webchick.net/includes/bootstrap.inc:1432
   0.0263 1254076 7. DatabaseConnection->query() /Users/webchick/Sites/webchick.net/includes/database/database.inc:2111 
   

Any ideas?

Incidentally:

mysql> select name from system where status = 1;
+-------------+
| name        |
+-------------+
| phptemplate | 
| system      | 
| block       | 
| color       | 
| comment     | 
| contact     | 
| dblog       | 
| filter      | 
| help        | 
| menu        | 
| node        | 
| path        | 
| php         | 
| search      | 
| statistics  | 
| taxonomy    | 
| update      | 
| user        | 
+-------------+

This is after running the upgrade, mind you. Where are the required field modules, etc?

OK, most of this is due to system_update_6051(), which appears to be totally screwed up - it is basically D6 code (not ported to D7) sitting in the D7 codebase, so not surprising it didn't work.

It also looks like #520760: SA-CORE-2009-007 user signature format is related to porting that stuff to D7. So perhaps that issue can take care of fixing this one?

Once that update function in system module bails out, none of the other system module updates run either, which probably explains all/most of your other errors, and definitely explains why none of the D7 modules got installed for you.

So I wonder: If you update the site to the latest D6 first, then update to D7, will it work? If so, then the remaining problems can probably be dealt with in the other issue.

When I updated to the tip of DRUPAL-6 and repeated the steps:

* 103 updates, instead of 111.
* fmd one was still there.
* New failure:

When I go to the front page I no longer get a fatal error, but instead get this on /node or node/XXX:

I can get to /user, I can log in, I can get to /admin, but random admin pages are also giving me that weird "Error" error. (admin/build/block, for example)

However, I'm not sure how much of this is related to the fact that we have buggy update functions since we haven't been able to run update.php in 12 months :P, and how much of this is actually coming from this patch.

Oh, I get it. That's the maintenance page. :) Well anyway, it's really irritating because I can't see what fatal message I'm getting. :P~

I tossed some debugging code here:

    if ($fatal) {
var_dump(debug_backtrace()); # DEBUG!
      drupal_set_title(t('Error'));      // We fallback to a maintenance page at this point, because the page gener
ation
      // itself can generate errors.

And see it's caused by a PDO exception:

SQLSTATE[42S02]: Base table or view not found: 1146 Table 'webchick.block_custom' doesn't exist: SELECT body, format FROM {block_custom} WHERE bid = :bid; Array
(
[:bid] => 1
)
in block_block_view()

tried upgrading a dummy D6 site with #113 and got this warning before starting the upgrade process: This update will been skipped due to the following missing dependencies: taxonomy_update_7002. This is because the taxonomy module was not enabled on my D6 site. update.php could ignore this and simply include the taxonomy update functions (which I did by tweaking the system table), but it assumes the module was properly installed on the site, which leads to

Update #7002
    * Failed: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'd7.taxonomy_vocabulary' doesn't exist

.

btw, I fail to understand why comment_update_7012 (Create the comment_body field) is dependent on taxonomy_update_7002. Maybe a short comment could be added to explain non-obvious dependencies?

@scor
In #34 http://drupal.org/node/211182#comment-2066602 there were added some example @before / @after ... could it be these are examples too?

In #108 http://drupal.org/node/211182#comment-2464478 is said we don't need both directions of a dependency. afaik this is only true for core modules. What if imagefield update 7002 needs to run after cck update 7003 and before 7008? See #31 http://drupal.org/node/211182#comment-1767942

What if imagefield update 7002 needs to run after cck update 7003 and before 7008

then the imagefield update process wasn't designed very well ;)

probably the easiest solution for that is to split up whatever is happening in update imagefield_update_7002 into independent updates.

btw, I fail to understand why comment_update_7012 (Create the comment_body field) is dependent on taxonomy_update_7002. Maybe a short comment could be added to explain non-obvious dependencies?

because if taxonomy module *is* enabled, then it's hook_entity_info() invocation expects taxonomy_update_7002() to already have run.

in reality, there is no dependency on the taxonomy update -- it only looks like it when taxonomy module is enabled, because of what's described above.

there's already a hack in node.install to manually run taxonomy_update_7002() before node_update_7006(). IMO this is a broken hack. i believe the correct fix for this is:

1. place a new update in system module which checks if the taxonomy module was enabled in the 6.x install
2. if so, then run taxomony_update_7002() and mark it as run

this ensures a smooth update regardless of the status of the taxonomy module in 6.x

there's already a hack in node.install to manually run taxonomy_update_7002() before node_update_7006(). IMO this is a broken hack. i believe the correct fix for this is:
- place a new update in system module which checks if the taxonomy module was enabled in the 6.x install
- if so, then run taxomony_update_7002() and mark it as run
this ensures a smooth update regardless of the status of the taxonomy module in 6.x

This is not less a hack. It is foolish to think that taxonomy will be the only module that have to upgrade a base table to match the entity definition.

We need to upgrade all new entities before starting to upgrade fields. The best way to do that is to use both @before and @after: each modules that need to upgrade a base table will have to @after a given update function in the system module (let's say system_update_N), and @before the next upgrade function (system_update_N+1). Modules that upgrade fields would then @after system_update_N+1.

That we need both @before and @after was clearly explained back in #27. Let's just restore that.

That we need both @before and @after was clearly explained back in #27. Let's just restore that.

The functionality was never taken away. See the API docs for hook_update_info() in the patch in #109 for how to achieve that.

In #111 I removed the description + code example of how to do this from the API docs (based on feedback that this is something we rarely want to encourage). If there are enough legitimate use cases, we can add it back to the docs.

I haven't looked carefully at the taxonomy module stuff - my understanding was that update.php will still run updates for modules even if they are disabled? (see http://api.drupal.org/api/function/drupal_load_updates/7) But if we need to move the dependency declarations around and/or reverse them to make this work, then obviously we should :)

Although in terms of disabled modules, here is one line from the patch that might not work:

+  // Now add any explicit update dependencies declared by modules.
+  $update_info = module_invoke_all('update_info');

As written, that will not pick up any dependencies declared in the .install file of a disabled module. We might need to do a custom hook invocation if we need that.

Restoring some sanity in this patch (and fixing #125), and adding the correct dependencies for entities upgrade.

subscribing, a little late

Rerolled to remove trailing dots (via @sun).

Without this patch, there's no way to perform a test update from D6 with alpha1. The code makes sense, contains a lot of docs, and of course we cannot execute hooks for not installed modules. The introduced @before and @after syntax of this patch comes even with valuable descriptions.

So we absolutely want to commit this patch for alpha1.

.

taxonomy module
Update #7002

    * Failed: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal6.taxonomy_vocabulary' doesn't exist

i know we'd like to get this in before alpha1, but with less than a day left, even the approach hasn't been agreed on by everybody, and it's *still* not working to upgrade the most basic drupal 6 site.

I tried the latest patch, both from the old ~April 2009 backup, and got the same results detailed in #114. I also tried upgrading to the tip of DRUPAL-6 first, and then upgrading, and still got the old 'fmd' error, as well as a new one (the one listed in #131).

I'm left with two choices: I can put in the alpha 1 release notes that the upgrade path doesn't work yet, or I can commit a patch that was crammed in at the last second and *still* put in the alpha 1 release notes that the upgrade path doesn't work yet. :P

So let's shoot for alpha 2.

And can someone explain to me why on earth we're going back to that @ syntax again, esp. when it's still not working? It seems like David nailed the explanation in #125. module_invoke_all() may not work for disabled modules, but you still have to parse all the .install files anyway, so what's the problem with a simple foreach loop through that list?

If this continues, I guess I'll need to get Dries in here to break the stalemate.

Damien, thank you for tracking down those parts of the dependency chain. Looks like that's part of the solution, although obviously it seems not all of it....

I think both of @webchick's errors are just plain bugs in the update functions, which obviously do need to get solved, but aren't related to this issue specifically. (And then when any of those fail, all hell breaks loose because the subsequent functions don't run - e.g., I think that's why the block_custom table did not get created; it's in a later update.)

And can someone explain to me why on earth we're going back to that @ syntax again, esp. when it's still not working?

Doesn't make any sense to me either. Doing it via a hook is consistent with the rest of Drupal, better-documented, and more flexible. In addition, you can't put logic in a code comment, but you can in a hook. For example, the case of D6, where CCK wanted to ensure that all its dependent module's updates waited until after CCK's to run. This kind of thing could be achieved with some logic in the proposed hook_update_info() without the dependent modules all needing to do anything themselves. It can't be done in a PHP docblock.

module_invoke_all() may not work for disabled modules, but you still have to parse all the .install files anyway, so what's the problem with a simple foreach loop through that list?

Right, all we need to do is create a function - e.g., update_invoke_all() - that lives in includes/update.inc and duplicates some of the logic of module_invoke_all(). Not a big deal.

I started working on fixing all this but ran out of time, and then found someone else was farther along anyway :) Hopefully we'll have a fully working patch soon. In the meantime, if anyone has any more reviews of the actual overall code (not the details of hooks vs @syntax) that would definitely be great.

the deps still weren't right in that last patch. attached works for me on a mostly core module based local 6.x install, upgraded to the tip of 6.x before the 7.x upgrade. couple of things:

1. system module is configured to run all it's updates first, this makes sense and we shouldn't be redundant about declaring deps. i've removed all the @after declarations that had no impact on updates that were already running after the system module's
2. all we care about these two base table updates is that they run before fields are created. let's run them to as close to the end of the system updates as possible -- this fixes a bug in the last patch where we were still running taxonomy_update_7002() too soon (before the table renames for the module in the system updates)

Let's move the change of the taxonomy tables back into the taxonomy module, where it belongs. I believe that putting that in the system module was an half-backed attempt at working around this issue.

Additionally, moving block table migration and the creation of the new blocks out of system.install too.

Renumber some updates in system.install to prevent a missing dependency in the last patch.

Getting there slowly and painfully.

This new patch:

- adds a forward dependency between taxonomy_update_7002() and node_update_7006()
- fixes a broken query in system_update_7035()

We need to remove the unnecessary system_update_7037(). There is no need to change the name of the box table twice, and it breaks update order.

With this last patch (and a few workarounds for #685486: filter_update_7003() doesn't work for contrib modules, #685572: system_update_7008() is broken, #685892: Upload -> File module upgrade path is broken and #686128: Body-to-field conversion is too slow), I managed to upgrade a copy of the drupal.org website successfully. It wasn't even too slow (it took about 45 minutes).

i'm able to update my local site w/o errors with this latest patch, nice progress!

Any chance we can refactor update_script_selection_form(). It looks like there is more than just UI there (e.g. incompatible updates). We worked hard this release to move non UI processing into update.inc

The newest code looks good - awesome that this works on the drupal.org database!

Moshe, the code in update_script_selection_form() is just to be able to figure out which incompatible updates to label in the UI. All the actual functionality is still in update.inc.

**

Hm, actually, hard to argue with success, but not sure I understand some of the actual dependencies here:

+ * @after system_update_7049()
+ *   Must run after the field module has been enabled.
+ * @before system_update_7050()
+ *   Blocks upgrading fields.

Neither of those two update functions seem to have anything to do with Fields?

+ * @after system_update_7021()
+ *   Must run after all the entities has been updated.

Similarly: system_update_7021() has to do with renaming the "use PHP for settings" permission, so I think this is the wrong dependency? (Also, minor: "has" should be "have".)

+ * @after system_update_7037()
+ *   Needs the block_custom table properly set up.

Doesn't this refer to the old location of that update? The other part of the patch moves the {block_custom} renaming to block_update_7002()...

Also, I assume based on the opinions expressed above (and the specific arguments by @webchick and I in #92 and #133) that this is going to have trouble getting committed while it still gets its functionality from parsing the PHP docblocks.....

I'll volunteer to port the final patch back to using hook_update_info() if we wind up needing to. It shouldn't be that hard.

+1 to hook_update_info() using docblocks possible can cause a lot of errors from typos to cutting-off the docblock by op-code cachers

Is there any chance of rolling a download with this and related patches #141 so we can test the concept or will #146 + #141 be in alpha 2? I ask because I have about 50 sites with various combinations of modules and settings that I could test and some free time until end of Jan. if this plus #141 works, should I wait for #146 or use #140?

If you're just trying to test various site upgrades and see what breaks (as opposed to actually writing code for contributed modules that use this API), it's certainly fine to use #140 while testing. From a functional standpoint, it should be the same either way.

Thanks!

I applied #140 to alpha1 and tested. WSOD that looks like it was inserting sequences.

I tried my previous test of reloading database, deleting sequences, and update.php and still WSOD :-( but down after:
// Install profile hooks are always executed last by the module system
$values['weight'] = 1000;
Nothing in watchdog. System.scheme_version still at 6203. sequences had changed to single row. To help people test the alpha versions, a write to watchdog for every change would be useful.

If I load, delete sequence, then update, it fails in update_fix_d7_install_profile() drupal_load_updates();.

dunno why this got moved to RTBC, sounds like it needs more work.

I am thinking that sequences change will always be a pain. If sequences becomes on value of 591 but there is a user module already up to 870, you are in trouble. Sequences should do something like select id from `sequences` order by `sequences`.`id` desc limit 1 to get the highest value including from add on modules using a sequence. If you are changing the format then you might as well rename it to sequence. If any module accesses sequences direct instead of through Drupal functions, they will get an error message to highlight their mistake.

Glad to see this moving along. My major concern is with leveraging the PHPdoc to capture meta-data. I definitely think we should use arrays as that provides more flexibility, avoids unwanted surprises (e.g. PHPdoc could be stripped by opcode caches) and consistency. So, yes, let's convert this back to arrays.

OK, here is patch #140 updated to go back to the previous approach of using a hook, and introducing update_invoke_all() as explained above.

I also added back the API docs for the hook, sort of combining what we had in #109-#113 for that (since we are now using reverse dependencies a few times in core here, I decided to add it back to the API docs after all, but with a warning not to use it too much.)

This patch contains essentially the same update dependencies as the patch in #140 (modified slightly due to recent commits at #563106: Cannot upgrade from Drupal 6 to Drupal 7 - meta issue). As per #145, I'm still not quite sure I understand them, but we can adjust them later. As long as this patch doesn't cause a regression in the current ability to upgrade D7, let's get it in - and then continue refining the exact upgrade path later.

I also am attaching the interdiff file between #140 and this patch so it's easier to see what changed.

#154 On mine it was 102 of 104 running in a loop. http://drupal.org/node/563106#comment-2524992. http://drupal.org/node/563106#comment-2525096 is a patch.

This patch looks good, but it looks like the 'dependencies' term is redundant. Specifically, is there a reason why we use:

+  $info['mymodule'][7000] = array(
+    'dependencies' => array(
+      'another_module' => 7002,
+    ),
+  );

instead of

+  $info['mymodule'][7000] = array(
+    'another_module' => 7002,
+  );

I'm still not convinced we only need a 'depends' as I said in #120 and not a reverse depends.

When two contrib modules A and B of which B depends on A are independent adding update_N's to their code it could happen that B needs so squeeze in an reverse depends. As #121 suggest it is not possible to split the update. Module B needs this 'reverse depends' right?

Is this an edgecase? I dunno.

Dries (#158):

I think explicitly saying 'dependencies' makes the code easier to understand, and also makes the hook more extensible. In terms of code style, though, it could easily be written more like this:

$info['mymodule'][7000]['dependencies'] = array(
  'another_module' => 7002,
);

Maybe a bit cleaner?

clemens.tolboom (#159):

The patch already has reverse dependency functionality like what you're describing, and uses it for many of the core modules. See the included API docs for hook_update_info(), as well as the actual use cases included in the patch.

(Whether it should get its own special syntax in the array is a different question.... personally I think it shouldn't because in most cases we don't want to encourage it, but I also don't really care that much.)

I suggest we remove the 'dependencies' until we have a reason to add a second key.

Ping? I'd like to roll a second alpha once the upgrade path is working, and would love for that to happen this week.

So upgrade path for #251792: Implement a locking framework for long operations should be in after 6.16 is out

rerolling #156 since a new update function has been added to block.install

In fact the recently committed block_update_7002() assumes a D7 schema, so it needs to run after "Rename {blocks} table to {block}, {blocks_roles} to {block_role} and {boxes} to {block_custom}."

removed the 'dependencies' key level.

--- modules/simpletest/simpletest.info	17 Nov 2009 21:24:18 -0000	1.15
+++ modules/simpletest/simpletest.info	28 Jan 2010 05:30:51 -0000
@@ -35,4 +35,5 @@ files[] = tests/schema.test
 files[] = tests/session.test
 files[] = tests/theme.test
 files[] = tests/unicode.test
+files[] = tests/update.test
 files[] = tests/xmlrpc.test

This hunk was in earlier patches but seems to have gotten lost in the latest ones - without it, I don't think any of the new tests will run?

+function hook_update_info() {
+  // Indicate that the mymodule_update_7000() function provided by this module
+  // must run after the another_module_update_7002() function provided by the
+  // 'another_module' module.
+  $info['mymodule'][7000] = array(
+    'dependencies' => array(
+      'another_module' => 7002,
+    ),
+  );

If removing the 'dependencies' key, both instances of it should be removed from the hook_update_info() docs as well...

I have to say that code like this isn't at all readable, though:

+function comment_update_info() {
...
+  $info['comment'][7005] = array(
+    'system' => 7049,
+  );

"Info" about what? Huh? We need the word 'dependencies' in here somewhere.

It could be in the hook name rather than an array key, but since we already do it via a 'dependencies' key in module .info files, it seems a lot simpler to keep it the way it was, rather than bikeshed it.

I think #167 (after adding back the modules/simpletest/simpletest.info hunk mentioned above) would be good to go.

I think I agree with David on this. I know it's a little extra typing, but not much, and it makes the code a lot easier to read/understand. Dries, are you able to be swayed on this at all?

Though either way, we should rename the hook to hook_update_dependencies(), since that's what it is, whether we're ultimately talking about dependencies or dependents. hook_X_info() is traditionally a 'registry' hook, but we are not registering updates themselves; that's done by naming convention.

Fixed comments from #169. Tested the update API tests of both patches locally and they all pass.

Well if it's hook_update_dependencies() then David's example would look like:

+function comment_update_dependencies() {
...
+  $dependencies['comment'][7005] = array(
+    'system' => 7049,
+  );

Which seems more readable to me?

Sooooo don't want to go down the road of bikeshedding... but....

+function comment_update_dependencies() {
...
+  $dependencies['comment_update_7005'] = array(
+    'system_update_7049',
+  );

Less square brackets, more copy/paste. :P

Dries, can you please pick the one you like best and STOP THIS EVIL TRAIN? ;)

Having looked at the different proposals, I like

+  $dependencies['comment_update_7005'] = array(
+    'system_update_7049',
+  );

the best. In other words, let's go with #173!

Yay! Thanks, Dries!

Well, I certainly tried just using the function names at one point - but the problem is that the calling code needs to know the module and version info of the dependencies separately. Which it could get via some kind of regexp (being very careful not to screw up any modules that have the word 'update' in their names...), but that's a bit ugly, plus any other caller that needs the info that way would need to do the same thing.

Separating it out makes the hook a tiny bit harder to write, but more clean to use - e.g., you can just do print $dependencies['mymodule'] to see all the dependencies for a given module, so on and so forth.

So if we have to change the hook name here, I vote for #172.

Yeah, definitely #172. The actual function names are rather irrelevant here, we need a clean module -> schema mapping.

Ok. That works for me. Let it be so.

OK, this patch implements hook_update_dependencies() as described in #172.

I also added one new fix to profile_update_7002(), which removes the last "failure" message I am getting trying to update from a D6 site with all modules enabled (granted, this is with a mostly empty database).

Since it's already been proven above that this approach works on a complex site like the drupal.org database, I'd suggest we get this basic framework in now, and then can have followup issues for e.g. any remaining fixes, more correct dependency declarations, etc.

Great!! Thanks, David!

Testbot apparently crapped out a few days ago, due to environment issues, so I can't tell if this breaks tests. On the other hand, I think this squarely falls under "It couldn't possibly be more screwed up than it currently is," and I believe this patch incorporates all of the previous reviews.

Committed to HEAD! Marking needs work for documentation in the upgrade guide.

Downloaded from this morning. Ran test. WSOD. Reloaded database. Deleted sequences table. Ran update.php. Reached update page with 106 pending updates and:
Strict warning: Only variables should be passed by reference in update_selection_page() (line 33 of H:\home\example.com\public_html\update.php).
Continue produced:
PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'name' in 'field list': INSERT INTO {queue} (name, data, created) VALUES

Back to the start. Reload database. Drop sequences and queue. Create queue using SQL form a D7 test site. update.php. Ran through to completion. Really nice having the extra line of information telling me which update is in progress. Two failed messages.
filter module Update #7004
* Failed: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '3-0' for key 'PRIMARY'
block module Update #7003
* Failed: SQLSTATE[42000]: Syntax error or access violation: 1091 Can't DROP 'list'; check that column/key exists

Logged in as administrator. No theme. No menu entries. I went to admin/appearance and set the admin theme to Garland. I like Seven but Seven was disabled.
Admin by task is empty. Admin by module has many entries.

Let's please keep general issues about the upgrade path in #563106: Cannot upgrade from Drupal 6 to Drupal 7 - meta issue, and keep this issue only for issues/comments related directly to the new update dependencies hook. I know I confused things above with my own tests; sorry about that.

block_update_7003() starts by dropping index list but there is no index named list. I removed the line. The next line creates index list but the index exceeds 1000 characters when you use fields theme and module as UTF8 so I put module in a separate index.

Documented at http://drupal.org/update/modules/6/7#update_dependencies

I'm guessing this issue still needs to remain open until some followups are filed, though.

Well I think this particular issue is fixed, and we should pick up the rest in the meta issue.

As per #145, some of the actual update dependencies committed here probably weren't correct. I've created a followup issue to straighten them out:

#717834: The dependencies declared in core's hook_update_dependencies() implementations aren't actually correct

When trying to contrib to #717834: The dependencies declared in core's hook_update_dependencies() implementations aren't actually correct I noticed undefined increments of hook_update_N returns from update_invoke_all('update_dependencies');

Digging into core I ended into

function update_invoke_all()
...
  $return = array_merge_recursive($return, $result);

is derailing our effort. According to http://nl.php.net/array_merge_recursive only same string keys are merged.

So these two lines are _not_ merged.

  // from comment.install
  $dependencies['system'][7050] = array(
    'comment' => 7005,
  );

  // from node.install
  $dependencies['system'][7050] = array(
    'node' => 7006,
  );

The second line is appended to the 'system' subarray and get an index 7051 on my system.

A solution would be to use the update function names instead of the current solution like this

  // comment.install
  $dependencies['system_update_7050'] = array(
    'comment_update_7005',
  );

  // from node.install
  $dependencies['system_update_7050'] = array(
    'node_update_7006',
  );

[edit: changed module_invoke_all into update_invoke_all]

So #172 and #175 are not good solutions. #177 has enough motivation not using the full function name. So what's left?

By prepending 'update_' to the update version number we comply to array_merge_recurse and have split the module from the update number.

  // comment.install
  $dependencies['system']['update_7050'] = array(
    'comment' => 'update_7005',
  );

  // from node.install
  $dependencies['system']['update_7050'] = array(
    'node' => 'update_7006',
  );

Wow, what a horrible gotcha. Nice job finding it.

I was under the impression that it could at least be solved with something like this:

  $dependencies['system']['7050'] = array(
    'comment' => '7005',
  );

(i.e., explicitly using a string) but it turns out that apparently PHP doesn't actually check the type here, so that won't work either. #189 seems like the best we can do - unless we want to start rewriting all of module_invoke_all()....

Patch in accordance with #189, i.e. everything changed to 'update_NNNN', plus the scripts used to make it.

The patch misses the essential array_merge_recurse part ... $dependencies['system']['update_7050'] = array(

Applied the suggestion to prepend by 'update_'.

I changed all hook_update_dependencies and system.api.php and update.inc accordingly.

There is one item left
- update_get_update_function_list contains a max function which imho should be removed. The system.api.php contains a hint to only register the highest version.

In #194 I choose to prepend _all_ numbers with 'update_'.

One could argue this isn't necessary. I like the symmetry of the keys and values and hope to prevent coding errors from ie contrib (See ie #191 where only the values are adjusted. ;)

Tried this on todays head.

Notice: Undefined variable: prepend_str in update_build_dependency_graph() (line 1124 of C:\sites\d6d7\includes\update.inc).

The come up with 98 steps instead of the usual 113 on the "apply appending updates" form

I cannot repo this Undefined variable :(

I can explain partly the "98 steps instead usual 113" ... this is introduced by array_merge_recusive in update_invoke_all('update_dependencies') (See #211182-188: Updates run in unpredictable order ... update items are _added_ instead of _merged_

But I expected not that much difference ... we have only 4 modules implementing hook_update_dependencies of which 2 or 3 produce a ghost dependency. See ie http://drupal.org/files/issues/hook_install.png where node_update_7007 or system_update_7051 are not defined in any of hook_update_dependencies.

#194: update_211182_194.patch queued for re-testing.

diff -r1.38 update.inc
>   $prepend_str = "update_";

This assignment isn't indented correctly; it's actually inside the preceding foreach() loop.

Otherwise everything looks good (though the patch should use the unified diff format).

Powered by Dreditor.

Although the basic approach in #194 should work, I'm starting to think it's somewhat unfortunate to change the API again just for this (and to lose the meaning of actually using the numbers). So instead, I tried a different approach in the attached patch - I realized that since we are already invoking this hook via a custom function anyway, why not just customize it a bit more? I also wrote some tests to explicitly check that this bug is fixed.

- update_get_update_function_list contains a max function which imho should be removed. The system.api.php contains a hint to only register the highest version.

We can't remove this because it's there to cover the case where two different modules declare these dependencies, and in that case we need to pick between them. It's somewhat of an edge case, but we do need to cover it. In the attached patch, I've also added a test to ensure that we are :)

Good find. Some feedback on the code:

+++ includes/update.inc	23 Mar 2010 05:19:22 -0000
@@ -1123,16 +1123,10 @@ function update_build_dependency_graph($
+  $update_dependencies = update_invoke_update_dependencies();

The word 'update' is used twice. That seems a bit repetitive. Can't we go with update_retrieve_dependencies() or something a bit more crisp?

+++ includes/update.inc	23 Mar 2010 05:19:22 -0000
@@ -1181,39 +1175,49 @@ function update_already_performed($modul
+ * the integer keys used to represent update function numbers) differs from the
+ * behavior of the PHP function array_merge_recursive(), which is used by
+ * module_invoke_all().

This seems to include more information than what people care for in PHPdoc. Might be better to move this to inline code documentation. If you think this PHPdoc is important for developers using the API, I'd explain why it matters to them and how it affects them. Right now, it probably reads like Chinese to them.

+++ includes/update.inc	23 Mar 2010 05:19:22 -0000
@@ -1181,39 +1175,49 @@ function update_already_performed($modul
+  $modules = db_query("SELECT name FROM {system} WHERE type = 'module' AND schema_version != :schema ORDER BY weight ASC, name ASC", array(':schema' => SCHEMA_UNINSTALLED))->fetchCol();

It would be good if we could add a comment explaining why we order/sort the result set the way we do.

Here's a revised patch with those changes - all good suggestions.

+++ includes/update.inc	24 Mar 2010 23:56:01 -0000
@@ -1181,39 +1175,45 @@ function update_already_performed($modul
+ * This function is similar to module_invoke_all(), with the main difference
+ * that it does not require a module be enabled to invoke its hook, only that
+ * it be installed. This allows the update system to properly perform updates

s/be enabled/to be enabled/

s/be installed/is installed/

+++ includes/update.inc	24 Mar 2010 23:56:01 -0000
@@ -1181,39 +1175,45 @@ function update_already_performed($modul
+              // If we have an explicit dependency on more than one update from
+              // a particular module, choose the highest one, since that
+              // contains the actual direct dependency.
+              if (!isset($return[$module][$update][$module_dependency]) || $update_dependency > $return[$module][$update][$module_dependency]) {
+                $return[$module][$update][$module_dependency] = $update_dependency;
+              }

I have troubles to understand this and other comments in this patch, as they are presuming a lot of knowledge about the internal structure + flow of the update dependency system. I'm not sure whether humans will be able to grok those comments in 2 years.

+++ modules/simpletest/tests/update_test_1.install	24 Mar 2010 23:56:01 -0000
@@ -7,6 +7,33 @@
+function update_test_1_update_dependencies() {

+++ modules/simpletest/tests/update_test_2.install	24 Mar 2010 23:56:01 -0000
@@ -10,9 +10,25 @@
 function update_test_2_update_dependencies() {
...
+  // See the documentation in update_test_1_update_dependencies() for an
+  // explanation of these dependencies.

+++ modules/simpletest/tests/update_test_3.install	24 Mar 2010 23:56:01 -0000
@@ -10,6 +10,9 @@
 function update_test_3_update_dependencies() {
+  // See the documentation in update_test_2_update_dependencies() for an
+  // explanation of the order in which these two modules' updates are expected
+  // to run.

Can we replace those inline comments with proper @see statements in the function docblocks?

117 critical left. Go review some!

Fixed #203.

I revised the comment in the second point. I don't know if that makes it more understandable.

This one should work. Also, I'm pretty sure that "...only that it be installed." is proper English, but I'll defer that to a native. I left it at that for now.

+ * This function is similar to module_invoke_all(), with the main difference
+ * that it does not require a module to be enabled to invoke its hook, only that
+ * it be installed. This allows the update system to properly perform updates
+ * even on modules that are currently disabled.

should be

+ * This function is similar to module_invoke_all(), with the main difference
+ * that it does not require that a module be enabled to invoke its hook, only that
+ * it be installed. This allows the update system to properly perform updates
+ * even on modules that are currently disabled.

If you say so :)

Thanks! Re #207, I think either is grammatically correct, but kept the one from the latest patch :)

1. I don't think we've fully addressed @sun's second point yet, so I've further rewritten the code comments in update_retrieve_dependencies() here, and also added @see hook_update_dependencies() to the docblock.

2. Adding the @see statements to the test update hooks was a good idea, but it at least one case, we still don't want to remove the inline comment, since without it it's not clear why the two functions are even related. So I added back a revised version of that here.

+++ includes/update.inc	10 Apr 2010 21:27:19 -0000
@@ -1184,39 +1178,65 @@ function update_already_performed($modul
+              // If there are redundant dependencies declared for the same
+              // update function (so that it is declared to depend on more than
+              // one update from a particular module), record the dependency on
+              // the highest numbered update here, since that automatically
+              // implies the first. For example, if one module's implementation

If we are already debating it so precisely, I thing 'first' should be 'previous ones'.

Other than that, a very good improvement of the comment(s).

113 critical left. Go review some!

Agreed, thanks!