Despite documentation, many users still put contributed modules in /modules - this causes bugs due to module weight/ordering, and of course it's bad practice, not good for upgrade paths etc.

So, as well as hiding them from the system modules page, what about moving them to profiles/default/modules (and the same for themes). Then they're a little more out of the way, and it seems like a more logical place for them overall.

No patch since this will be a cvs move - we don't want to lose history for these, so marking straight to needs review.

Comments

pasqualle’s picture

Status: Needs review » Postponed (maintainer needs more info)

as I know profiles are used only at install, so it does not seems a logical place to me..

what is the bug exactly?

rickvug’s picture

Would this make the modules only work with the default install profile? What about expert or any other profile that may be added? I've always assumed that the modules in profile_name/modules only worked with sites running that profile, much in the same way example.com/modules only work with example.com. Correct me if I'm wrong or I'm not understanding the issue at hand.

adrinux’s picture

Title: Move /modules and /themes to profiles/default/ » Move /modules and /themes to a subfolder
Component: usability » other
Category: bug » task

+1 on the move out of the top level - certainly new users look at /themes and often think that's where custom or downloaded themes should go. I can see the same happening with modules.

At this point, who, other than a core developer needs to even look in /modules and /themes? They just clutter up the directory and confuse new users. I was going to suggest /includes/modules and /includes/themes. Certainly /misc doesn't seem right. Other than that it would have to be a new folder to contain both (/core?).

Changing the title to something more generic, to reflect debate about where to move them. And changing from bug -> task which seems more appropriate. Probably too late in the 7.x cycle for this though...

* Personally I'd also move scripts into a /devel subfolder, or misc, and all those top level txt files into a /docs folder, misc could do with some tidying up (/javascript and /images ? hardly misc items...). What arrangement we have has grown organically over several years and could do with some tidying up. Doing so would make things easier and less daunting to new users, making it more obvious where to find things.
When you've got used to the current shambolic state it's easy to think it's not a problem. You really need to approach it with the eyes of someone opening that 'drupal-X.x' folder for the first time.

EvanDonovan’s picture

Status: Postponed (maintainer needs more info) » Needs review
Issue tags: +Usability

"[W]ho, other than a core developer needs to even look in /modules and /themes?" People who get PHP errors & want to find out where they're coming from. People who are developing for contrib and want to look at the core APIs. People who are basing a theme off of one of the core themes.

That said, I think this makes sense. core/modules & core/themes would be more logical, and would imply that you shouldn't put your own modules there. sites/all is not an obvious place for anything, but if the installation instructions make it clear enough, that shouldn't be a problem.

To go with those, the files from /misc could go into core/js and core/images. /includes could move to core/includes, to go along with core/modules & core/themes. The text files could go into /docs. The scripts, I think, should stay where they are.

catch’s picture

Status: Needs review » Active

No patch here.

core/ seems like a decent idea - there's a chance there'd be confusion with /includes being in core but not in the core folder, but at least it says "don't put anything else here, thanks".

johnalbin’s picture

This is a huge usability improvement, IMO. +1 But it would require a fairly big patch as well since a ton of paths would change.

And breaking the CVS history would massively suck for developers. But the earlier we do this, the more CVS history we'll have later. Actually, I think there is some CVS voodoo you can do to preserve history, but damned if I know how to do that. I think we'd have to hire dww for a week or more to preserve the history and lock out CVS commits during that timeframe.

Regarding a docs/ directory: we still need to keep the COPYRIGHT and LICENSE docs in the root for legal reasons. And we'd still need to alert people about where to look for INSTALL/UPGRADE info. So, how about creating a README.txt file in the root which just points the reader to the docs directory for install upgrade info.

So we'd move from:

.htaccess
CHANGELOG.txt
COPYRIGHT.txt
INSTALL.mysql.txt
INSTALL.pgsql.txt
INSTALL.sqlite.txt
INSTALL.txt
LICENSE.txt
MAINTAINERS.txt
UPGRADE.txt
cron.php
includes/
index.php
install.php
misc/
modules/
profiles/
robots.txt
scripts/
sites/all/modules/
sites/all/themes/
sites/default/
themes/
update.php
xmlrpc.php

(23 files or directories in the root)

to:

