move all CVS-specific code into a separate contrib module [#150316]

while i'm one of those deploy-from-CVS people myself, and i definitely want to work on http://drupal.org/node/124661 and other issues to make update_status behave better for sites deploying from CVS, i fear the consequences of putting CVS-specific code directly into core. :( it ties us too closely to a specific tool, and complicates our ability to change tools some day.

so, in anticipation of moving update_status into core, we need to clean it of all the cvs-specific code, and move all that functionality into a separate module that can remain in contrib.

here's an initial attempt at that. i'm calling the new module "cvs_deploy.module" for lack of a better name. other suggestions encouraged. ;) i'm not just calling it "update_status_cvs.module" since i think it could be valuable in other ways, like a form_alter() to make human-readable version strings on the admin/build/modules page, etc.

i'm trying to code this as if core itself was going to support a new hook_get_version() (again, a better name would be welcome) to give contribs a chance to find and sanitize the right version string for a given project. for now, only update_status.module is invoking the hook, but as we move into core, this hook should be invoked directly from module_rebuild_cache() (or whatever that ends up being renamed as a result of http://drupal.org/node/147000).

i don't know of a good way via the existing hook APIs to say "just find me a single value" instead of automatically treating it as an array. so, each call site is doing something a little silly with the results of the module_invoke_all(). feedback on a better approach for that would be appreciated, too.

the only functionality we "lose" with this patch is that we don't automatically ignore all CVS-deployed modules. instead, we look at their effective version strings and the usual rules apply (mostly as if everything was a dev snapshot). IMHO, this is a bonus, not a draw-back. If you deploy directly from a tag via CVS, this new behavior is much better than what we have now. In other words, this patch really paves the way for http://drupal.org/node/124661 to ever really work.

i'm calling this critical since i consider this patch a blocker for to moving update_status into core.

Comment	File	Size	Author
#6	update_status_no_cvs.patch_1.txt	9.67 KB	dww
	update_status_no_cvs.patch.txt	9.55 KB	dww

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

dww

we/he/they

CreditAttribution: dww commented 9 June 2007 at 00:27

re: the name of the new module, in IRC, bdragon suggested "deploy_from_cvs.module" and "deploy_via_cvs.module" as possible alternatives. any other ideas?

Comment #2

merlinofchaos CreditAttribution: merlinofchaos commented 9 June 2007 at 18:09

Because my dad taught me never to use prepositions in a module name (ok ok so my dad never touched a computer in his life), try cvs_deploy.module

Comment #3

dww

we/he/they

CreditAttribution: dww commented 12 June 2007 at 00:35

re: "cvs_deploy.module" -- that's what's in the patch now. i guess that means you're happy with that.

however, any feedback on the other things?

+        // Give other modules a chance to fill-in and clean the version.
+    $versions = module_invoke_all('get_version', $file);
+    $versions = array_values(array_filter($versions));
+    $file->info['version'] = $versions[0];

that clearly sucks. what's the right way to do this?

2) it makes things easiest for hook_get_version() if we pass in the object we get back from module_rebuild_cache() and/or system_get_files_database(), which is how it is in the existing patch. however, is that too crazy? should we make the hook do a little more work if it needs to find the path to the module file to simplify what gets passed in (e.g. just pass in the array we parsed from the .info file)?

any other thoughts on the code in the patch?

thanks,
-derek

Comment #4

drewish CreditAttribution: drewish commented 12 June 2007 at 19:54

one way of getting a single value from a hook is to pass it in as a parameter and let the hook implementers modify it by reference.

i'm not sure what you've got there warrants its own module. it looks more like an include file to me...

Comment #5

dww

we/he/they

CreditAttribution: dww commented 12 June 2007 at 20:14

thanks for the input, drewish.

a) yeah, a reference probably would be better. it's more like hook_version_alter(), really. that's probably a better name for it, in fact. i just worry that'd confuse people since it doesn't follow the usual hook_*_alter() path (it's not a FAPI-style array, there's no id, etc).

b) re: a separate module vs. an include file. this code really, really, shouldn't be in core, for a bunch of reasons (see http://drupal.org/node/87298#comment-154604 (comment #5) for some some of my earlier thoughts on this problem, and also my meta comments at buytaert.net/linus-torvalds-on-git). therefore, it's a stand-alone module. also, i have plans to add a hook_form_alter() to it for the D5 version so it puts in nice human-readable version strings for the admin/build/modules form, and potentially other things to make this a little more reasonable to have to download and install if you want it.

also, keep in mind, this is *only* for the power-users who deploy from CVS, and it's a tiny, easily audited module. so, such power users should have no trouble running cvs co contributions/modules/cvs_deploy and enabling it, even if it doesn't do much (yet). ;)

Comment #6

dww

we/he/they

CreditAttribution: dww commented 12 June 2007 at 21:23

File	Size
update_status_no_cvs.patch_1.txt	9.67 KB

ok, here's a new, better patch that renames the hook to "hook_version_alter()" (since that's really what it's doing), and just passes the version string around by reference so that each hook can modify it directly, instead of messing with the array manipulation stuff. any final complaints?