D6 modules satisfy D7 module dependencies.

Still an issue in the latest D7 code.

Just to clarify, you don't need Drush make files or anything like that to trigger this bug. All you need is to have a D7 module with a dependency, and then accidentally download the D6 version of the module it depends on (rather than the D7 one). The Drupal UI will let you try to install the original module in that case, and bad things will happen. It shouldn't let you install it at all.

I ran into this bug a couple of times as well. Sometimes ending up with a WSOD. Bumping to critical.

Weird, there is the following code in system_modules() in system.admin.inc:

        $requires_name = $files[$requires]->info['name'];
        if ($incompatible_version = drupal_check_incompatibility($v, str_replace(DRUPAL_CORE_COMPATIBILITY . '-', '', $files[$requires]->info['version']))) {
          $extra['requires'][$requires] = t('@module (<span class="admin-missing">incompatible with</span> version @version)', array(
            '@module' => $requires_name . $incompatible_version,
            '@version' => $files[$requires]->info['version'],
          ));
          $extra['disabled'] = TRUE;
        }

...which apparently doesn't work. Not sure why, though, yet.

Won't that code above only check against the Module Major/Minor version? It doesn't look like it even considers the core version in the comparison.... If that really is the case, then, would prefixing DRUPAL_CORE_COMPATIBILITY . '-' onto the first version fix this problem?

A similar thing would also need to be done for the theme check too I'd imagine.

Here's a start. Needs review only for the testbot.

Needs work for the following:

• Tests (added tag)
• Is the condition in the if actually correct?
• There is a change in the existing string for incompatible module versions (vs. core versions), which is unrelated, strictly speaking. I think the change makes sense (for D8), but maybe not in this patch.
• The existing check for incompatible module versions, and for consistency the check for incompatible core versions introduced in this patch, mis-use the "admin-missing" class. It would probably be better to use dedicated classes and style them all the same in Seven. That would allow for themes to theme those differently. Also D8 material.
• The string that appears on the Modules page now says "incompatible with core version". I originally went with "incompatible with Drupal core version", but I think there is an initiative to eliminate the word Drupal from interface text to facilitate distributions. Is there some way to get the distribution name, which we could use here? Or how could that be better worded, without using the distribution name?
• Probably something I didn't see

D'uh....