.htaccess
COPYRIGHT.txt
LICENSE.txt
README.txt
core/includes/
core/misc/
core/modules/
core/scripts/
core/themes/
cron.php
docs/CHANGELOG.txt
docs/INSTALL.mysql.txt
docs/INSTALL.pgsql.txt
docs/INSTALL.sqlite.txt
docs/INSTALL.txt
docs/MAINTAINERS.txt
docs/UPGRADE.txt
index.php
install.php
profiles/
robots.txt
sites/all/modules/
sites/all/themes/
sites/default/
update.php
xmlrpc.php

(14 files or directories in the root)

johnalbin’s picture

I'd also like to point out that with the layout proposed above, that users when thinking of "Ok, where do I put my contrib modules/themes?" would be confronted with the following directory choices:

  • core
  • docs (unlikely candidate)
  • profiles
  • sites

So its more likely that they would find sites/README.txt and, thus, sites/all/modules and sites/all/themes. We should probably add some warnings in a core/README.txt file and in the root README.txt.

catch’s picture

Status: Active » Needs review

Thinking about it, this needs a decision rather than a patch (and probably needs dww rather than a patch too), so putting back to CNR.

damien tournoud’s picture

I support that change. The only issue is that it would break any patch we have in the queue right now, so I suggest we do that right before the release of an unstable version.

By the way, I'm unsure on messing up with the history. This would probably break all non-CVS mirrors we have (including my GIT mirror), as those would probably not be able to handle the messed up history.

moshe weitzman’s picture

I'm fine with this. A less intrusive change would be to whitelist core modules and core themes for the /modules and /themes dirs so that any non core stuff in those dirs would no longer get recognized. right now, they get recognized but have incorrect weight which causes subtle bugs. i gues we could both move these dirs and add a whitelist if we want.

dww’s picture

A) update.module already whitelists core themes (see #490562: Hard-coded list of core themes for update module is now stale). We could generalize this, and make that whitelist part of system.module, and add a module whitelist, and then tell system module to ignore everything in /modules and /themes that's not on the list. That'd be nice for update.module to defend against contrib developers to say their modules belong to the "Core" package, too...

B) Just because we have core/themes doesn't prevent people from installing things there. So, update module is still going to want its whitelist, even if this change goes through. All a subdir would do would potentially discourage people from putting their stuff there. The idea that core/README.txt will prevent that is about as hopeful as the idea that modules/README.txt prevents it already. ;)

C) Yes, you can preserve CVS history when moving files around like this. In fact, we preserved all the history when node.module moved from modules/node.module to modules/node/node.module, etc. No, you don't need to hire me for a week (not that I'd turn it down if anyone's feeling charitable). There's a cvs_rename script in the tricks directory, and a few different issues where folks have used it. You do need someone with shell access to cvs.d.o to run it. It is a bit of a hack. One of CVS's biggest failings is in renaming files. :( I don't know how badly this hack confuses git mirrors, but that doesn't seem like a terribly critical reason to avoid doing this. The git mirror is a mirror, after all.

All that said, I don't think this is worth doing. A whitelist would be more effective at actually preventing the problems that can come. We could then have a nice big drupal_set_message('error') about it when we find a non-core theme or module in the wrong place, and print that at admin/build/(modules|themes), which is a far more more visible place for the warning than any README.txt file will ever be. This change would be a big hassle for people used to finding and working with core in the current locations. It'd break every patch in the queue, and probably hundreds or maybe thousands of little scripts, aliases, and shortcuts that people who contribute to core use to streamline their work. We can preserve the CVS history, but it's a hack, and it potentially confuses some things. Just lock out non-core and warn people when they try to do the wrong thing, and we're way better off, IMHO.

p.s. Another evil thought: the packaging script could ship the modules and theme directories chmod 555 by default. ;)

catch’s picture

Title: Move /modules and /themes to a subfolder » Prevent contrib being placed in /modules and /themes
Status: Needs review » Needs work

whitelist sounds good, changing scope.

alexanderpas’s picture

+1 for movement of modules and themes etc. to /core

-1 (currently) to disable additions to those directories.(think of acquia drupal, and core deevelopement)

+1 for whitelisting modules in such a way, that they can't be added to the Core group, without modifying the whitelist, but instead, will be added to the Core (Development) group.

