Cannot upgrade Drupal core if the comment module is enabled

Text is a required module, so I don't think that we should need a dependency.

But maybe the required core modules should install first. Otherwise we might have to keep adding more of these dependencies. Or it could be an issue in comment.install

@das-peter The dependency system uses graph.inc to figure out the order modules need to be installed. This is a great system. What we should do is actually declare the dependencies like you did for comment module and let it do what it does. Bolting on some solution on top of a dependency mapping system so we can try to avoid declaring dependencies in some cases is just not pretty. The dependencies make sure modules are installed in the correct order and provide a level of documentation on what dependencies a module is using.

I'm marking this as critical because basic install profiles just break in some cases.

The attached patch adds a bunch of dependency mapping that will provide graph.inc the level of detail it needs during the initial install process. For comment and fields anyway.

Edge cases are not release-blockers.

Sadly, this isn't as much of an edge case as it looks. The minimal profile doesn't install much so we don't see it there. The standard profile is lucky enough that we don't see it there.

If a profile has comment module but not color module as a dependency it will fail to install. If the following profiles were ported to D7 this issue would cause their installation to fail:

• Open Atrium
• Open Publish
• Well, some others

Is this really not critical?

For anyone interested in replicating the problem with installing comment module. In the standard.info file (part of the standard profile) comment out the dependency on color module. Then try to install the profile.

Properly defining dependencies is a good idea for a multitude of reasons. Also think updates.

Committed to CVS HEAD. Thanks.

I can't upgrade my site.

Upgrading from D6 to D7 should not check if new modules are installed.

add tag

Hm. Actually, we have end-to-end tests for the upgrade process. Why aren't those failing?

The upgrade path should (in theory.. i'm not sure of the implementation) be able to handle enabling new dependencies if they are available.

Oh fun. We are uncovering bugs all over the place :). Subscribe.

Moving to base system and re-titling.

So, the comment module now correctly depends on the text module, but our update process doesn't allow you to continue without first enabling the text module, and we cannot enable the text module without running the update process first.

The check was introduced in #228860: Upgrading a module with a newly added dependencies breaks things badly. I'm not sure of the way forward here except completely rolling back this other patch.

We discussed with catch on the IRC, and we don't see this working without removing the 'disabled dependency' check added by #228860: Upgrading a module with a newly added dependencies breaks things badly.

• We cannot automatically enable dependencies during the update process, because that would be unreliable (those would be enabled before the real upgrade is performed...
• Currently you cannot enable those modules at all manually (except by manually messing up with the database)

As a consequence, the plan could be:

• Remove the 'disabled dependency' check
• Document that a module that defines new dependencies needs to call module_enable($my_new_dependencies) during its own update process. Those modules are guaranteed to exist and to be in the correct versions because of the checks in system_requirements().

I'll roll a patch.

Here's the patch, works for me locally. Not sure where the 'enable new dependencies in hook_update_N() should go apart from update docs.to a

Added the module_enable() to comment_update_7000() and just moved the variable deleting to a later update function rather than re-numbering just for a few variable_del(), hope that's alright.

I would argue that we don't actually need the change in the comment module: those modules are enabled by the system module during its own upgrade process.

Hmm, was thinking it was worth it for documentation even if redundant, but instead I just modified the comment in comment_update_dependencies() slightly.

This makes sense on so many levels. The check is completely unnecessary and doesn't make any sense. We already run updates for modules that are disabled. Why would a disabled module's dependency prevent the upgrade from running but the module being updated may be disabled itself? It doesn't make any sense that the update would fail if both are disabled.

Note that other checks remain in place, including checks to make sure the dependent module actually exists. This patch removes the unnecessary check.

This looks good for me too.

Coming from the other issue, the reason this check was introduced was modules that were available but not installed.
Example:
New version of module A depends on module B. Module B is part of project C, which the user has downloaded for a different reason.
In #228860: Upgrading a module with a newly added dependencies breaks things badly it was argued that it might not be good to blindly enable module B, and install its schema with it, because the user should be given the choice which modules to enable. Maybe he doesn't trust module B or it conflicts with module D etc.
I haven't really read this issue enough to know whether this is only about (re)enabling previously installed modules or whether the above use-case is actually flawed in some aspect, just throwing this out there so that it does not get overlooked.
Hence, setting to needs review.

We had a test for this!

Fixing the upgrade path seems more important to me, but it we need to think about #28 some more.

Talked to DamZ, and decided this was ready. Committed to CVS HEAD. Thanks all!

I think a related problem: #890210: Cannot enable Comment module after a Drupal 6 to Drupal 7 upgrade