_system_rebuild_module_data() and _system_rebuild_theme_data() will choose 6.x versions of modules and themes over 7.x versions

If you follow the instructions precisely then you should not end up with D6 CCK in your D7 modules directly. Maybe I'm reading this differently though? Downgrading from critical though, it's wrong to end up in that state, so at best we can try to make the documentation more explicit.

6.  Remove all old files and directories from the Drupal installation directory.

7.  Unpack the new files and directories into the Drupal installation directory.

8.  Copy your backed up "files" and "sites" directories to the Drupal
    installation directory. If other system files such as .htaccess or
    robots.txt were customized, re-create the modifications in the new
    versions of the files using the backups taken in step #1.

9.  Verify the new configuration file to make sure it has correct information.

10. Run update.php by visiting http://www.example.com/update.php (replace
    www.example.com with your Drupal installation's domain name and path). This
    step will update the core database tables to the new Drupal installation.

    Note: if you are unable to access update.php do the following:

      - Open your settings.php with a text editor.

      - There is a line that says $update_free_access = FALSE;
        Change it to $update_free_access = TRUE;

      - Once update.php is done, you must change the settings.php file
        back to its original form with $update_free_access = FALSE;

11. Ensure that the versions of all custom and contributed modules match the
    new Drupal version to which you have updated. For a major update, such as
    from 5.x to 6.x, modules from previous versions will not be compatible
    and updated versions will be required.

I think if you follow this precisely:

Copy your backed up "files" and "sites" directories to the Drupal installation directory.

then you will end up with 6.x modules files in your sites/all/modules. Then, the system table gets built with the 6.x sites/all/modules/cck/text.module rather than the 7.x modules/text.module , and the user has to figure out that they have to really quickly delete that module on disk and even more quickly refresh the modules admin page, or something could explode as the 7.x core tries to use the 6.x module.

Discussed with Moshe just now - alexpott, matason or I would be really happy to discuss some more. I think this should be upgraded to a critical but I don't want to play ping-pong with ticket statuses!

Updating status..

Code looks great to me.

Excellent work, can I suggest a small grammatically correction to the comment?

// Work out if there are anything in the $files array that will be overwritten on merging.

should really be...

// Work out if there is anything in the $files array that will be overwritten on merging.

I'd love to see some more code comments.