how about, allowing, for example:

/acquia/themes/
/acquia/modules/
/distribution.info

where distribution.info contains the information required to let drupal find those direcories.

geerlingguy’s picture

One of the things I like about Drupal is the simplicity of the structure of files in a Drupal install. It makes sense to me. Doing this would make even more sense. The more structured the site files are, the more likely I'd be able to find the file I'm looking for.

xano’s picture

We could add a setting to info files that indicates if a module or theme is part of core or not. This would allow forks or specific packages to put all their core stuff in /modules and /themes as well. On the other hand lazy or novice contrib maintainers may add this setting to their info files as well.

dww’s picture

-25 for the suggestion from #15. We already have that. It's called package = Core and contrib maintainers copy that a lot. If the motto is "do as core does", then people take that literally and copy core's .info files verbatim. We have the same problem with people sticking version = VERSION in their .info files. Sadly, we just can't rely on .info files -- contrib developers are too inconsistent and unreliable. Hence, the proposal for a whitelist in core, which can't be broken.

Re: forks and externals distributions... there's profiles/foo/modules and profiles/foo/themes for that. See What's the best practice for where modules should be located inside an installation profile? for more. And, if we have a whitelist, it wouldn't be Drupal without an alter hook. ;)

To the folks who say "yay, another level deeper in the directory tree would be so much better for UX..."

- Where's the proof of that claim?

- I can *prove* that moving all these files around is going to be a fairly big hassle on a few different levels.

It'd have to be a pretty clear, big win for UX and technical reasons before I was on board with the pain in the ass it's going to cause. Moving core modules into their own subdirs was obviously a clear technical win (first for .info files, then for page split .inc files, and now for .test files, etc, etc). That was worth the hassle, even though CVS is a crappy tool for this job. In this case, it's just an unsubstantiated assumption that people will logically know what to do if there's another "core" directory in the tree. Show me a usability study about that and maybe I'll believe you. ;)

Can we at least *start* with patches for the whitelist approach, please? The technical problem is that if people put contrib modules in there, things can get screwy with weights and unexpected behavior. We can easily prevent that problem, and provide clear feedback to the site admin/builder that they put stuff in the wrong place, in ways they can't ignore. Let's start there, and assess if there's still a problem after that. Thanks.

David_Rothstein’s picture

Re: forks and externals distributions... there's profiles/foo/modules and profiles/foo/themes for that.

A distribution is not the same thing as an install profile, though. Requiring distributions to put their modules in the profiles/ directory would make it difficult for users to switch to a different distribution after they have already installed Drupal. It also means that a single Drupal distribution cannot easily contain more than one install profile.

Putting stuff in modules/acquia (or, more generally, modules/somedistribution) is indeed a bit of a hack, but given the way things currently work, it's basically the only workable option. So perhaps it would be better to look at ways of improving that before going too far down the path of a whitelist?

The technical problem is that if people put contrib modules in there, things can get screwy with weights and unexpected behavior.

Well, all core modules have a weight of 0, and any module which relies on running after those is supposed to explicitly set a higher weight, right? Many of the more popular contrib modules do this already. For the ones that don't, relying on the filename sorting sounds like a bad idea -- see also #211439: (Regression) module_list() function does not sort correctly.

pwolanin’s picture

Install profiles are currently pretty useless, so I hope we are going to really do something different there.

If the whitelist approach goes forward, an easy short-term fix/intermediate fix would be to enable a profiles/all directory, which would be for modules and themes possibly shared by multiple profiles and an initial place for a distribution to logically drop all its files.

David_Rothstein’s picture

Well, all core modules have a weight of 0

For the record, I am wrong -- all core modules do not have a weight of 0 anymore (forum and block appear to be the exceptions).... however, the larger point remains :) Contrib modules should use weights when they need to control ordering, and Drupal should stop sorting by filename, as per #211439: (Regression) module_list() function does not sort correctly, and that would seem to eliminate the "bug" aspect of this issue.

We'd still want to discourage people from installing contrib modules in the core directory somehow, but that would be for best practice reasons only, I think.

gábor hojtsy’s picture

profiles/all sounds like a good idea.

alanburke’s picture

Subscribe

