Only allow enabling modules with the same Drupal core compatibility version

screenshot

hehe, thanks for the encouraging words, chx. ;)

here's a new patch that includes the aforementioned DB update to automatically disable every module or theme in {system} that either:

- has no code installed at all (no .info present)
- doesn't define its drupal version (no 'drupal' in the .info)
- defines the wrong 'drupal' in the .info

tested it pretty thoroughly, but i'm setting this back to needs review, since this could use some independent testing and review. thanks.

@Stefan: please try the patch, or at least look at the screenshot: that's exactly what it does already.

@Dries: i don't quite follow your point #2, can you elaborate? re: #3 - i don't like greying out the checkbox, since we use that regarding dependencies. it's not just that you could install this module if you installed something else. the problem is that you need to completely replace the code before you could enable it.

@merlinofchaos, Dries: actually, parsing from the version string would be better. To best support CVS deployments via update_status in core, i think we're going to want to modify module_rebuild_cache() and system_theme_data() [1] to update the version info in the array each one returns, based on the goodness from http://drupal.org/node/146027 and http://drupal.org/node/146352. then, no matter *what* happens, we'll have the canonical human-readable name, and that's all we'd ever be parsing. the only place this goes wrong is pure HEAD checkouts, but i really don't care about that.

re: themes -- .info files for themes are new in D6, so even if we were incredibly lame and didn't do anything else with themes, the functionality in this issue would still work for themes in D6. it'd be just like the .info files for D5 modules: any theme without a .info obviously couldn't load in D6 core. but, it'd be easy enough to expand this functionality to the themes page as a separate patch after this lands.

attached patch is just the changes in in modules/system/*, and operates on parsing human-readable version strings. once update_status moves into core, we'll have even better support for CVS deployments, and things will still work fine regarding this patch, without the need for this new attribute in all .info files.

thanks,
-derek

[1] side note: it'd be great to unify those 2 into a consistently named API. see http://drupal.org/node/147000

upon further consideration, patch from #4 might be better, after all. while the CVS stuff can be solved as i explained above, i had another thought on why a separate .info attribute might be better: non-drupal.org contributions. while it's easy for us to parse our own version strings that follow the d.o conventions, what about other modules that might have their own version strings, conventions, etc? seems a little lame to force them to start their versions "6.x-*", regardless of what their existing naming/numbering convention is. although, maybe that's a feature, not a bug. ;) anyway, by using a separate drupal = 6.x, we solve this problem, since folks can add that to their own .info files, and we don't force their version numbers to match our expectations.

honestly, i can see both sides of this (and have already provided patches for each), so i'll let Dries and others decide which approach they prefer.

cheers,
-derek

i don't think "system" is any more clear or semantic than "drupal". i'd vote for "drupal core" since on d.o, this taxonomy is refered to as the "Drupal core compatibility", and i think everyone will understand what "drupal core = 6.x" means in a .info file.

re: a separate attribute vs. parsing version string. HEAD checkouts is one problem. the more difficult to solve problem is what about versions for external things? for example, see the whole thread on the devel list about 3rd party libraries and plugins. what if someone wants to either:

a) host a module elsewhere, and use their own version numbering scheme that makes more sense for them? they'd have to prepend "6.x-" to their version strings if we're just parsing that directly, which could be lame.

b) include a drupal "module" that really just provides a given 3rd party plugin, which is then shared among many actual drupal contribs? in that case, the version of the thing should really be the version of the given plugin, and it's a separate attribute about whatever version of core this plugin is targetted for (e.g. jQuery 1.0.x == drupal 5.x, vs. jQuery 1.1.x == drupal 6.x, etc).

re: version-specific dependencies -- that's a nice idea, but a huge can of worms that's already been discussed at length before. don't have time right now to track down the nids and threads of interest. i think that's way too ambitious for D6, so i'd rather just see this patch (or something like it) go in now, and if we want to try to do version-specific dependencies in D7, fine with me.

if there's agreement on "drupal core" or "core version", i think those are my 2 favorites. i'd roll patches, but i'd rather just decide a name first and then do the work. the logic is trivial based on my existing patches above.

oh, i should add: i'd rather completely break things for pure HEAD checkouts, than make assumptions like "HEAD must mean the latest drupal core, whatever that is". that assumption was the root of much evil in drupal's history, and destroying it was a goal of mine all along with the release system and now this issue. please don't drag us back into the dark ages with this assumption. ;)

seriously, please read comment #4 at http://drupal.org/node/124661 about how to best support CVS deployments.

re-roll for system.install conflicts. retains "drupal = 6.x", since Dries, merlin and myself all agree that's fairly self-explainatory, and i think overall, a separate attribute is better than parsing the version string.

another re-roll to avoid fuzz. can someone else please RTBC this so it gets in ASAP? thanks!

ok, after a brief discussion in IRC, we converged on "core". 1 second and i'll re-roll this.

new patch that uses "core" and fixes the conflict in system.install.
(this is now properly back to needs review now, after the collision with webchick's previous comment).

re: incompatible: fixed.

re: Shouldn't system_update_6024() be a generic function? Do we have to write a similar version when going from Drupal 6 to Drupal 7? It looks like something that should run after every upgrade?

great point. new patch moves it into update_fix_core_compatiblity() in update.php, which is called on *every* visit to update.php from here on out. ;) why not? better safe than sorry, right?

re: inconsistency in the APIs: see my comment #9 above and http://drupal.org/node/147000 ;)

in that case...

note: as soon as this lands, i'll add code to the packaging script to automatically set this attribute when it's adding the proper version string to the .info files. i'll also update the upgrade docs to mention this as part of the changes to .info files in D6. however, do you think this is CHANGELOG.txt worthy? if so, attached patch adds the following line to the "Usabilty" section of the changelog:

* Only allow modules and themes to be enabled that have explicitly been ported to the right core API version.

feel free to modify when you commit if you don't like that wording. ;) the patch also fixes a minor typo in CHANGELOG.txt i noticed while adding this new entry.

FYI: here's the patch for package-release-nodes.php for once this feature hits core. already tested on s.d.o and it works fine.

note: unlike core itself, the d.o packaging script is free to just parse the version string to find this info, since it knows that all projects on d.o have consistently formatted version strings. we could also query the DB for the "Drupal core compatibility" taxonomy term for each release node, but that doesn't seem necessary. also, note that this would add the core = "5.x" line to newer 5.x packages, too, but there's no harm there, since 5.x core just ignores this attribute.

Excellent, thanks!

Now pending:
- Upgrade docs for modules and themes
- The .info file docs
- Committing that patch to the packaging script and installing it on d.o

Stay tuned. ;)

6.x module upgrade docs done: http://drupal.org/node/114774#info-core
6.x theme .info file docs done: http://drupal.org/node/137062

Not sure what to do about 6.x module .info file docs, since it seems like we either need to fork http://drupal.org/node/101009 or split it up into subsections for each core version or something. However, there's already a doc issue about this: http://drupal.org/node/137069

Modified version of the above patch commited to the packaging script: http://drupal.org/cvs?commit=69766
I decided to not include the core = 5.x line on 5.x modules. I'll see if anyone actually would prefer we started doing that. However, there are tons of existing 5.x .info files that don't have it, so I think it's cleaner to just treat this line like the existance of the .info files for 5.x -- if it's not there, it must not be 6.x yet.

Yay, all fixed. ;)