+++ includes/common.inc	27 Aug 2010 14:48:22 -0000
@@ -4819,7 +4819,20 @@ function drupal_system_listing($mask, $d
+    // Work out if there are any elements in the $files array that will be
+    // overwritten on merging with $files_to_add.

It would be great if we could explain why we want to work that out.

+++ includes/common.inc	27 Aug 2010 14:48:22 -0000
@@ -4819,7 +4819,20 @@ function drupal_system_listing($mask, $d
+      // If this is incompatible with Drupal core remove it from the array.
+      if ($info['core'] != DRUPAL_CORE_COMPATIBILITY) {
+        unset($files_to_add[$file->name]);
+      }

Would be great to explain why this is the proper thing to do.

Sounds like a "needs work".

Here's a new patch with expanded comments.

However, I'm concerned about this line:

+++ includes/common.inc	27 Aug 2010 14:48:22 -0000
@@ -4819,7 +4819,20 @@ function drupal_system_listing($mask, $d
+      $info = drupal_parse_info_file(dirname($file->uri) . '/' . $file->name . '.info');

Is it a requirement anywhere in Drupal that all files for a module be in a single folder?

What if two modules share the same folder?

What about .inc files that module authors put inside an 'includes' folder?

Powered by Dreditor.

I don't think it is...

You start with $file->name. That could be anything, a .module or a .inc.
You make its directory name with dirname($file->uri). This is the first potential failure. You're assuming foo.module or foo.inc lives inside a folder called foo.
You then take the directory name, append the .info file. Second potential failure. You're assuming foo.inc is in the same folder as foo.info.

You started with some/path/foo.inc and you're now looking for some/path/foo.info. There's no guarantee that file exists.

Granted, the bug being fixed here won't be affected by the second problem, as _system_rebuild_module_data() is the caller in question, and this specifically asks for .module files. But I'm sure I've seen contrib modules dump multiple .info and .module files into their main folder.

And given drupal_system_listing() is a general purpose API function, we should guarantee what we're doing.

I think the $mask check is potentially brittle. What if a mask is changed at a higher level up to check for '/.*.info$/' ? The function should still behave the same.

So instead of checking $mask == we should explicitly check that $file->uri ends in .info or .module with preg_match(). Thoughts?

> we should explicitly check that $file->uri ends in .info or .module with preg_match().

Agreed on checking that explicitly, but isn't there a PHP function for getting a file extension? Failing that, substr would be quicker than a regexp.

Actually, on reflection, I think we should just check with file_exists() that the .info file is there. If we can't find a .info file we just assume the generous, existing scenario, and leave the original on the list for inclusion. Otherwise we leave ourselves open to too many edge cases.

    foreach (array_intersect_key($files_to_add, $files) as $file) {
      // Work out where the .info file for this resource SHOULD be
      $info_file_name = dirname($file->uri) . '/' . $file->name . '.info';
      // If it has no info file, then we just behave liberally and accept the new resource on the list for merging
      if (!file_exists($info_file_name)) { continue; }

      // Otherwise, parse the info file so we can check core compatibility
      $info = drupal_parse_info_file($info_file_name);

      // If the resource is incompatible with Drupal core, remove it
      // from the array for the current search directory, so it does not
      // overwrite the existing resource location in the $files array
      if ($info['core'] != DRUPAL_CORE_COMPATIBILITY) {
        unset($files_to_add[$file->name]);
      }
    }

Looks good to me.

I think this patch should really check isset($info['core']) and not do anything if it isn't set. Otherwise it might cause problems for other, alternative uses of .info files besides the ones supported by Drupal core (Skinr comes to mind).

***

But do we really want to take this overall approach? I guess it's technically correct to always ignore 6.x modules, but parsing all these info files isn't cheap, and we're basically adding it to low-level code that runs all over the place, just to support a smoother experience for sites upgrading from D6...

Seems like another option to solve this would be:

1. Document in UPGRADE.txt the list of D6 contrib modules that you need to delete from your filesystem in order for the upgrade to work.
2. Check for this situation in the update.php requirements check, so we can warn people from within update.php itself if they haven't done this step, and prevent them from running the updates until they have.

That seems like it would be pretty simple, and although it would require a bit more manual work for people upgrading from D6, maybe the tradeoff is worth it? (It's good practice for them to delete obsolete code from their filesystem if they're never going to use that code again anyway...)

True, I guess this code won't be hit too often. I'm not totally opposed to it - it just seems a little disturbing that we're adding it in this very generic place mainly to solve a specific D6->D7 upgrade issue. Although you are right that there are other situations where this issue could arise too (although they are probably something of an edge case).

Anyway, the patch looks better although I think I just noticed what may be another bug. It seems like the patch is assuming that the files array will always be keyed by the module or theme name (i.e., $key = 'name')? But $key can be other things too. This line in particular - unset($files_to_add[$file->name]) - is suspect, but there might be other problems with it too... drupal_system_listing() is a very generic function that can search in arbitrary directories with arbitrary regular expressions and return its results in a variety of ways, so we have to be very careful here, as @joachim pointed out above.

Thanks, looks great. And I tested it manually to validate that it works as intended.

Good catch, and nice work on the docs as well. The only thing that'd be really great here is to have a test that proves it's working (and so it doesn't break again). I imagine we'd need our test D6 upgrade database to enable something like Blog module, and then have a "fake" Blog module that has hidden=TRUE and core=5.x in it, then see what happens.

Could you look into this to see if it's possible? If not, we can commit the bug fix as-is.

Also, should this be marked for backport?

Actually, I think there may be a way to test it - although it's very new (perhaps didn't even exist at Copenhagen yet!). We now have a profiles/testing directory which can be used for this kind of thing.

I tried that in the attached patch. This test correctly fails if you leave out the original fix.

Tested against a fresh checkout of head, patch and test look good!

I'm not sure how this fell off my radar (sorry!), but people upgrading to beta 1 are encountering issues I think due to this patch not being committed. So I would like to do that! However.

Test is really clever! But I really don't like abusing the testing profile that way. :\ That profile should stay extremely low-key and used strictly as a low-overhead way to run functional tests, not as Yet Another Dumping Ground For Modules That Don't Fit Anywhere Else.

We can't do the blog.module thing in modules/simpletest/tests/? I would think it would work since S comes after B, and it's nested deeper, so it should come later. Could we give that a stab? If it doesn't work, then I guess we'll have to commit this without a test.

Escalating to critical.

Ah, nuts. :\ Ok, then. Could we get a re-roll with just the fix? As hard as it is to say, I think going with no test is preferable to adding a weird test module to testing profile.

Yay! :)

Awesome, thanks Alex! Let's see if folks have better luck with this patch.

Committed to HEAD! Yay, CPH code sprint! :D

I think we should have tests.

I only put it in the 'testing' profile because that seemed like a good out-of-the-way place. And it's just files sitting in the filesystem (the testing profile doesn't ever enable them, or ever need to)...

However, other options include:

1. Putting it in the standard profile directory itself, e.g. profiles/standard/modules/tests (just like many core modules have a 'tests' directory with test modules in them, it seems reasonable to have one for a profile too).
2. Creating a new profile, 'testing_standard' or something, that is a mirror of the Standard install profile but which can be used for testing profile-specific functionality. This might be a good idea in the long run anyway - although there isn't much we can test right now, it would be great to be able to eventually write tests for some functionality that install profiles can implement (but which the core install profiles don't actually implement, and therefore never actually get tested).

