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

The current implementation contains 3 loops in the declarations.

1. system_update_7050 to comment_update_7005 defined in commment.install
2. node_update_7006 to system_update_7047 defined in node.install
3. taxonomy_update_7002 to system_update_7047 defined in taxonomy.install

In all three it looks to me that the 'before' and 'after' are swapped.

[edit] removed inline image

The image in #2 is wrong. See #211182: Updates run in unpredictable order comment #188 which is about array_merge_recurse not accepting numbers as merge index.

Edit: removed image link ... it's of no use to watch.

Hmmm ... I'm not sure whether my graph is ok :(

Building the full graph with

$modules = module_list();
$starting_updates = array_fill_keys($modules, '7000');

$update_functions = update_get_update_function_list($starting_updates);
$graph = update_build_dependency_graph($update_functions);

did not give me circular loops.

tagging

The graph in #3 was definitely wrong. The attached version is the current graph of update_dependencies. There are now 12 update hooks defined instead of the earlier 9. So is this issue now solved?

I'm not sure what the title implies. The image in #6 show links
- system_update_7021 -> comment_update_7012
- system_update_7049 -> comment_update_7005

One would not expect the definition of
- system_update_7021 -> comment_update_7012
altogether in comment_update_dependencies()

fwiw I created a patch for devel to generate these images #801156: Displays the defined hook_update_dependencies and full install graphs. to get a somewhat cleaner image which is the same as #6 without the _first / _last node.

Ew... those graphs look so tangly and intimidating.

Note that my patch in #890128: Comment body missing if comment module enabled after a content type module (number #38) is changing some dependencies mentioned here, namely taxonomy_update_7002() is now a dependency of system_update_7020(), and taxonomy_update_7004() is substituted for the former dependencies of 7002.

Fixing component en bump prio as this still isn't fixed.

taxonomy_update_7002 is pointing to user_update_7006 making a loop in the graph. It isn't sortable anymore ...

See also #211182: Updates run in unpredictable order #223

This is the same bug as #1072450: Upgrade from 6.20 to 7.2 - Update #7003 still fails.