pwolanin’s picture

Status: Needs work » Needs review
StatusFileSize
new548 bytes

simple patch for profiles/all

alanburke’s picture

Issue Summary.
I could be completely wrong about all this, but having spent some time trying to get my head around what installation profiles are, what distributions are, and what Drush Make can do...

At the moment,
A distribution [not necessarily built with Drush Make] can place its modules in
a. /modules [ I believe Acquia Drupal does this?]
b. /sites/all/modules/
c. /profiles/profile-name/modules

With Drush make, that's limited to
/profiles/profile-name/modules
at the moment.

So a distribution which wants to have multiple installation profiles, must either
a. Build it themselves, putting the modules wherever they want in a or b
b. Duplicate the modules in two separate profile directories.

The patch at #22 would let a distribution put modules in
/profiles/all/modules
and all installation profiles within that distribution could share them.
So +1 for that.
[Drush Make would have to add support for that too.]

Alan

pwolanin’s picture

Title: Prevent contrib being placed in /modules and /themes » Support contrib being placed in profiles/all

updating title, since I don't see it in the cards to "prevent" anything at this point.

damien tournoud’s picture

Not sure I get the point. Do we really want to allow several installation profile to "merge" modules in profiles/all? I don't really see the difference with sites/all. I might be missing something, otherwise -1 here.

pwolanin’s picture

@Damien - the goal is to be able to have a distribution that includes multiple install profiles that share modules and themes. A distribution should not be dropping stuff into sites/all to prevent confusion with the user's downloads, and above people object to putting it unders /modules and /themes. So this gives us a viable place to put modules and themes that's outside of core's area and outside the user's area.

rickvug’s picture

I agree with @pwolanin. This would be a cleaner alternative for Acquia Drupal or any distribution that wants to offer multiple install profiles.

tstoeckler’s picture

Status: Needs review » Needs work

Can someone claim http://drupal.org/project/all then so someone doesn't make a profile out of it. :)
Seriously though, I think this is a good step for profiles/distributions. The current behavior is the last thing that messes up our otherwise strict seperation of core vs. profiles vs. sites.

That having been said, the function header needs to be updated to reflect this change. I'm also wondering if we should leave some of the rationale for this change as an inline comment, as (going from the comments in this issue) it doesn't seem to be self evident.

pwolanin’s picture

Status: Needs work » Needs review
StatusFileSize
new1.99 KB

Looks like Benjamin Melançon took it as a module - so it's not a profile and it would work fine as sites/all/modules/all or profiles/all/modules/all

This patch fixes up the code comments as suggested, and also puts the new code before the profile-specific search. That way, a module in profiles/ACTIVE_PROFILE/modules will be used in preference to profiles/all/modules

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

I'm going to boldly RTBC because the code is sound.
People that were against this approach earlier (Damien Tournoud, ...) haven't commented lately so I don't know if they are convinced or if they just haven't senn this lately.

David_Rothstein’s picture

Agreed, the code looks good, and I just did a quick test to make sure it works - it does.

tstoeckler’s picture

Shameless bump.

alex_b’s picture

Status: Reviewed & tested by the community » Needs work

Placing modules in profiles/all suggests that profiles are able to share modules or themes which they just can't in any meaningful way.

The fallacy of the argument in #26 / #27 is that there can be somehow an assumption which installation profiles can be present in the profiles/ directory. For instance, add an installation profile that requires a different Views version in profiles/all and the model breaks.

For sharing modules/themes in controlled environments we have sites/all.

yhahn’s picture

I would add to #33 that a better solution to the use case at hand - having a distribution ship with multiple install profiles that are managed together and expect the same dependencies / module versions - would be to better distinguish between an install profile's search path and the install profile's install/update routines.

For example, if there were such a distinction a set of profiles (I will call them apples, bananas and oranges) that share the same search path might look like this:

profiles/fruits/fruits.make
profiles/fruits/apples.install
profiles/fruits/apples.info
profiles/fruits/bananas.install
profiles/fruits/bananas.info
profiles/fruits/oranges.install
profiles/fruits/oranges.info

Much like how a single Drupal.org project can contain multiple modules, this would allow a profile project to contain different "variant" install profiles though they all share and have jurisdiction to using the projects found in profiles/fruits.

