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
Comment #1
pasqualleas I know profiles are used only at install, so it does not seems a logical place to me..
what is the bug exactly?
Comment #2
rickvug commentedWould 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.
Comment #3
adrinux commented+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.
Comment #4
EvanDonovan commented"[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.
Comment #5
catchNo 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".
Comment #6
johnalbinThis 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:
(23 files or directories in the root)
to:
(14 files or directories in the root)
Comment #7
johnalbinI'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:
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.
Comment #8
catchThinking about it, this needs a decision rather than a patch (and probably needs dww rather than a patch too), so putting back to CNR.
Comment #9
damien tournoud commentedI 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.
Comment #10
moshe weitzman commentedI'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.
Comment #11
dwwA) 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. ;)
Comment #12
catchwhitelist sounds good, changing scope.
Comment #13
alexanderpas commented+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:
where
distribution.infocontains the information required to let drupal find those direcories.Comment #14
geerlingguy commentedOne 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.
Comment #15
xanoWe 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.
Comment #16
dww-25 for the suggestion from #15. We already have that. It's called
package = Coreand 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 stickingversion = VERSIONin 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.
Comment #17
David_Rothstein commentedA 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?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.
Comment #18
pwolanin commentedInstall 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.
Comment #19
David_Rothstein commentedFor 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.
Comment #20
gábor hojtsyprofiles/all sounds like a good idea.
Comment #21
alanburke commentedSubscribe
Comment #22
pwolanin commentedsimple patch for profiles/all
Comment #23
alanburke commentedIssue 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
Comment #24
pwolanin commentedupdating title, since I don't see it in the cards to "prevent" anything at this point.
Comment #25
damien tournoud commentedNot 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.
Comment #26
pwolanin commented@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.
Comment #27
rickvug commentedI agree with @pwolanin. This would be a cleaner alternative for Acquia Drupal or any distribution that wants to offer multiple install profiles.
Comment #28
tstoecklerCan 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.
Comment #29
pwolanin commentedLooks 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
Comment #30
tstoecklerI'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.
Comment #31
David_Rothstein commentedAgreed, the code looks good, and I just did a quick test to make sure it works - it does.
Comment #32
tstoecklerShameless bump.
Comment #33
alex_b commentedPlacing 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.
Comment #34
yhahn commentedI 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:
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.
Comment #35
tstoecklerWhy 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.
Comment #36
yhahn commentedPlease see my comment in #34. Note that
profiles/allimplies that all the profiles on a distro are managed by the same maintainer which is not a safe assumption. E.g.profile/amay be maintained by Jane whileprofile/bmay be maintained by John and B may very well stop working on the basis of a module placed inprofiles/allby A.Comment #37
tstoecklerOf 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).
Comment #38
yhahn commentedThe difference between
profilesandsites/allis 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 intoprofiles/aand profile B intoprofiles/band they will both reliably work without conflicts. This is, for example, whydrush_makeand the drupal.org installation profile packaging does not allow an install profile to place projects outside ofprofiles/[x]-- it guarantees that a profile cannot mess with another profile's projects or anything insites/.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.
Comment #39
tstoecklerOkay, 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.
Comment #40
pwolanin commented@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.
Comment #41
yhahn commentedWorking 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.
Comment #42
tstoecklerRe #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)?
Comment #43
pwolanin commentedIt 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.
Comment #44
pwolanin commentedI 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
Comment #45
alex_b commentedI 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.
Comment #46
adrian commentedInstead 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.
Comment #47
adrian commentedWhat the other patch does is add a method like the following :
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.
Comment #48
adrian commentedUsing 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 :
That allowed the modules to be picked up.
Comment #49
adrian commentedPatch 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.
Comment #50
joachim commentedWould this be necessary with #22336: Move all core Drupal files under a /core folder to improve usability and upgrades?
Comment #51
tstoecklerIn #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/alldirectory, allow to have aprofilesdirectory 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/profilessites/all/profilessites/example.com/profilesIf I'm not mistaken that would bring all the benefits of what was discussed here, without the potential problems that were also raised here.
Comment #52
Bathie commentedHello!! The patch did not apply. Hope the reason is that it was too old.
Comment #53
robloachI'd much rather have #562042: Search for install profiles in sites/[all|site]/profiles folders, and move core profiles into /core/profiles.
Comment #54
tstoecklerYes, 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.
Comment #55
dwwAgreed. #562042: Search for install profiles in sites/[all|site]/profiles folders, and move core profiles into /core/profiles seems like the much better solution to this.