Fix hook_system_info_alter()

See also http://drupal.org/node/147000 about unifying the API related to .info files for modules and themes. Perhaps in the cleanup for #147000 we could introduce this alter hook?

Sorry, it's late. This should be "hook_info_alter()" for consistency. Actually, just "info" might not be descriptive enough, but at least it matches .info files. Better suggestions for the name of this hook are welcome.

except themes have .info files, now, too. ;)

hook_system_info_alter()?

Turns out the major refactoring of system_theme_data() and module_rebuild_cache() over at http://drupal.org/node/147000 isn't as promising as I'd hoped. Given the pace of core commits these days, I don't want to break other patches nor make this one dependent on #147000. So, here's a simple patch to add the hook I'm going to need to make update_status and cvs_deploy play nicely together in a clean way in D6. Trivial patch -- we just use drupal_alter() for this. We pass in a 2nd, read-only arg to the alter hook, which is the $file object we get back from drupal_system_listing(). Obviously, this will need some docs once it lands, but the critical thing is getting it in core before the API freeze.

Thanks,
-Derek

Changing title of issue to match the patch. ;)

Oh, and in case you're wondering, we pass in the $file object since a) we've already got it sitting right there and b) it's useful info to potentially use when deciding how to alter the .info data. For example, cvs_deploy needs to know the directory each module/theme lives in to find the CVS sticky tag to alter $info['version'] correctly. Hitting the disk is expensive as it is, so it's lame to call drupal_system_listing() over and over in each hook implementation just to get this data (again). Hence, we pass it on in as additional context when altering the $info array since it doesn't cost us anything, and it can potentially save a lot.

In case you're wondering, that'd be something like this:

function foo_system_info_alter(&$info, $file) {
  if ($file->name == 'system') {
    $info['dependencies'][] = 'pony';
  }
}

Slightly hacky, but works like a charm (just tested it myself). ;) Go ahead, just try and disable pony.module after that code runs....

Whoops, problem in the previous patch (not actually with the hook itself, but how we use the altered results). :( In module_rebuild_cache(), we end up saving the unaltered info array into the {system} table, instead of the altered one. :( This is wrong, since folks that just want to grab the info from the cache in the DB should get the fully altered data... I double checked and we get it right in system_theme_data(), it's only module_rebuild_cache() that does the wrong thing here. Attached patch corrects it. This is critical for the hook to really do its job, and will allow update_status to just get the cached info from {system} instead of pounding the filesystem over and over again.

Not sure what you read, but the most relevant issue is http://drupal.org/node/150316 -- the one where I ripped all the CVS-specific code and logic out of update_status and moved it into cvs_deploy. However, to summarize:

If you deploy from CVS and don't have the benefit of the packaging script's additions to the .info files, update_status doesn't have the information it needs to be able to fetch history and compare with what you've got. In particular, there's no version information. cvs_deploy can figure out the version based on the CVS tag you've checked out in your local workspace for any given module, and it has code to convert CVS tags into human-readable Drupal version numbers. Update status needs these version strings to compare against the versions in the release history, since that's how all the logic works, how it finds matches, compares against what you have installed, etc. However, there's no way for core's forthcoming update.module to get those nice version strings from cvs_deploy. Also, update.module's available updates report isn't a form (it's more like the status report at admin/logs/status), so cvs_deploy cannot form_alter() the nice version strings into place for the visual presentation of it. The only ways for update.module to get these version strings are:

a) if (module_exists('cvs_deploy')) -- we basically never have core check for contribs like this.

b) hook_version_alter() (this is what D5 update_status does in contrib). However, there are other things cvs_deploy might be able to figure out and fill in if it's not in the .info file, like a reasonable datestamp to compare against, the core compatibility term ("core = 6.x"), etc. Instead of a bunch of separate hooks for each thing we might need, I choose...

c) hook_system_info_alter() -- which gives us the chance to alter anything in the $info array at all. This seems like the cleanest approach to the cvs_deploy and update.module problems, and prevents introducing a bunch of very special-case hooks that probably no one but cvs_deploy will ever use. It's also potentially very powerful and flexibile for things I didn't think of at first, such as chx's idea about forcing a module to be required. Given that themes have .info files now, the patch for php compatibility enforcement, etc, it seems entirely possible that modules might find a need to programatically alter another module or theme's .info data in some way. Plus, we reuse the drupal_alter() method to ensure this works just like the other slew of alter hooks in D6.

Hope that helps,
-Derek