The main change required for this would be to eliminate the convention/assumption that the active install profile is both the search path directory and the filename of the info/install files within that directory. To this end see adrian's work on #911354-7: Tests in profiles/[name]/modules cannot be run and cannot use a different profile for running tests.

tstoeckler’s picture

Status: Needs work » Needs review

Placing modules in profiles/all suggests that profiles are able to share modules or themes which they just can't in any meaningful way.

Why not? Differing module versions is as much a problem as it is in the sites/all sites/$site sphere, so no real counter-argument there.
Also #34 might make sense for certain use-cases, but it also might not make sense, if a profile itself consists of a bunch of more files, for instance.
I also don't see how you'd solve the Acquia Drupal use-case with that. And conceptionally profiles/all is exactly what they want: They want modules and themes you always have available, no matter what your installation profile is, so to speak for 'all profiles', hence 'profiles/all'.
Setting back to needs review at least, but codewise this is still RTBC.

yhahn’s picture

Status: Needs review » Needs work

Please see my comment in #34. Note that profiles/all implies that all the profiles on a distro are managed by the same maintainer which is not a safe assumption. E.g. profile/a may be maintained by Jane while profile/b may be maintained by John and B may very well stop working on the basis of a module placed in profiles/all by A.

tstoeckler’s picture

Of course these things can happen, but they can also happen with sites/all in exactly the same way. Just like with sites/all you should only ever put modules there if you know what you're doing. Or you download something like Acquia Drupal and never touch it. Also the reference to multi-project modules is invalid. Modules are never different variants of each other and they are definitely not exclusive to each other which profiles are.

I'm going to play status ping-pong, but this is definitely needs review (at least).

yhahn’s picture

The difference between profiles and sites/all is that the install profile system was explicitly designed to allow profiles to have control over the modules they use.

At the moment a user doesn't need to know exactly what she is doing around the profiles/ directory. She can download and build profile A from drupal.org into profiles/a and profile B into profiles/b and they will both reliably work without conflicts. This is, for example, why drush_make and the drupal.org installation profile packaging does not allow an install profile to place projects outside of profiles/[x] -- it guarantees that a profile cannot mess with another profile's projects or anything in sites/.

As for packaging multiple profiles and distinguishing between profiles install routines and search paths, yes this is new ground. The comparison to modules is an analogy, not a literal comparison.

tstoeckler’s picture

Okay, thanks for that explanation. I now understand your reasoning, but I still disagree.
First of all the scenario you describe in #38 will work perfectly with this patch. Modules in /profiles/$profile override those in /profiles/all (just like with sites), so you can and probably should (in most cases) develop your profiles completely agnostic to each other. Putting stuff in profiles/all should be the rarer case and we should discourage people from that in most cases (and of course keep the limitation for d.o-profiles) but essentially it's the same as with the sites/ directory.
You can (and should*) stay for yourself in your own sites/$site (profiles/$profile) directory, but you can also put stuff in sites/all (profiles/all) except then you're opening a whole can of worms.

*I've never understood why we encourage user's to put stuff in sites/all by default, for exactly that reason. It's a pain as soon as you have to update one module in sites/all.

pwolanin’s picture

Status: Needs work » Needs review

@yhan - the exact use case for this patch's functionality is when "all the profiles on a distro are managed by the same maintainer".

In fact, I don't see how much of anything else makes sense unless you are in a very exploratory mode and installing different profiles and dropping the database in between. Or are you imagining this in the context of multi-site where different sites would be using different profiles? In any case - if you are doing that you're an expert.

yhahn’s picture