Hm. #38.1 sounds interesting. Then that would work consistently for contributed profiles, too.

Is there some overlap here with #911354: Tests in profiles/[name]/modules cannot be run and cannot use a different profile for running tests?

Here is a patch using profiles/standard/modules/tests.

I also put a README.txt file explaining what the 'modules' directory is for, because otherwise it probably looks odd (and all-too-inviting) to be sitting there empty except for a 'tests' directory.

As far as I can tell, #911354: Tests in profiles/[name]/modules cannot be run and cannot use a different profile for running tests is unrelated because that seems to be about the particular case where Profile X ships with a module whose tests want to run using Profile Y instead... But in all the patches here, the tests are written to use the profile they are shipped with.

Ouch, I ran into this bug when testing the upgrade path against beta 1.

Tests in #40 are quite awesome. The approach is sound, although it adds testing stuff to /profiles/standard/modules, but that's probably ok (along with the new README:

This directory is used to place modules that are shipped with an installation
profile but that are not part of Drupal core. It should be used by developers
of the installation profile only. Modules which you download for your site
should be placed in sites/all/modules or an equivalent location instead; see
the README.txt file there for more information.

The Standard profile only uses modules that are part of Drupal core, so this
directory is empty (except for automated tests).

...sounds good.)

My one criticism is that it unnecessarily extends DrupalWebTestCase -- the only API function it calls is drupal_system_listing(), we should be using DrupalUnitTestCase in that case. Other than that I think this is RTBC. I reversed the patch in #36 and indeed got the new test to fail the incompatible_test assertion.

The attached patch switches to using DrupalUnitTestCase.

I also made a slight adjustment to the README - I realized it was better to reference modules/README.txt (rather than sites/all/modules/README.txt) for more information, since that one is the one that contains the most detailed instructions as well as a link to a d.o. handbook page, etc.

Nice, looks RTBC now.

I'm really sorry to do this, but can we please postpone this on #938600: Decide what to do with test modules under testing profile? After seeing the patch as I asked for it, it doesn't really make any sense. I'd like to come to consensus on this once and apply it everywhere, since this is coming up in a couple of issues atm.

See my response in #938600: Decide what to do with test modules under testing profile.

I think we should return to using profiles/testing/modules for this test. There's no relation between this test and the standard profile, plus this is purely a unit test, i.e. the modules never even get enabled, at all. We don't consider profiles/testing/modules a "dumping ground" but for this test it's the logical place.

OK, here are the tests from #29 included as their own patch.

With this approach, it looks like we can't use DrupalUnitTestCase anymore (I guess the $profile = 'testing' stuff doesn't work with that) so this continues to use DrupalWebTestCase as #29 did.

The tests pass, and this code was basically RTBC before... so I'm boldly marking my own patch RTBC :)

WHOA WHOA WHOA WHOA.

j/k, i second that RTBC. Too bad we can't make it a pure unit test, but it's good enough.

Leaving this for @webchick as she asked for this to be postponed.

#46: 895140-drupal-system-listing-46.patch queued for re-testing.

#46: 895140-drupal-system-listing-46.patch queued for re-testing.

Tests pass again - must have been a glitch.

*gulp* Ok. I think I've committed this to HEAD. :) Hard to tell with all those subdirectories of subdirectories in CVS. We'll find out if testbot barfs in a moment. :P

Test passes as committed. Thanks Angie! :P

Thanks! I just did a CVS update and it looks correct to me. Guess we will find out for sure :)