Well, at least I didn't break anything, that's a good thing.
Back to needs work (see #6).

You can do something like this to get the distribution name.

$profile = drupal_get_profile();
$info = system_get_info('module', $profile);
$info['name'];

Even simpler, use this: http://api.drupal.org/api/drupal/includes--install.inc/function/drupal_i...

However, I wonder if it's better not to bother... Distributions of Drupal might be using a different version numbering than Drupal itself, so e.g. "Open Atrium core version 8" might not mean anything useful and might even confuse people.

Looking at the current code for when the module itself has the wrong version, it does this:

    $status_short .= t('Incompatible with this version of Drupal core. ');
    $status_long .= t('This version is not compatible with Drupal !core_version and should be replaced.', array('!core_version' => DRUPAL_CORE_COMPATIBILITY));

Maybe better just to use the same language here too. In general we don't want to use "Drupal" in the interface text, but I think this is an exception (the message only appears when you're mixing Drupal versions together anyway so to get to that point you're probably somewhat deep into Drupal itself).

In the middle of writing tests (coming soon) the following things came up:
- When you enable a module with a missing dependency by force and then submit the module page, you are informed that the module is being disabled again. There is no such thing for incompatible versions (neither for incompatible module versions (which are already checked) nor for incompatible core versions (which this issue is for))
- If a module depends on a hidden module with an incompatible version (module version or core version) the module is not disabled on the modules page. IMO that's a bad thing, but enough of an edge case to deserve its own issue.
- Dear Lord, system.test and friends are just about the messiest pieces of **** you can find in Drupal core. Also for another issue...

Here are some tests.
This adds 4 assertions, 2 of which already pass (the checking of incompatible module versions) but were previously untested.

Here we go.

The if condition is now actually correct. I also changed the newly introduced string here a bit from the previous patch to be more inline with the other string.

This is now ready to go, pending reviews. It is the least intrusive patch that fixes this issue.
It introduces a new string:
@module (<span class="admin-missing">incompatible with</span> this version of Drupal core)
The completely awkward placement of the span is consistent with the string for an incompatible module version.

That's very strange.
I get 2 fails for the second to last patch and all green for the last one.
Can someone try this out locally to see if it's just me?

+++ b/modules/simpletest/tests/system_test.module
@@ -264,6 +264,12 @@ function system_test_system_info_alter(&$info, $file, $type) {
+  if ($file->name == 'system_incompatible_module_version_dependencies_test' ||
+      $file->name == 'system_incompatible_core_version_dependencies_test' ||
+      $file->name == 'system_incompatible_module_version_test' ||
+      $file->name == 'system_incompatible_core_version_test') {

in_array()?

+++ b/modules/system/system.test
@@ -422,6 +422,35 @@ class ModuleDependencyTestCase extends ModuleTestCase {
+    $checkbox = $this->xpath('//input[@type="checkbox" and @disabled="disabled" and @name="modules[Testing][system_incompatible_module_version_dependencies_test][enable]"]');

Don't you have to do something (something weird) to make that Testing package group appear in the first place?

6 days to next Drupal core point release.

1. Sure, will do that on the next re-roll. I was just trying to be consistent with the rest of that function, which does exactly that. That whole group of tests could be revised to use system_info_alter() to conditionally alter the information based on the running test, instead of having a half dozen worthless test modules. That's something for another issue, though. I agree, though, that that if statement is unneccessarily ugly.

2. That's exactly what that first hunk in system_test_info_alter() is for.

Possibly related: #1228488: Drupal 7 text module update (added during beta) runs on sites that have D6 CCK text module installed

I come to this issue as a result of #978944-12: Handle exceptions thrown in cron. In that patch, a new test module was introduced to confirm that a hook_cron() failure in one test module would not affect the execution of hook_cron in another test module. The results show all green, which normally would be great -- except that the tests were being performed for Drupal 8 and based on the new test module's info file it was a Drupal 7 version!!

As I stated in #978944-17: Handle exceptions thrown in cron,

If we are testing for Drupal X, I would expect all modules enabled for that test suite also must be of version X.

In the referenced patch there is the following code

+  function setUp() {
+    parent::setUp(array('common_test', 'common_test_cron_helper'));
+  }

I would expect that function to fail (and hence the test to fail) if each of the modules in the array could not be enabled in turn. My implicit assumption has been that one cannot enable a Drupal X module for Drupal Y. Apparently that assumption is wrong.

Perhaps issue #1171436: Assert that modules passed into setUp are properly enabled. might part of the solution to this differing modules version issue. Thoughts and insights are welcome!!

@tstoeckler: Tests fail on my local machine exactly as they did on the testbots. I think you forgot to add your test module files like system_incompatible_core_version_dependencies_test etc.? Git diff is not sufficient if you want to add files, see the Advanced patch contributor guide http://drupal.org/node/1054616

Oh my god, I can't believe I didn't notice that!!!!
Thanks @klausi, I'll roll a proper patch.

Sorry, I had totally forgotten about this.
Let's see if this works.

Tagging as novice for the task of rerolling the Drupal 8.x patch on account of #22336: Move all core Drupal files under a /core folder to improve usability and upgrades.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

Update for #26 - d8 /core update.

Shameless bump.
This has been sitting around for weeks or months (depending on how you count) with a patch that needs review. That is not unusual for a critical bug in itself, because critical bugs are often really tricky, nasty bugs that only Drupal superheros can fully comprehend and review. This one isn't! Anyone with a little Drupal experience can review this patch code-wise, and for trying it out no experience at all is needed! Let's get this in. :)

This patch has tests already, so removing that tag. It'd be helpful to also have the tests uploaded by themselves, to demonstrate that they fail without the fix.

I actually read this patch when I marked it for reroll in #26, and there are a couple one-line summaries over 80 chars, but I couldn't come up with a rewording that fit, so I didn't say anything. :P

+++ b/core/modules/system/system.admin.incundefined
@@ -811,6 +811,7 @@ function system_modules($form, $form_state = array()) {
+        // Disable this module if it is incompatible with the dependency's version.

Also this inline comment is a few characters too long and should be wrapped.

Other than that, the patch is well-written and the approach looks sound.

Tagging for manual testing. Please follow the steps to reproduce in the summary--or rather the git equivalent, using 8.x instead of 6.x and 9.x instead of 7.x--and make sure you can reproduce the bug. (Edit: If someone wants to write updated steps to reproduce, that'd help testers.) Then, apply this patch, and repeat those steps to confirm that the bug no longer occurs.

The easiest way to reproduce this is to make a backup of system.admin.inc before applying this patch and then manually enabling the system_test module. Something like this (untested):

# Apply the patch
cd /path/to/drupal/8/dev
wget http://drupal.org/files/1002258-27-d6-d7-incompatible.patch
cp core/modules/system/system.admin.inc core/modules/system/system.admin.inc.orig
patch -p1 < 1002258-27-d6-d7-incompatible.patch

# Restore original system.admin.inc
mv core/modules/system/system.admin.inc core/modules/system/system.admin.inc.patched
cp core/modules/system/system.admin.inc.orig core/modules/system/system.admin.inc

# Enable test module
drush en system_test
# Now visit the Modules page on your Drupal site and verify that you CAN enable the "System incompatible core version dependencies test" module

# Restore patched system.admin.inc
cp core/modules/system/system.admin.inc.patched core/modules/system/system.admin.inc
# Now visit the Modules page on your Drupal site and verify that you CANNOT enable the "System incompatible core version dependencies test" module

Following instructions in #31, the results were as described (see attached).

Thanks @kathyh! Screenshot says it all. Now I feel comfortable calling this RTBC. :)

+++ b/core/modules/simpletest/tests/system_incompatible_core_version_dependencies_test.info
@@ -0,0 +1,7 @@
+core = 8.x

+++ b/core/modules/simpletest/tests/system_incompatible_core_version_test.info
@@ -0,0 +1,6 @@
+core = 9.x

+++ b/core/modules/simpletest/tests/system_test.module
@@ -264,6 +264,14 @@ function system_test_system_info_alter(&$info, $file, $type) {
+  if (in_array($file->name, array(
...
+  ))) {
+    $info['hidden'] = FALSE;
+  }

I wonder whether we should adjust the "core" .info property of the incompatible module in the alter hook, so this test doesn't "suddenly" break Drupal core when we're switching to D9?

Not sure though. If core maintainers are fine with the hardcoded version numbers, then I'm too.

This won't be the only place that needs a mass string update when core is updated, so I don't really mind either way.

However couldn't we have one file that is 7.x and one that is 8.x - and then it'd presumably work for both 7.x and 8.x if the assertion is generic enough? (with the roles the test modules are playing reversed though).

Hmm, I think hardcoding relevant version numbers is actually easier to understand than using the same files for opposite roles. I think the latter would trip me up, at least, because the easy thing to change (dummy test files, in an expected location in a .info) would be the same while the tricky thing to change (real code in tests) would diverge between branches. Unless I'm misunderstanding?

Yeah the files changing roles was a bad idea. Moving this back to rtbc since I don't think the alter is going to make this clearer and a find and replace in a couple of years is fine.

Maybe we should do something like this?

Oopsie, crosspost. RTBC for the old patch is fine too. :P

Testbot had too many martinis at lunch.

webchick suggested we use an old version instead. I haven't tested this locally because my dev environment is suddenly hosed, so I'm going to apologize for the noise and let testbot do the work.

#42: 1002258-42.patch queued for re-testing.

Right, the thinking was that an older N.x will /always/ not be the version of Drupal we're talking about, so we won't ever need to ever adjust it again. :) It's also what this bug was about in the first place.

That looks much better. 8.x will be caught by general find and replace, whereas 9.x wouldn't.

I am saying this is RTBC but a) the issue is not critical b) once this patch is in we need to keep the issue open because it is still possible that you can't enable a module because the module it depends on has a requirement error. Thanks to _module_build_dependencies we dont need to worry about deep dependency chains so that's the only other problem. An elegant solution would, however, react when a module is marked not-enable-able and mark modules depending on it also not-enable-able.

OK disregard b) above. Apparently we don't run requirements checks just on visiting admin/modules so we can't mark dependent modules based on that disabled.

However, what I said about elegance stands. We now have duplicated the code that checks for core compatibility and also check every module for core compatibility N times. It'd be more elegant to say "Oh. This module is rotten. Let's visit dependent modules and mark them as rotten too!" Isn't it great that $files[$module]->required_by is filled in by _module_build_dependencies() :) ?

On the other hand that would, instead of the core dependency check duplicate somewhat the iteration (not every file but for something important it can be a lot) but I still think it'd be more elegant. I let the core committers decide.

Hmm, I see chx's point about duplicating the check, and potentially checking the same module N times, but also trying to apply this to dependencies seems like it wouldn't necessarily end up a net win. Given the existing code is structured like this and we want to backport the fix, committed and pushed to 8.x.

This will need a re-roll for 7.x.

D7 port, with the test modules changed to 7.x dependency where appropriate.

Edit: I really need to stop giving my interdiffs the extension .patch.

Looks good.

Committed and pushed to 7.x. Great work, folks!