Working with multiple install profiles in a single drupal codebase is increasingly common and with a variety of uses like starter sites (http://drupal.org/project/cod), products (http://drupal.org/project/openpublish), and testing environment profiles (http://drupal.org/project/feeds_test) I think we will see an increase in the need to have multiple profiles available at once. It seems given that they may often not be maintained by the same individuals.

As noted in #34 I am not against this use case. But I think it should be implemented in a way that can actually be used by the existing install profile repository structure, Drupal.org packaging and code compatibilty concepts. As noted in #39 the current implementation could not be used by a current d.o install profile project, nor would it be encouraged for people creating new distros on Drupal.org. As it is now, you need your own repository mirroring Drupal core, all your contrib projects and your custom install profiles to make use of this functionality.

tstoeckler’s picture

Re #41: That is because what you describe is, I think, one level above what we currently support on d.o. Right now your limited to /profiles/[profile-name]/*, now including modules and themes, etc. Building multi-profile distributions is something that, while being integral for keeping development of distros on d.o in the long run, is something that should be discussed seperately from and probably in the infra queue. This patch, though, would actually make something like that possible. Of course the concerns you mentioned are very valid und would have to be discussed, but again, I'm not sure that this is the issue for that.

Also, did I read #41 correctly, that you are basically in agreement that this patch is an improvement of the current situation (no more /modules/[distro-name]/* hacks)?

pwolanin’s picture

It sounds like an alternative suggestion would be supporting the sharing of modules within a group of profiles like:

/profiles/[group-name]/[profile-name#1]/modules

/profiles/[group-name]/[profile-name#2]/modules

/profiles/[group-name]/all/modules

so either profile #1 or #2 could use the "all" modules for the group. In theory this is possibly a better architecture, it's just much more than the trivial change this patch requires, and it's possibly a little late to be adding that feature (but honestly, if that feature is fully BC, seems like we could add it to D6 AND D7). Even in the context of this better architecture, this patch could be considered a supporting the case where [group-name] is the empty string, so I'm still tempted to set it back to RTBC and consider grouping a new issue.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

I think we should consider this still RTBC as a quick and easy fix.

For more substantial changes let's discuss at: #922726: Support grouping of install profiles or multiple profiles in one directory that share modules

alex_b’s picture

Status: Reviewed & tested by the community » Needs review

I just talked to pwolanin on IRC. While we didn't have the time to hammer out a concrete proposal, we agreed to hold on the profiles/all shortcut until we have a better understanding of what an ideal solution would look like. Like suggested in #44, let's continue this conversation over at #922726: Support grouping of install profiles or multiple profiles in one directory that share modules.

The use case is clear: multiple profiles should be able to bundle their dependencies in a directory safe from the 'general usage' of sites/all.

adrian’s picture

Instead of adding another special hardcoded case, this (related) problem is trying to use the approach of making the search paths configurable :

http://drupal.org/node/911354#comment-3481270

This would mean that we could implement additional search paths much more easily.

adrian’s picture

What the other patch does is add a method like the following :

drupal_add_system_directory('key', 'profiles/$x');

This could be used in a hook_init in your install profile, or specified in your settings.php or in sites.php and so forth.
The only requirement is that the paths need to be configured before drupal_system_listing is run on the registry build.

adrian’s picture

Using the patch on this issue :
http://drupal.org/node/911354#comment-3504640

I was able to add the following to my install profile myprofile.profile :

  function myprofile_init() {
    drupal_add_system_directory('profiles_all', 'profiles/all');
  }

That allowed the modules to be picked up.

adrian’s picture

Patch 17 on #911354: Tests in profiles/[name]/modules cannot be run and cannot use a different profile for running tests allows this issue to be cleanly solved in contrib, without needing any use case specific code in core.

The API in that patch now makes provisions for precedence, allowing you to safely determine which modules should have precedence over your own custom paths. You can add the additional path to be considered before profile/$profilename, by just specifying the scope as 'before:profile'.

If you wanted your directories to take precedence over the profile provided ones you could use 'after:profile' instead.

joachim’s picture

tstoeckler’s picture

In #22336: Move all core Drupal files under a /core folder to improve usability and upgrades I think a different approach was discussed:
Instead of having a profiles/all directory, allow to have a profiles directory per-site. I.e. with #22336: Move all core Drupal files under a /core folder to improve usability and upgrades you would have the following folders:

  • core/profiles
  • sites/all/profiles
  • sites/example.com/profiles

If I'm not mistaken that would bring all the benefits of what was discussed here, without the potential problems that were also raised here.

Bathie’s picture

Hello!! The patch did not apply. Hope the reason is that it was too old.

tstoeckler’s picture

Yes, I think this is pretty much won't fix.
Especially after the core move /profiles should be split into /core/profiles and /sites/all/profiles, which the above issue is basically about IIRC.

dww’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Closed (won't fix)