Support subdirectories inside modules

Subscribing. This is a welcome improvement. Will try and test later.

Now that you mention it, I agree this is a (minor) problem. However, I'm not sure I like this approach, and you're misrepresenting the problem...

but update status requires all contributed modules to live directly under the modules directory

This isn't true. The correct statement is: "If you deploy directly from CVS and your .info file doesn't define 'project', all contributed modules must be in a subdirectory that includes 'modules' at the path directly above where the modules live." So, your 3 options for using update_status without error are:

A) get tarballs from d.o which have project automatically defined in the .info files via the packaging script.
B) define 'project' yourself in the .info files.
C) put things in a subdirectory with "modules" in the path. For example:

/index.php
/modules
/modules/node
/modules/contributions
/modules/contributions/modules
/modules/contributions/modules/views
/modules/contributions/modules/cck
...

When you checkout directly from CVS, you're going to have a "contributions/modules/views" directory anyway, unless you go out of your way to undo that.

So, now that we're all clear on the real problem here (sites that deploy from CVS and go out of their way to alter their directory structure), I think the better solution might be to fix cvs_deploy.module to better support this. True, in D5, that'll also mean a small patch to update_status, but in D6, we've got hook_system_info_alter() to save us. Basically, in D5, update_status itself provides a hook that cvs_deploy depends on, hook_version_alter(). It's specific to update_status, but it works. In D6, since I was in core anyway, I could do the altering more cleanly and completely -- any module can alter what other modules (and all of core) see in the .info files.

Therefore, to fix this in D6, all we have to do is put some logic into cvs_deploy to alter the .info file to add a 'project' line. In D5, we'll have to get update_status and cvs_deploy to agree, namely, we either need to expand hook_version_alter() or add hook_info_alter() or something. Then, all the logic for dealing with this problem can live right where it belongs, in cvs_deploy.

Once we know we're dealing with CVS deployments, we can solve it automatically by inspecting the CVS/Repository files and figuring out definitively what project a given module belongs to, since we can safely assume that regardless of where in the filesystem we are, CVS/Repository will include "contributions/modules/[project_name]" (and maybe some extra directories on the end if we happen to be inside a subdirectory of the project, like in "views/modules" or something.

See what I mean?

Oh yeah, I totally forgot about sites/all/modules in my example. ;) You'd never put core modules there, and that's really the best place (or sites/[site]/modules) for your contrib modules. And, if you do that, update_status Just Works(tm). So, there's your best practice: leave core alone, and put contribs in sites/all/modules. Then, there's no need to patch anything to get this working.

I'm getting dangerously close to pulling the "won't fix" trigger now... ;)

If you're stuck on believing this needs to be fixed, I suggest you:

A) Take a look at how hook_version_alter() is currently implemented.

B) Decide if you want to expand that to include altering other parts of the .info file, or introduce a slightly more general-purpose hook for this and leave hook_version_alter() alone (on the off chance that someone else wrote a module that implements that hook for some reason).

C) Attach a patch to this issue that alters both update_status.module and cvs_deploy.module that includes the appropriate changes based on what you come up with in (B).

If you do all that, and get someone else to review and test it, I'll consider taking another look at this issue. Otherwise, I'm going to "won't fix" it.

Cheers,
-Derek

Oh yeah, I totally forgot about sites/all/modules in my example. ;) You'd never put core modules there, and that's really the best place (or sites/[site]/modules) for your contrib modules. And, if you do that, update_status Just Works(tm). So, there's your best practice: leave core alone, and put contribs in sites/all/modules. Then, there's no need to patch anything to get this working.

No... per your #2, update_status/cvs_deploy requires the level just above to be called "modules" which prevents you from doing nice separation like:

- sites/all/modules/contrib/views
- sites/all/modules/contrib/cck
- sites/all/modules/custom/mysite
- sites/all/modules/custom/foo

(FYI: This is the best practice recommended in Pro Drupal Development, so it's wide-spread.)

It's nice to keep the custom modules in their own directory, because then update_status ignores them. However, currently we have to do either this:

- sites/all/modules/views
- sites/all/modules/cck
- sites/all/modules/custom/foo
- sites/all/modules/custom/mysite

... which is annoying, because it's very difficult to see the "custom" folder in amongst 20 other folders.

or:

- sites/all/modules/contrib/modules/views
- sites/all/modules/contrib/modules/cck
- sites/all/modules/custom/foo
- sites/all/modules/custom/mysite

which is also annoying, because *obviously* if I'm in sites/all/modules, I'm talking about a module. :P Why have to nest the files even deeper needlessly?

@webchick: ahh, ok, contrib vs. custom... makes sense. sorry, I missed that important part of the first sentence in the original issue. ;)

@quicksketch: i understand the point of your patch. you're punting to the admin to have to go in and fill out a relatively confusing setting to get this somewhat obscure case (mix of CVS deploy and custom modules) to behave nicely. since the root of the problem is not having the right data in the .info files when you deploy from CVS, i'd rather we fix it there, automatically, without the admin having to fill out any weird setting about it.