I would be tempted to mark that one as duplicate (it discusses at least two separate bugs, and doesn't have nice graphs) and mark this one critical. But just cross-linking for now.

Better component.

I reported a new bug #1217648: drupal_depth_first_search (graph.inc) allows for circular graphs ... it's an old comment from #211182: Updates run in unpredictable order for which we need more attention / focus.

I'm bumping this to critical and marking #1072450: Upgrade from 6.20 to 7.2 - Update #7003 still fails as duplicate.

It should be noted that the graph in #9 is (as far as I've understood) created after the patch #3 from #1072450: Upgrade from 6.20 to 7.2 - Update #7003 still fails has been applied: http://drupal.org/node/1072450#comment-4141550

@catch: Might it be a good idea to add this patch here as well? (just for reference).

Hmm we shouldn't be calling user_permissions_get_modules() at all in updates so that is all kinds of broken, but there's presumably no way to fix that with a direct query either.

What tests should break when #15 is applied?

Does the testbot runs an upgrade? afaik we only test the dependencies in update.test. We don't have an upgrade.test right?

The graph with #15 applied is now

We do have upgrade tests, they're in modules/simpletest/tests/upgrade

.

subscribing

+++ b/modules/comment/comment.install
@@ -83,14 +83,16 @@ function comment_modules_enabled($modules) {
+  // comment_update_7005() creates the comment body field and therefore must
+  // run after all Field modules have been enabled, which happens in
+  // system_update_7027().

This comment is a bit unprecise: It's text.module that needs to be enabled. The other modules don't really matter.

+++ b/modules/filter/filter.install
@@ -152,8 +152,9 @@ function filter_install() {
+  // filter_update_7005() migrates role permissions and therefore must run
+  // after the {role} and {role_permission} tables are properly set up, which
+  // happens in user_update_7007().

Could you elaborate on that, please? filter_update_7005() calls _update_7000_user_role_grant_permissions(), which seems to be the reason for this dependency. But user_update_7007() simply creates a weight column on the {role} table and does nothing with the {role_permission} table.

+++ b/modules/node/node.install
@@ -447,17 +447,23 @@ function node_install() {
+  // node_update_7006() migrates node data to fields and therefore must run
+  // after all Field modules have been enabled, which happens in
+  // system_update_7027(). It also needs to query the {filter_format} table to

See above.

+++ b/modules/node/node.install
@@ -447,17 +447,23 @@ function node_install() {
+  // node_update_7008() migrates role permissions and therefore must run after
+  // the {role} and {role_permission} tables are properly set up, which happens
+  // in user_update_7007().

See above.

+++ b/modules/system/system.install
@@ -1681,11 +1681,19 @@ function system_update_last_removed() {
+  // system_update_7067() migrates role permissions and therefore must run
+  // after the {role} and {role_permission} tables are properly set up, which
+  // happens in user_update_7007().

See above.

+++ b/modules/taxonomy/taxonomy.install
@@ -251,26 +251,11 @@ function taxonomy_field_schema($field) {
+  // taxonomy_update_7004() migrates taxonomy term data to fields and therefore
+  // must run after all Field modules have been enabled, which happens in
+  // system_update_7027().

The field and instances that are created in taxonomy_update_7004() use field types, widgets and formatters all provided by taxonomy.module, so I don't really get why this is neccessary.

In general, though, this patch looks really good. I didn't look through all update functions, though, only through all hook_update_dependencies() invocations and the patch fixes everything I could find that is currently wrong with those.

This comment is a bit unprecise: It's text.module that needs to be enabled. The other modules don't really matter.

Well Field and field_sql_storage matter, all other field modules are not required, so I think this is fine.

Could you elaborate on that, please? filter_update_7005() calls _update_7000_user_role_grant_permissions(), which seems to be the reason for this dependency. But user_update_7007() simply creates a weight column on the {role} table and does nothing with the {role_permission} table.

That comes under 'properly set up' IMO, {role_permission} is altered in user_update_7006() etc.

+++ b/modules/taxonomy/taxonomy.install
@@ -251,26 +251,11 @@ function taxonomy_field_schema($field) {
+ // taxonomy_update_7004() migrates taxonomy term data to fields and therefore
+ // must run after all Field modules have been enabled, which happens in
+ // system_update_7027().
The field and instances that are created in taxonomy_update_7004() use field types, widgets and formatters all provided by taxonomy.module, so I don't really get why this is neccessary.

Because adding fields and instances depends on having the field and field_sql_storage modules enabled.

I really think these comments are decent and don't need more detail than they already provide - the purpose here is to leave an audit trail of the dependencies, I don't think we need detail of which specific parts of which update function are the bit being depended upon or why fields and instances depend on the field module. The doxygen for the updates themselves, and the functions they call, are there in case people want to look further.

We could consider adding @see for all update references since they won't be linked from the hook otherwise - but that shouldn't hold this issue up.

David's right about forum_update_7003() in #16, there's an issue/patch at #1220602: Call to undefined function _update_7000_field_read_fields() during Forum update 7003.

In terms of testing this I am not sure what we can do. It passes the current tests, which means that with the upgrade databases we have in the Drupal 7 tests, both the current broken dependencies and these new ones pass, so we didn't make that case any worse.

To add new tests, we would need to create a database (or databases) that fail with the current dependencies and pass with this patch applied. I don't think we have a single example in any upgrade path issue of such a database, and nor did chx's call out on twitter for one produce anything, that leaves the following choices:

- hope we eventually get presented with a Drupal 6 database that fails with the current dependencies, then use that as the basis for a test.

- try to build a failing database from scratch based on the dependency changes here - may or may not be a fruitful task, I don't fancy it personally.

- commit this more or less as is without new test coverage - since it passes the existing upgrade tests. Note that some patches in #1182296: Add tests for 7.0->7.x upgrade path (random test failures) caused the tests to fail, so it's not that our dependencies are untested, they just not catching a lot of cases.

#1 and #2 feel like open-ended tasks to me, if someone wants to tackle them then great, but given the number of hard to reproduce bug reports with fatal errors in the queue at the moment, I'd be tempted to say we should commit this - ideally this week so there is a full month before the next point release, and that would unblock some of the other dependent issues.

More urgent in terms of upgrade path testing for me is #1182296: Add tests for 7.0->7.x upgrade path (random test failures) (which would also test some of these dependencies if they relate to 7-7 updates at all, which I believe forum_update_7003() does).

The worst that can happen with this patch is that the upgrade path throws a different set of fatal errors for a different set of users, to the fatal errors it already throws. While the fatal errors are bad, they're not the same as 'successful' updates that have actually deleted all your comment bodies, so it's a different kind of risk committing it.

Moving back to 'needs review'.

Tagging.

Talked to sun a bit about this yesterday trying to think of ways we could test it.

All we could come up with would be having multiple Drupal 6 databases, with more or less every permutation of modules installed - each with content. Then test the upgrade path for each of those combinations.

With the current database dump system we have that would be hours if not days of painstaking work to create. sun mentioned possibly downloading a 6.x tarball within the test, installing it in multiple ways and upgrading the database that way. This also sounds like a lot of work but has other advantages if it turned out to be viable (like we could test the latest 6.x release instead of whichever release it was when someone first made the database dumps). It'd be a major refactoring of all our upgrade path tests though either way.

I'm not sure either of these should hold this patch up, so I'm marking RTBC for feedback from webchick on how to proceed. Since it is early in the month, we could commit the patch for three weeks to try to unblock some other issues, and prod people to test 6.x updates on dev, then roll it back again if there are regressions or we don't think there was enough testing of it.

fwiw I manually tested this patch on a medium-large Drupal 6 site (that originally started on 4.5). I had to delete a three year old row for image module from the {system} table manually, but apart from that the upgrade went smoothly with no errors (I did not QA the upgrade, but the worst that can happen here is fatal errors etc. usually).

I have not tried upgrading that site without the patch applied, but that confirms that it at least does not only pass with the core tests.

Note the image module issue has nothing to do with this patch, not sure if there is an existing report for that or not.

@David. I'm suggesting we commit the patch this week and leave it in until the week 7.8 is released.

That gives approximately 2-3 weeks to work on that other issue, as well as for people to test upgrades with 7.x-dev and report back if they get fatal errors. If it turns out to be bad, we can revert it before 7.8 goes out.

IMO since people are already getting lots of fatal errors on upgrade due to this bug, it's OK if the worst that happens is they get some different fatal errors with the patch applied. This is very different to issues in update functions themselves which might lead to a 'successfully' upgraded site with incorrect data. If your upgrade throws a fatal, then you definitely know it didn't work.

The more we hold this up, especially on messes like user_update_7006(), the more we hold up making progress on the 3-4 upgrade path issues that are already blocked on this one.

I'm comfortable with the approach suggested by catch in #33.

OK, then. I've been trying on and off unsuccessfully for the past 2-3 days to run an upgrade on a staging copy of the d.o database as David Rothstein suggested (there are some truly 'fun' and unique issues with this database, since it dates from before Drupal 1.0 ;)), but I can always do so after the patch is committed, as well, I guess.

Committed and pushed #15 to 7.x.

Uh. I guess marking fixed? Still feels like it's "needs review", really. :\

JFTR
This issue might be related to #982748: Database Errors trying to upgrade D6 site to D7.