All I can say is: that's also a ton of work and a lot to maintain. I've thought about this approach numerous times, and it certainly is attractive for some reasons. But there are a bunch of nasty details to get right, and it's going to take a long time and a lot of careful effort to make it solid and trustworthy. I don't see that happening in the next 20 hours (since I'll have to be sleeping for some of those), and then I'm gone visiting family (yes, bad timing), and then the freeze will hit... So, I don't see how we're going to resolve this discussion before the freeze, and then it's going to be too late to change the API if we decide we need this hook after all. And we'll be mostly screwed for the deploy-from-CVS crowd if we do that and then can't resolve the problems with only server-side solutions.

Also, in context, we only invoke this hook when we're rebuilding the cached info data from disk, which is already rare and expensive, so I don't really buy the "it's cruft" argument. And, in spite of the fact that cvs_deploy + update is the currently most obvious use case, I'm convinced other interesting, imaginitive solutions will be built on top of this hook if it is allowed to survive.

I don't have time or energy to enumerate the problems of having CVS write back to the .info files at packaging time right now (I've been up all night on the final push to get a core-worthy patch for update.module (comment #25) submitted). When I have the chance, do you want that discussion to happen right here in this issue, or somewhere else?

Thanks,
-Derek

Well, it needs to be reviewed by the core committers, then. It doesn't need work, it needs a decision.

1) All of this assumes people like to deploy sites directly via CVS. We do this on d.o, and many other power users do, too. Every other comment in here assumes deploy-from-CVS. Also, for the sake of the argument, let's say it's the packaging script that would be doing this automated CVS commit stuff. That's an implementation detail -- it could live in a separate script if we wanted, but it doesn't really change anything. However, wherever it lives, the work has to happen *before* the packaging script is going to actually release stuff, so we might as well have it in the packaging script itself.

2) For the admin/build/(modules|themes) pages to work nicely, and for the update(_status)?.module's logic to compare what's available with what you have, we need the real, human-readable version string in the .info file.

3) We cannot trust contrib maintainers to always get this right if they have to do it manually. Period. Nor do we want to raise the barrier on making releases any higher than it already is.

4) Assuming we let the packaging script worry about this (which it does for the packages already), it would have to have CVS write-access to the contrib repository. When it's going to package up an official release, it would have to checkout the source from a given release tag, modify the .info file, commit the change to CVS and move the release tag. However, there's no reasonable way to do this. :( You can't commit a change to a tag, you need to commit on a given branch, then move the tag. However, it's impossible to know what branch a given release tag was made from (HEAD, DRUPAL-5---2, etc).

For this to work, the packaging script would have to do the following:
a) Create a new branch rooted at the release tag (at least for the .info file).
b) Edit the .info file and commit it on this branch.
c) Move the release tag for the .info file to point to the end of the branch

So, *every* release tag will make a new branch in contrib, leading to an explosion in the # of branches, etc. People are already completely confused by branches and tags, and this will make it much worse.

5) Things aren't much better for dev snapshots that already live on branches, either. :( We'd have to commit version = 6.x-1.x-dev back into the .info file in CVS. At least in this case, we know what branch to commit it to. Unfortunately, once something is in CVS, experience shows that people just cut and paste and re-use, even if it's wrong (witness how many contribs are doing version = VERSION in their info files already, if not package = Core - optional, etc). So, once the updated version strings are committed back to CVS, they'll get copied around and misused. Then, the packaging script will have to go in and correct things based on whatever the release nodes actually say. So, the .info files will be in flux, with race conditions, and if you happened to checkout at the wrong time, you get the wrong info because the .info says it's 5.x-2.x-dev, but really it's 6.x-1.x-dev, etc.

6) All of these automated commits and tags are going to generate a lot of noise for the various channels for monitoring CVS activity (email, RSS, http://drupal.org/cvs, etc). Unless we also put even more work into the xcvs-* scripts to hard-code the fact that they should basically ignore all automated activity. But then there will be changes in CVS that aren't visible via our usual tools. This sort of behind-the-scenes-but-still-visible behavior will probably lead to much confusion, too.

7) Purely from the infrastructure side, I'm not thrilled about running the packaging script as a user that has full write access to the entire CVS repository. I like that it's now read-only, and think it should stay that way.

I could keep going, but I'm sure Dries has more important things to read and think about...

Hope that clarifies why I supported a tiny patch to add an alter hook so we could solve these problems via contrib, instead.

p.s. The hook could be useful for solving other problems, too.

Bump. The API freeze has come and gone and this API change was NOT rolled back (thankfully). ;) cvs_deploy.module in D6 depends on it. In spite of the fact that I spent much time explaining the situation and problems here, there was no reply (which is fine, I know we're all busy). So, at least for D6, cvs_deploy.module is the way this is going to work. Therefore, all we need is my patch from #12 so that the API works in all cases as expected. It was a simple oversight from the original patch, and if the API is there and this patch is not, it's a bug and the API is broken.

I just double-checked and #12 still applies (without fuzz, even!) to the latest HEAD. This is RTBC. It's even been independently tested by frando in #17. So, I'm not really RTBC'ing my own patch. ;) I'm just re-RTBC'ing...

Thanks!