splitting your sites/all/modules into "contrib" and "custom" is both a common and best practice. Would this still be possible with some combination of hook_version_alter() and .info files? It seems with the lack of packaging information, cvs_deploy will need some additional help somewhere.

Please re-read what I've written in this thread. I already answered this question. "custom" modules have no version info (at least nothing update_status can work with), so who cares if update_status is ignoring them? If you actually have custom modules where you provide your own server to distribute them, are rigorous with the version numbers, and run the server-side code for update_status, then you already have to put some stuff in your .info files to point to your custom server, in which case, you can just define 'project' directly. So, the only problem here are the contrib modules you're deploying from CVS. And those already have all the data they need, in the form of the CVS/Repository file. Please look in sites/all/modules/contrib/views/CVS/Repository on a site where you deployed views from CVS to see what I'm talking about. No, the current hook_version_alter() is *NOT* sufficient, as I explained in detail above...

Using sites/[site]/modules is not sufficient for multisite. We have custom modules that are run on all of our 30 or so (and growing) multisites.

@quicksketch: yes, that's exactly what I proposed in #2 above:

Once we know we're dealing with CVS deployments, we can solve it automatically by inspecting the CVS/Repository files and figuring out definitively what project a given module belongs to, since we can safely assume that regardless of where in the filesystem we are, CVS/Repository will include "contributions/modules/[project_name]"

I guess I wasn't perfectly clear, but I had mind that we'd open CVS/Repository (using similar logic like when we're trying to find CVS/Tag), read the line, split it on '/', grab the 3rd field, and use that.

Also, doesn't really matter what queue this issue lives in, since the patch will have to touch both modules (at least in D5). True, in D6, we can solve this entirely via cvs_deploy, so I guess it makes more sense for the issue to live in cvs_deploy land, even if the patch has to reach back and touch update_status, too. ;)

@merlinofchaos: not entirely sure what your point is about sites/[site]/modules... can you elaborate? Are you just replying to my comment in #2 where I was confused about the split between contrib and custom being the point of this issue? "You'd never put core modules there, and that's really the best place (or sites/[site]/modules) for your contrib modules"... I just meant that sites/*/modules was the best place for contribs, either "all" or [site], and that easily kept core and non-core modules apart. I didn't notice custom vs. contrib, and I agree, you can't use all vs. [site] to segregate those...

Note: I just changed the API between update_status and cvs_deploy to fix http://drupal.org/node/158526 (see in particular http://drupal.org/node/158526#comment-695600 (comment #6) for details). So, this is now possible in both branches without further changes to update_status. The API change will go out with update_status 5.x-2.1, which should be out relatively soon now...

The current patch has nothing to do with how this should be solved, so let's start over and call this active...

Ugh, this is made very difficult by the fallback code for trying to determine the project based on the directory. :( See http://drupal.org/node/211403 for ripping it out of both update.module in core and update_status 5.x-2.*.

Here's a patch for 5.x that works if you apply the patch from http://drupal.org/node/211403 to update_status.

Re-rolled for DRUPAL-5 now that I committed http://drupal.org/node/211259
Also includes a version for D6 in HEAD, assuming http://drupal.org/node/211403 lands.

In the process of testing http://drupal.org/node/211403, I confirmed that this patch is ready to go. :)

Whoops, the D6 patch was broken on the admin/build/modules page since in that case, we hit hook_system_info_alter() before we get a chance to require update.compare.inc.

Note: this is sort of stupid. We should just decide core vs. contrib directly via the CVS/Repository file, instead of calling update_get_project_name() here. See http://drupal.org/node/211403#comment-700370. Even if *that* fall back code won't be purged for D6, there's no reason cvs_deploy can't actually set the right value itself here.

Oh, duh, the patch... ;)

Even better version for D6 (again, assuming http://drupal.org/node/211403 lands).

And a backport of #21 to DRUPAL-5.

Tested #21 with the core patch referenced and no more fatal errors on admin/build/modules. Also confirmed that image module AND image attach are found under sites/all/modules/contrib/image.

RTBC, for real this time. ;)

off-topic, but just a note here as it's discussion by people with opinions on best practices for subdirectories within /modules :-)
Would anyone care to review/edit/correct the recently added handbook on directory precedence WRT /modules?

It arises from a recent thread about Where to put your modules.

I was going to expand it even further with an explanation of my

site/{sitename}/modules/contributions
site/{sitename}/modules/modified
site/{sitename}/modules/custom

methodology, but I wasn't aware that it was indeed 'best practice' or that it had been previously documented anywhere. :-)

I'd add that I find

modules/contributions
modules/custom

To be vastly useful, as I can anchor the two dirs to different cvs repositories (one my own), and checkout from either without too much effort.

</off-topic> sorry for the hijack

x-post on the status. Sorry.

Committed #22 to DRUPAL-5. Just waiting for http://drupal.org/node/211403 to land and then #21 will go into HEAD.

p.s. @dman I'm already aware of that thread: see http://drupal.org/node/211980#comment-697960 for example.

Yay, #211403 just landed, so I committed #21 to HEAD.