When a module is upgraded, but it has a new dependency from the older version, Drupal does not check for this, and things break.

Better explained via an example:

1. Install Drupal 5.x and pathauto 1.x (does not depend on token yet).
2. Configure it and make sure it is working on one or two new nodes.
3. Replace pathauto 1.x by pathauto 2.x (which depends on token), observe that there are no error messages about the token module being a new requirement.
4. Try to create a node, and see the breakage : token functions not found.

No indication is given that an additional dependency is required.

Somehow core should reevaluate dependencies when a new version is installed, and give an indication to the admin (e.g. a dsm on all pages [or /admin/*] for user 1) that dependencies have not been met.

However, this is currently not possible/easy, because the system table does not store existing versions (versions are only read from the .info file). If we check during update.php, that is not enough either, since not all version upgrades would have a hook_update_XXX corresponding to it.

So, this issue is for a discussion on this.

Although this issue was found on 5.x, it is opened against 7.x because it is the active branch.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

LAsan’s picture

Any news about this?

catch’s picture

Priority: Normal » Critical

This seems critical to me, could easily lead to WSOD.

gpk’s picture

>the system table does not store existing versions
Maybe I'm missing something but is the schema_version stored in {system} sufficient? hook_update_N() could actually do a requirements/dependencies check first and return with 'success' => FALSE and a suitable message in 'query', if the checks fail. If this worked then we can handle the problem within the module :)

Two problems with this:
1. it's a bit of hack
2. you might end up with a partially completed update - if hook_update_N-1() ran, which updated the database say, but update_N failed.

So I guess the better solution would be to check dependencies and hook_requirements() just before any updates are actually run.

Also duplicated at http://drupal.org/node/260993.

dww’s picture

There's a related problem here (which I could have sworn there's already a core issue about, but I can't find it now). In the situation you describe, once you install pathauto 5.x-2.*, if you have token.module in your filesystem somewhere, the checkbox for it will be unchecked and disabled. :( The only ways to actually turn on the new dependency are to: a) downgrade back to pathauto 5.x-1.* or b) manually edit the 5.x-2.* pathauto.info file to remove the dependency on token. Then, you can reload the modules page, and enable the dependency. We ran into much grief with this in project_issue when we converted to using core comments, since we added a dependency on comment_upload. See #18920: make project_issue.module use comment.module. and #186259: module dependency checks for some of the gory details.

KarenS’s picture

I also ran into this when I added a new dependency on Date Timezones in the Date API and then it that module was neither enabled nor accessible so that it could be enabled, since the checkbox to enable it was disabled.

I see a couple related issues here:

1) The checkbox on the modules list should not EVER be disabled if the module is not enabled!

2) As noted above, the new dependency should be auto-enabled somehow

KarenS’s picture

FWIW the workaround I finally implemented (since this is still broken and we're still getting bit by it) is to add module_enable() in my install file to force the dependent module to be enabled. That seems to be working, so maybe something like that could be done by the core update/install process when it finds a problem.

The thread in #3 suggests doing it the other way around -- auto-disabling a module with a dependency, but I think that's more problematic than auto-enabling a module. How can you be sure you'll do that before the module does something that will break your site when it's disabled? Plus what if there are multiple levels of dependency? It could get very complicated to figure out which modules need to be disabled to get back to the spot where you would have been if none of them were enabled in the first place.

And you can't stop and ask the admin at this point because the site is totally unstable with the broken dependency. You could, however, do an auto-enable and notify the admin that it was done.

dww’s picture

FWIW the approach we took in project_issue was that we made our hook_update_N() fail if the new dependencies weren't met. Definitely not the most fail-proof, but since nothing else worked without the schema changes, it's not like project_issue users could upgrade and then forget to run update.php. ;) I don't think that's a viable solution for core, however.

The problem with trying to auto-enable is what if there's a whole new project you have to download and the module(s) simply don't exist in the filesystem?

moshe weitzman’s picture

OG took the same approach as dww when og_notifications introduced a dependancy on notifications module. This would be good to fix in core.

kbahey’s picture

Perhaps as a minimal thing we have a page with unmet dependencies for enabled modules after update.php completes.
There is no way to catch everything there though, since at any point a user can install a newer version of a module (e.g. 6.x-1.x to 6.x-2.x).

Perhaps a hook_requirements that runs when we are on ?q=admin then?

Mike Wacker’s picture

Status: Active » Needs review
FileSize
1.53 KB

Here's a first stab at a fix. It just aborts without an error message if there are unmet dependencies. Take a look at it and give me a list of work items for the next iteration of the patch.

Status: Needs review » Needs work

The last submitted patch failed testing.

lilou’s picture

Status: Needs work » Needs review

Setting to 'needs review' - testbot was broken.

Status: Needs review » Needs work

The last submitted patch failed testing.

Mike Wacker’s picture

Status: Needs work » Needs review

testing.drupal.org broke; remarking as need review

Status: Needs review » Needs work

The last submitted patch failed testing.

deekayen’s picture

Status: Needs work » Needs review

trying out a new testing bot that apparently wasn't working

Status: Needs review » Needs work

The last submitted patch failed testing.

sun.core’s picture

Status: Needs work » Needs review

Nice one! Can we re-roll this sucker?

Mike Wacker’s picture

Status: Needs review » Needs work

I'm looking into it, but it's going to take more than a re-roll. This patch still needs two things.

  1. Dependecies in the 7.x info files also can include an optional version number (e.g. >1.5).
  2. It probably would be wise to check the PHP version, in case that changes with a module.
Mike Wacker’s picture

Status: Needs work » Needs review
FileSize
3.21 KB

Attached is the new and improved patch

flickerfly’s picture

Status: Needs review » Needs work
FileSize
3.19 KB

I changed the patch so it would apply with -p0, it's attached.

The patch applied fine, but did not test out.

Here's how I tested it:
I went to the devel module, added a dependency (tracker) to the .info file that I didn't have enabled and changed the datestamp for good measure. Then I went to the module page and refreshed it.

Results: The devel module still is enabled, but shows a dependency that is disabled.

If there is something wrong with my test, please correct me, but I suppose the proper status from my results is needs work. Please provide an better test method and I'll try that instead. Maybe I didn't trigger drupal to recognize that the module had changed somehow?

Mike Wacker’s picture

Status: Needs work » Needs review

@flickerfly
The problem with that method is that you did not run update.php, which you are supposed to do whenever you upgrade a module.

flickerfly’s picture

Ah, yes. Drush spoils me by doing that automatically. :-)

I just checked that and it does show an error in a nice red color.

flickerfly’s picture

Status: Needs review » Reviewed & tested by the community

Argh, which means I should change the status...

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Looks like this latest patch still needs review. Patch authors shouldn't set their own patches RTBC.

Is there a way we can call this function from the modules form as well, so we don't have the same checks happening multiple places?

sun.core’s picture

Issue tags: +D7 upgrade path

This looks pretty promising, and, with so many changing module dependencies for D7, I'd consider this elementary for the upgrade path. I wouldn't, if this patch wouldn't look as if it could work. So let's get this done!

Mike Wacker’s picture

@webchick Can you clarify a bit?

It may also make sense to put this in hook_requirements() for the 'runtime' op. While in theory you're supposed to run update.php each time a contrib module is updated, some people only do it when the upgrade includes a schema update. That scenario is unrelated to the D7 upgrade path, but it's something we might also want to account for.

tstoeckler’s picture

I had a go. The error now comes up both when visiting update.php and also on all admin pages as REQUIREMENT_ERROR.
I attached some screenshots. I also changed the strings around a bit.
Before the title of the requirements warning was the name of the module and the value was Missing/Disabled/etc.
Now the title is consistently "Unresolved dependencies" with the value being e.g. "Overlay (Missing)".
Old patch with a disabled requirement
New patch with a missing requirement
New patch with a disabled requirement
New patch with requirement with a non-matching version

Notes/Known issued:

  • I basically used the same code functionality-wise from the previous patch. I only included a check to not show 2 errors if a module is disabled and has the wrong version (in that case showing it being disabled as an error, is wrong because enabling it does not solve the problem)
  • I moved the new function into update.inc. It now gets called by system_requirements(). Since update.php already includes those there is no update.php hunk in this patch. It now feels kind of weird that a function that lives in update.inc gets only called by a function in system.install. On the other hand the function itself is very much related to the update process. Comments appreciated
  • As you can see in the screenshot with the non-matching version, the version string is surrounded by paranthesese, which looks a bit weird. I tried some regex, but I'm not very good, so it didn't work. (I tried: preg_replace('/(\()(\w\-.)(\))/', '/$2/', $compatibility))
  • Reviews on the new strings (and anything else, of course) welcome!

Status: Needs review » Needs work

The last submitted patch, 228860-always.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
4.02 KB

Calling system_rebuild_module_data during installation doesn't really work that well.
New patch attached to change that.

One thing I didn't note in the last comment:

As you can see here (Same screenshot as above) there is an error with displaying a missing module. In that screenshot Block module is missing, which is an obvious lie. For some reason it always displays the module that is listed in the .info file directly before the one that is actually missing. Weird.

So technically needs work, but needs review on the points above and want to see if the bot is happy now. I could install fine with this patch.

Status: Needs review » Needs work

The last submitted patch, 228860-always_2.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
4.05 KB

Yeah, I mixed up the two cases: Install / Not install.
My D7 is broken at the moment, so I tried manually editing the patch file.
Let's see how that goes.

Oh, and:
Online patch debugging due to PIFR 2.x: GREAT SUCCESS!

Needs review only for bot. If it passes, still needs work for the things mentioned in #28 / #30

tstoeckler’s picture

Bump.
Just to re-cap this is a really critical issue with a really beautiful solution (which I didn't think of, though, I'm not trying to boast here...), which essentially needs 2 things to be committable.
1. Someone who is even the slightest bit not completely horrible at regular expressions (unlike me) needs to remove the parenthesis around the version (in case a different version than the one found is needed)
2. Someone to find why some array pointer is always off by 1, because it shows the wrong module as being missing (the one above the actually missing one in the .info file)
Now if I were a real developer, I would think those are pretty simple issues. But I've stared at it already for long enough and wasn't enlightened.

(Sorry for being so verbose, just wanted to make the point that there are probably people reading this who could fix this in ~5min.)

hairybear’s picture

thanks great info!

tstoeckler’s picture

All right new version.

  • Added a comment. (Explaining the error I made in #30, which is probably not obvious)
  • Fixed the status report page showing the wrong module name when the module is missing (Didn't have anything to do with array pointers =), just a wrong variable name
  • Fixed the parenthesese around the version (NOTE: I had to use substr() AND trim(), which feels really weird, but I couldn't get it to work otherwise. Improvements appreciated)
  • Before, if a module raised it's PHP requirement in an update, the status report page would show multiple PHP entries: The normal one (green) plus one for each module needing a higher version. I collapsed this into one entry. Each module that needs a higher version is simply added to the requirement description. (see screenshots)
  • The previous change required me to roll the function (previously named update_check_dependencies() into system_requirements()). Since system_requirements() was the only function calling it, I guess that's OK.

Now, *really* needs review. No known issues (that I know of).

tstoeckler’s picture

FileSize
28.49 KB
8.56 KB
tstoeckler’s picture

FileSize
4.35 KB

Sorry for the multiple posts, but there some "validation errors" when uploading the files.

tstoeckler’s picture

FileSize
3.97 KB
+++ modules/system/system.install	15 Mar 2010 14:38:43 -0000
@@ -70,7 +73,6 @@ function system_requirements($phase) {
-

Unrelated. Please remove

Powered by Dreditor.

Updated patch attached.

amc’s picture

#38: 228860-always_4.patch queued for re-testing.

JacobSingh’s picture

Another nuance of this:
JacobSingh: problem arises when you define a class in your module like MyModuleClass Extends OtherModuleClass
[11:16am] JacobSingh: and OtherModuleClass belongs to a module which is not enabled yet (i.e. a new dependency).
[11:16am] JacobSingh: Since update.php bootstraps MyModule, it throws a fatal
[11:17am] JacobSingh: while trying to parse the file, since the class extends a non-existant class. Making it basically impossible to use your site or provide an upgrade path

sun’s picture

@Jacob: Could you clarify whether this means that this patch needs work? Or can you RTBC?

JacobSingh’s picture

Sorry, I didn't review the patch.

I was just specifying a use case / bug that I found. That bug is there now, and I don't think this patch will solve it or make it worse.

Is there another bug for that behavior?

tstoeckler’s picture

Since update.php bootstraps MyModule, it throws a fatal

If you look at update.php you see that it bootstraps a long time after it does system module's requirement checking. Since this patch moves the dependency checking of upgraded modules in system_requirements(), update.php should leave you with a nice error about which module is missing.

I'm not sure how you would go about enabling that module, though, because I guess trying to visit the modules page would give you a fatal?!

Anyone have some sample code, so I can test this out?

randallknutson’s picture

FileSize
69.79 KB
75.64 KB

I tested this by enabling the pathauto module and then adding token as a dependency in the .info file. After running update.php, I get the dependency error in screenshot-1. Token is still disabled in the module page but on the status report page it shows the dependency issue still there in screenshot-2. Did not test the class issues as I'm not sure I'm totally following what is going on.

Only problem I found is that I cannot enable token since it is a requirement of another module. Perhaps we should not disable the checkbox if the module is disabled and it's dependencies are met even if it is a dependency of another module.

randallknutson’s picture

FileSize
4.49 KB

Rerolled the patch with one line changed in system.module that allows enabling modules that are a dependency of another module but aren't enabled. This makes it so that I can now enable the token module. Once it is enabled though, I cannot disable it since it is a requirement. Once token is enabled, the requirement error goes away.

Looking good for me now.

tstoeckler’s picture

I had thought of that, but had hoped no one would bring it up.
I wouldn't have thought, i is a one line fix, though.
So, great catch!!!
I guess we need another review for RTBC, though.

gordon’s picture

#drupalcon - reviewing this now.

gordon’s picture

Issue tags: +drupalcon code sprint

add drupalcon code sprint tag

gordon’s picture

Status: Needs review » Reviewed & tested by the community

I have taken a look at this and it looks good.

Having it on the status page is good, and will stop modules from stuffing up things.

This is not that complex and I think it can be RTBC

catch’s picture

Status: Reviewed & tested by the community » Needs review

Wait a minute, hook_requirements() is called on a tonne of pages, can we at least check how expensive it is to be rebuilding all this information every time that's requested and on which pages it's called?

puregin’s picture

Issue tags: +Drupal update

subscribe

Mike Wacker’s picture

Status: Needs review » Reviewed & tested by the community

@catch #50

I grep'd around for 'requirements', 'runtime'. It only is invoked inside system_status. You can see from the Drupal API that only two functions invoke it, and those two functions are callbacks for the following three paths:

  • admin
  • admin/by-task
  • admin/config

We should be just fine if it's limited to the admin pages. There's no impact for non-admin users.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

I spoke with catch about this. He's spent the better part of the last year working his butt off to make Drupal 7 more performant. While my grepping confirmed what Mike says, both catch and I think it'd be a good idea to profile this and make sure it doesn't actually make things dramatically worse.

So if someone could get Devel query log running and report back the number of queries and page execution time on /admin and /admin/modules before/after this patch, that'd give us an idea of what we're looking at in terms of a performance degradation during 'normal' use, if any.

KarenS’s picture

This patch has drifted a long way from where it started. I'm wondering if it is solving the right problem in the right way.

The original issue is that if you have a module already enabled and it later adds a new dependency on a module that is not already enabled there is no warning and no way to enable the new module because the the checkbox to enable the module is disabled on the modules page.

One line of this patch fixes the problem of not disabling the checkbox if the module is not enabled. I totally agree with that fix.

The rest of this patch is about creating a way to warn the administrator that there is now a critical instability in their system caused by this disabled module. This warning is important and critical.

But the current patch is doing lots more than testing for that situation. We have tests for php version and module version and lots of other things. And it has been added to system requirements with no regard for the the phase, so it would even run during a fresh install, even though the original problem could never exist in a new install, it is an upgrade problem.

We could simplify this patch quite a lot if we only fix the checkbox and test only for missing dependencies and do it only during runtime and update. That would reduce the amount of work that is being done, to address the questions about performance.

I get that this also now does lots of testing of other things we are not really testing now, like that the installed version of php works for each module, and that might be useful and important, but I think that is something that needs a separate discussion and a different issue. Or we should rename this issue to reflect the more global problems that this patch is attempting to solve.

KarenS’s picture

Also we completely dropped all discussion about whether the required module should be auto-enabled or the module with missing dependencies should be auto-disabled during the update to stabilize the system. None of the patches in this thread address that issue.

So if I am doing an update from D6 to D7 and the D7 version of some of my modules now have dependencies on disabled modules that were not previously required (as mentioned by @sun in #26), what should happen? Warning me and fixing the checkbox is not enough because you can't get to the modules page to enable that module until at least the core update has run. At the very least we should be doing some testing to prevent any module updates from running if they have missing dependencies or all hell might break loose when their update code fails.

We have nothing here that would solve those problems. This is a thorny issue.

JacobSingh’s picture

@KaernS: Thanks for bring this back to earth.

I agree on your approach to solve the smaller problem first.

In terms of workflow, I think the only thing we can do is disable a module if its dependencies are not met. This brings up another issue with the class autoloader which I mentioned somewhere (can't find the issue). If I define a class in my module which extends a class in another module and that other module isn't included... bam, fatal error. :(

Seems to me that class defs need to be wrapped in class_exists() or something, although I'd much prefer if we just fixed our file name schemes to use a sane autoloader with filenames matching class names...

At any rate, the point is, a module with an unmet dependency has the capacity to royally screw stuff up. AND modules should be "modular" I think. :) This means that turning them off shouldn't cause your site to collapse. Granted the theme layer in Drupal is so tightly coupled most times that this will happen, but suspend disbelief. ;)

-J

KarenS’s picture

For the class problem, I think the logical solution is that any module that extends the class of another module should explicitly include that module in its list of dependencies. Then core can take care of making sure that the module is available. There are fancier things we could do but they all involve API changes, so I'm guessing they are out of scope.

About 'turning a module off shouldn't cause your site to collapse' -- yeah, having a little trouble suspending disbelief on that one :)

JacobSingh’s picture

The problem is, it will throw a fatal when you include the file.

Which means I have module A
I make module B a new dependency and I create this class:

AClass extends BClass() {

}

Then, you update module A. Before you even get a chance to fix your site by adding module B, it is completely broken. I don't think you can even access update.php. You get a fatal error on every page.

Also, if I was to call any function in module b directly in hook_init or _build or whatever will get called on most pages, I think that would also prevent you from getting to the modules page.

For those reasons I suggest that modules with unfulfilled deps should be turned off. If we had a system wherein upgrade modules wasn't about overwriting the existing code; A system where you tell Drupal, hey I want to upgrade, here is my rpm or .deb or .zip or whatever, can I do it safely; then we could handle this case. Otherwise, it feels like it's a user error... the update manager should respect a new dep when you are upgrading a module, so hopefully we'll be taken care of there before they overwrite the new code.

-J

KarenS’s picture

Ah! Now I see what you are talking about. So that is another example of the fact that we have to fix the problem, not just report it.

As to enabling vs disabling, if we disable a module we also have to check for all other modules that depend on that module and disable them too, and so on all the way down the chain. That could create a whole different problem. So disabling modules potential involves disabling lots of modules. Enabling a module would never affect more than one module.

If the new module is not available to enable, we would have to disable the dependent one I guess.

And this issue kind of ties into #773828: Upgrade of disabled modules fails - which discusses update problems caused by modules that are disabled. So we can't run the updates without enabling modules but missing dependencies cause fatal errors if we do enable them.

What a can of worms!

tstoeckler’s picture

Actually we are sort of fixing the problem (well, halfway) because in Jacob's case we are detecting that dependency miss in system_installrequirements which gives you a nice little warning on update.php, no blow-up there. It doesn't get you anywhere, though, in terms of actually disabling that module. Disabling it automatically though (also from system_installrequirements() ? ), since we already have all that information now, shouldn't be much of a problem.

Mike Wacker’s picture

@Karen #54

It's entirely possible a new version of a module could introduce code that could increase the least compatible version of PHP. I did that once with a module of my own by using some date functionality available in PHP 5.2 and above, and some module maintainers may want to take advantage of PHP 5.3 in the future.

Likewise, a newer version of a module (especially with a new major version number) could only be compatible with the 2.x or 3.x series of some given module, whereas the old version worked with a previous series. Having the 1.x version enabled does not solve the dependency issues. I totally stand by addressing those problems.

The one thing that involves performance implications, though, is adding it hook_requirements(). Here was the rationale for that one: some modules updates, especially ones that do not require a schema update, don't need the user to run update.php, even if they're "supposed" to always run update.php. In those cases, then the only way to catch new dependencies is via hook_requirements() with the 'runtime' op.

That being said, the chances of new dependencies being introduced in that type of update is significantly smaller.

Mike Wacker’s picture

Status: Needs work » Needs review
FileSize
4.24 KB

Here's an alternate version which only runs during updates. This would obviously solve any performance issues. I also cleaned up the code and the comments a bit.

tstoeckler’s picture

I also agree that the PHP version check needs to stay. It can wreck your site just as a missing module.
I just re-read this whole issue starting at #45 and I think the only solution to this problem (I can think of), would be for update.php to provide an interface that, in addition to telling you what's wrong (which it does with the patch in #62), gives you two buttons:
1. (In case the missing dependency is available) "Enable @required-module"
2. "Disable @requiring-module"
That way a risky/pro-Drupaler can just (obtain and) enable the module, if he/she is certain it will do no harm. A cautious user will want to click the second button, and probably try @required-module first.

A different question, though, is that idea should hold up this patch, which is, in itself a huge improvement. Because you can actually go to update.php and see what just happened, if your site WSODs after you upload a new module.

Mike Wacker’s picture

@tstoeckler
I see your point here, but that probably should go into a separate issue and not this patch. While we had to widen the scope of the patch to catch other dependency issues that we also discovered, this patch should solely focus on dependency detection and not dependency resolution.

tstoeckler’s picture

I agree, it's not me you'll have to convince, rather KarenS and Jacob Singh. Or, better: Dries or webchick.

gordon’s picture

I had an idea about this. Why don't we do exactly what install does.

So when you are installing it calls hook_requirements('install') and sees if there are any issues, and if there are errors it reports them and you can't proceed until they are fixed.

So for update.php we add a 'update' $phase to hook_requirements() which gets checked before you do any updates and then displays any errors if they are any.

This means that there is no performance hit as this is only run during update.php, and ATM we are only checking for missing dependencies, where this means that we could all any module to check any number of checks before the update is run and make sure the site is in the best possible state before running the update.

An example for this would be in e-Commerce if your receipting system is out of balance, you do not want to be doing any updates as this could make it even worse.

Also it will give use much more than the hook_update_last_removed().

I will create a patch for this tomorrow.

Gordon.

gordon’s picture

#62 - Looking at this is what I was thinking, but it is missing the code for update.php

gordon’s picture

Status: Needs review » Needs work

Changing to needs work.

tstoeckler’s picture

@gordon: That's kind of what I was proposing. If you install a module and something goes wrong, this usually has to do with a missing PHP extension, or similar. In this case, it might be that a module must be enabled first. But since you can't bootstrap Drupal, you can't go to the modules page and enable it (also currently, even if you get there, you can't enable the module, because the checkbox is disabled (and unchecked)). That's why update.php needs to facilitate for either: enabling a required module or: disabling the requiring module.

tstoeckler’s picture

Also back to needs review, because there is not yet a decision whether or not this patch is sufficient for a commit.

Mike Wacker’s picture

Status: Needs work » Needs review

@gordon I don't know if you've looked at the patch in #62, but here is line 312 of update.php:

$requirements = module_invoke('system', 'requirements', 'update');

The patch adds code to system_requirements, and this code only gets run in the 'update' phase. This removes the performance issues we were worried about earlier when it ran during 'install', 'runtime', and 'update'.

gordon’s picture

Status: Needs review » Reviewed & tested by the community

@Mike Wacker ah yes that was what I was missing. I don't remember that being in there.

I don't think this patch is going to get much better then. Setting to RTBC

Gábor Hojtsy’s picture

Same issue exists in older versions of Drupal, so if we can backport, then it is even better :)

KarenS’s picture

Status: Reviewed & tested by the community » Needs work

A missing dependency potentially creates a critically unusable environment. Performance issues or not, we have to have a runtime warning about this, not just an update warning.

And we seem to have tossed off the concern about what happens if the instability creates a fatal error that prevents you from getting to the modules page to fix it. We have also tossed off the concern about what happens in a D6 -> D7 update where you cannot get to the modules page until core has been updated.

I agree that deciding to disable/enable missing modules may need another issue, but we can't just fix a couple problems and ignore the others. If we need other issues to fix some problems, we should identify the specific problems that will not be solved by this fix and agree that fixing them is outside the scope of this issue.

So my summary of the problem is:

A module may add a new dependency -- which could be a dependency on another module, a requirement for a higher version of PHP, or a later version of a previously required module. If the new dependency is not met, it creates several additional problems:

(1) If the module it is dependent on is not enabled, the checkbox to enable it cannot be checked on the modules page, it is an empty, disabled checkbox.

(2) The missing dependency makes it dangerous to run updates.

(3) The missing dependency may create fatal errors which make it impossible to get to the modules page to enable the required module.

(4) The required module may be missing, so there may be no way to enable it.

(5) A D6 -> D7 upgrade creates additional problems because you can't get to the modules page to manually enable newly required modules until at least the core updates have run, but if you run update.php to update core you will also update modules with missing dependencies.

(6) In all cases, the administrator should be warned that a dangerous instability exists.

The patch in this issue addresses (1) and (2) and partially addresses (6) -- it does not warn the administrator unless they go to update.php.

catch’s picture

Status: Needs work » Reviewed & tested by the community

If you upgrade a module without going to update.php, then you're not following basic Drupal best practices. If we cover that case, that's plenty to deal with this issue. IMO refinements can happen in followups, some issues won't be solvable without a recovery.php.

Dries’s picture

I read up on this issue (welp!), and I agree that the patch in #62 is a first good step. Not being able to run update.php should prevent most damage from happening, especially if you follow the instructions. However, it seems like the dependency checking is incomplete -- in Drupal 7 we're also supposed to verify version information and not just the mere existence of a module. Can't we leverage some of the existing dependency checking code?

I pinged chx and I asked him to comment too. I'd love to get his take.

tstoeckler’s picture

in Drupal 7 we're also supposed to verify version information

That's what were doing. See the code below:
+ // Check for an incompatible version.

Can't we leverage some of the existing dependency checking code?

The patch uses drupal_check_incompatibility.
See also http://drupal.org/files/issues/dependency-2-3.png for how it looks (note: The parantheses have been removed by now)

Dries’s picture

Dang, I missed that function call in the patch. Thanks for correcting me tstoeckler. I'll do another review shortly.

catch’s picture

On the performance issue, system_rebuild_module_data() was measured to be 4 seconds on some sites, had anyone tested the earlier versions of this patch, even with devel timer, that would have shown up. #794936: system_rebuild_module_data() called twice on admin/build/modules adds a static cache for that though - however that only means that if it's /already/ called on a page, it wouldn't hurt to call it a second time. I'd still be opposed to adding it anywhere else. Either way, followup patch.

Mike Wacker’s picture

@catch We decided to only apply the patch only to the 'update' op, so it will only run when update.php is run. There's no performance hit during normal runtime anymore.

catch’s picture

@Mike, understood, I just wanted to make clear what the implications are if a follow-up patch gets posted.

Dries’s picture

Mike or someone else, can you create a follow-up issue? Once created, I'll commit this patch.

chx’s picture

My only concern (at least this is what relates to my work in this area) when I initally looked at the patch was whether all the depencies are covered but apparently system_rebuild_module_data calls $modules = _module_build_dependencies($modules); and so we are good there, too.

te-brian’s picture

Not sure if its been mentioned.. but modules that add new dependencies can enable them in the update hooks. That's what I've seen done (and followed myself) with things like CTools, for example.

Mike Wacker’s picture

Dries’s picture

#62: drupal-228860.patch queued for re-testing.

Dries’s picture

Patch no longer applies so asking for a re-test.

Dries’s picture

Patch no longer applies so asking for a re-test.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal-228860.patch, failed testing.

gordon’s picture

Status: Needs work » Needs review
FileSize
4.47 KB

I have re-rolled this patch so that it applies to the current HEAD

catch’s picture

Status: Needs review » Reviewed & tested by the community

Was already RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for the reroll. Committed to CVS HEAD.

Status: Fixed » Closed (fixed)

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

tstoeckler’s picture

+      $name = $file->info['name'];
+      $php = $file->info['php'];

These two lines produce warnings, when upgrading from a stock D6 site.
I don't know why, and I also don't know you don't see those in D7.
I will investigate, I just wanted to post in case someone has a similar issue.
If I find something worth noting, I will re-open or move to a new issue.

Damien Tournoud’s picture

This broke the upgrade path from Drupal 6 to 7 badly. See #877690-19: Cannot upgrade Drupal core if the comment module is enabled

KarenS’s picture

I noted that this would break the update path in #74 :(