With the addition of info files and install hooks to installation profiles, they gain a status equivalent to that of modules and themes. However, in order to make an installation profile available, unlike modules or themes, it must apparently be added to the profiles/
folder in the code-base itself.
This may work (more or less) when the profile and code-base are distributed together as a third-party package, but if eg. a profile is supposed to be downloaded by itself or created by the local web admin, we want it to be outside the code base, for the same reason as other addons. That's exactly what sites/all/
or sites/[conf_path]
is for.
I propose that install_find_profiles() be extended to use the same search pattern as module and theme discovery, looking in ./profiles/
, ./sites/all/profiles/
and ./sites/[conf_path]/profiles/
(in order of increasing priority, to allow overrides).
Comment | File | Size | Author |
---|---|---|---|
#61 | platform.profiles.61.patch | 19.84 KB | sun |
#61 | interdiff.txt | 1.78 KB | sun |
#44 | platform.profiles.44.patch | 19.57 KB | sun |
#44 | interdiff.txt | 2.71 KB | sun |
#42 | platform.profiles.42.patch | 18.57 KB | sun |
Comments
Comment #1
cburschkaSurprisingly simple. This patch replaces file_scan_directory() in install_find_profiles() with drupal_system_listing().
A slight modification was required to install_profile_info to remove the hardcoded ./profile/$name path. Specifically, I changed install_profile_info to take the whole file object (name, uri and all) instead of just the name, and used dirname($profile->uri) to get the proper directory.
I think this will also fix an undiscovered bug when a profile is deeper in the ./profile hierarchy (for example, if there is a "profiles/lovelycms/lovelycms_minimal/, profiles/lovelycms/lovelycms_full/" structure), as file_scan_directory() will already recurse, but install_profile_info() will assume a direct sub-directory of profiles/.
This is a preliminary patch that I have only tested as far as the select-profile screen. More fixes might be required to actually install it properly.
Comment #3
cburschkaHad to extend several other places that hardcode the path into using the object properties instead. drupal_check_profile() needs a parameter change as well.
The tests will probably fail to pass again, as I haven't updated them yet. Just trying to get the site to install for now.
Comment #4
lrvv CreditAttribution: lrvv commentedThe last submitted patch failed testing.
Comment #5
cburschka... what?
-
Anyway, yes it did, but only by manual testing, not the unit test. I had to modify _system_get_module_data() to use drupal_get_filename() for the profile path instead of hard-coding it. Here's the new patch.
Glad that at least the existing test case doesn't break anymore.
Comment #6
cburschkaAh. SimpleTest depends on install_profile_info() working the other way. That's why the unit test breaks so strangely.
It's relatively simple to allow both, which this patch does.
Comment #7
mgiffordI applied the patch and it worked fine. I also set up a new install with this patch. That didn't run into any problems either.
I didn't actually put another profile in the site to test that piece.
Comment #8
cburschkaOther than whether there are any hidden problems in other parts of the system not yet test-covered, one of the main questions for this patch is now the design (ie. does it do things the way they should be done).
What I'm particularly worried about is that install_profile_info() accepts either a file object of the .profile, or a base name of the profile. In the first case, it uses the file object's URI to locate it, in the second case it calls drupal_get_path().
The justification for not doing it the first way globally is that SimpleTest and drupal_get_profile() work with bare profile names, and reworking them would drastically change the scope. As to the second option, the installer already works with file objects in $install_state['profiles'] which have all the information. Passing one part (the name) of the information to install_profile_info(), only for it to have to scour the system files for another part of the information (the path) doesn't feel right.
Tagging this "review swap" - anyone who takes a good look at this patch (anything from design, rigorous testing, code style) gets a patch review from me in return. :)
Comment #9
mgiffordI've applied this patch again and tested it a bit further. I set up 2 different mock install profiles. Placed one in /profiles and the other in /sites/all/profiles.
Both showed up properly. I got this error during the install:
I hate these new D7 errors. However, in this case I'm quite certain it's because the install profile I constructed was faulty. I could have just replicated all of the functionality, but decided to add/remove modules.
Comment #10
cburschkaAre you working from a fully up-to-date HEAD? This error was occurring earlier today on clean installs due to a missing file....
Comment #11
mgiffordI just updated the CVS. I also copied the expert profile into /sites/all/profile & just renamed everything to expert2.
After that the install went off seamlessly!
What else would this need to be committed? Bringing profiles up to the level of themes/modules makes sense to me.
Comment #12
adrian CreditAttribution: adrian commenteda site can't have multiple install profiles, so it doesn't make sense to search for profiles in the site directory.
Comment #13
adrian CreditAttribution: adrian commentedplus then you have sites/$site/profiles/$profilename/modules
it just doesn't seem worth the extra complication, all the additional nesting.
Also, keep in mind that we're working on packaging scripts to create the entire profile and all dependencies as a single download.
profiles are basically applications built on the drupal framework,
and sites are instances of those applications.
Comment #14
cburschkasites/all is not an instance.
Comment #15
adrian CreditAttribution: adrian commentedprofiles/$profile is not an instance either.
Comment #16
jmiccolis CreditAttribution: jmiccolis commented@Arancaytar, having /profiles in parallel to /sites is by design. Both the /sites and /profiles folders allow you to make themes and modules available to a site under certain conditions. For example /sites/foo.example.com/modules exposes module to a site that is accessible at foo.example.com, whereas a /profiles/$profile/module exposes a module to a site if it was *installed* using that profile.
Stacking the sites and profiles concepts in such a way that all profile must live in /sites/all/profiles doesn't get us anything flexibility wise, and makes it less clear when a module will be available to a particular site.
I'm with Adrian and I'd vote that we don't change how this works.
Comment #18
Dave ReidSubscribing, I still think this should be valid. If I have a site-specific profile I'd like to have it included in sites/mysite.com/profiles so it could be properly put in a revision system per-site.
Comment #19
David_Rothstein CreditAttribution: David_Rothstein commentedI agree; this seems to make sense - sites/all/profiles would be for profiles that you intend to make available for any site to install with, whereas sites/example.com/profiles would be for profiles that only that particular site is allowed to choose from. This would bring consistency with modules and themes, plus help with upgrade confusion (i.e., the general mantra that you can safely upgrade core as long as you keep your sites directory intact would actually be closer to the truth). I don't think I see the downside.
Not sure if this should be for Drupal 7 at this point though. And it does seem like it would require changes to the d.o. profile packaging scripts.
Comment #20
RobLoachAdding the Platform Initiative tag since different platforms could essentially be installed and maintained via different install profiles. Also, this probably needs a nicer Issue Summary.
Comment #21
scor CreditAttribution: scor commentedGood stuff Rob! Could you use git diff -C -M so the patch is easier to review?
Comment #22
RobLoachUnfortunately it wasn't working with git diff because we're moving files. I used git format-patch instead.... If you just scroll down, you'll see it's pretty much the same format.
Comment #23
scor CreditAttribution: scor commentedtry
git format-patch -C -M origin/8.x --stdout > patch.patch
Comment #24
rfayYou have my support!
Comment #25
RobLoachAlso just thought it might be a good idea to add a sites/all/profiles/README.txt and core/profiles/README.txt.
Comment #26
scor CreditAttribution: scor commentedsame as #20 with the file move hunks hidden.
Comment #27
quicksketchI don't think either of these are necessary. If you're working on making an install profile, you're already in a <1% of Drupal developers. Any company or user that is providing a distribution should be able to figure out how profiles work. I personally would prefer to avoid adding a "sites/all/profiles" directory by default for the same reason (you're already working with <1% of sites that use custom profiles). And since you can't add an empty directory to Git version control, avoiding a sites/all/profiles/README.txt is the same thing as not including the directory by default.
Other than that comment. I'm not sure this passes the smell-test:
I'd prefer a way to easily load a "profile object" if we're going to introduce a new object. Unfortunately
install_load_profile()
is already in use. On the flip side, neither modules nor themes have "module objects" or "theme objects". As the goal of this patch is consistency with other Drupal er, "things" (modules/themes/theme engines), I'd like to reuse our existing patterns or consider if other things need this new pattern applied to them.Comment #28
sunComment #30
RobLoachSince distributions are shipped as install profiles, we need install profile discovery on a per-site basis. Therefore, I'm adding this to the distribution blocker queue.
Comment #31
boombatower CreditAttribution: boombatower commentedindeed.
Comment #32
sunI agree with this proposal, but would like to see the top-level
/profiles
to be re-purposed, so that it has the effect of the proposedsites/all/profiles
here (i.e., don't dosites/all/profiles
).IIRC, @quicksketch or someone else proposed the same for
/modules
(vs./sites/all/modules
), but I'm not able to find the issue that proposes it.In the end, we should not throw the multi-site feature into everyone's face. Let's have simple top-level /modules, /themes, /profiles directories instead (all of them as replacement for current /sites/all/ equivalents).
Comment #33
RobLoachLet's move profiles to core/profiles first, so that the scope of this issue is a bit smaller: #1661468: Move install profiles to core/profiles
Comment #34
sun#1661468: Move install profiles to core/profiles has been marked as duplicate, because the proposed strategy would have suboptimal consequences:
So let's move the patch from over there in here and perform the full change in this issue. Also unassigning @Dave Reid, since he wasn't involved lately anymore.
Comment #35
glennpratt CreditAttribution: glennpratt commented@sun Big +1 to #32, though it seems like that could be done all at once in that issue without changing this one.
Do you have a link to that issue or proposal now? I couldn't find it but would like to help.
Comment #36
RobLoach@glenn The current patch is attached. Support for sites/all/profiles and sites/example.com/profiles is missing from it, as outlined in #20.
Comment #37
RobLoachHmmmm.
Comment #38
RobLoachForgot to remove the standard2 testing profile.
Comment #40
sunJesus. That install system code must date back to Drupal 5 or even 4 times... Anyone with a reasonable grasp of current coding patterns in HEAD should be able to roll a monster patch to bring it on par with the rest of core... But of course, that's off-topic for this issue. ;)
Comment #42
sunacd7228 Fixed install profile URI is lost when rebuilding module info.
Comment #44
sund135a0f Fixed install_profile_info() / drupal_get_filename() fails to locate any profiles during installation.
Comment #45
RobLoachThis is a major improvement to Drupal as it means that people will no longer have to "hack core" to add new install profiles. Contributed Drupal distributions will finally be able to be packaged correctly with non-core install profiles in sites/all/profiles rather than mixed in with the core ones.
Tested by doing the following:
I'd consider this RTBC, but one question....
Do we have a follow up set up for this? When does this case happen?
Comment #46
sunI searched the queue, but wasn't able to find an existing issue for that. (Admittedly, I ran out of steam quickly, since the search keyword "profile" yields thousands of totally irrelevant results ;))
So I've created a new one: #1676196: Install profiles are registered as modules
Comment #47
RobLoachDone!
Comment #48
sunSpeaking of ambiguity, let's clarify this issue title... ;)
Comment #49
sun#44: platform.profiles.44.patch queued for re-testing.
Comment #50
RobLoach#44: platform.profiles.44.patch queued for re-testing.
Comment #51
sunComment #52
sunTagging.
Comment #53
bleen CreditAttribution: bleen commentedA related, possibly overlapping issue: #1617860: /profiles/all folder for distributions that include multiple install profiles
Comment #54
bleen CreditAttribution: bleen commentedSorry to come into this late in the game, but after reading this issue a bit more carefully now, I have a concern.
This issue addresses (correctly) the idea that more and more people want to create an install profile that can be downloaded from D.O. (or wherever) as a simple profile folder and dropped into the profiles folder (wherever it lives). As it was noted in the original post regarding the current state of the profiles folder:
Unless I'm missing something, the solution proposed in this issue can make it harder for organizations like mine that are developing one distribution of Drupal with multiple custom install profiles. I dont want to maintain "views" in 3 different folders in sites/all/profiles.
The way the folders are currently structured, there is a simple solution to our problem (add profiles/all - see #1617860: /profiles/all folder for distributions that include multiple install profiles) but if the proposal in this issue is committed than there is no longer an easy solution for those people who DO distribute drupal as a third-party package with multiple profiles.
Thoughts?
Comment #55
RobLoachInstructing users to mix their own install profiles with Drupal core's install profiles is, more or less, instructing them to "hack core". This is why we have sites/all/modules instead of asking them to place their modules in core/modules. Having sites/*/profiles stays consistent with the community standard to use sites/*/[modules,themes,etc].
We could still set up sites/*/profiles/all no problem, but that should happen over at #1617860: /profiles/all folder for distributions that include multiple install profiles.
Comment #56
boombatower CreditAttribution: boombatower commentedI don't see how this changes anything with regard to your issue of sharing modules between profiles. The current solution provides not better way to handle this, you simply place views in sites/all/modules. Putting your modules in profiles/[name]/modules will not be a requirement to use profiles. Instead, it will simply add a new place to look and allow 3rd party profiles to package everything together.
That said the whole concept of sharing modules between profiles online work when you have complete control over all the profiles....otherwise you cannot count on a specific version (which you may need to and should do in quality profiles).
Comment #57
bleen CreditAttribution: bleen commentedThis is precisely the distinction we use to decide which modules to put in a profile vs. which to put in sites/all ... if we need to control the version it should be in profiles. So in the case where we have a distro with multiple install profiles and they all rely on views-7.x-3.1 where should we put views? Sites/all is not quite right in that case ...
Anyway I fear I'm getting off topic and I dont want to side track this issue any more than I have .... I just want to make sure it wont get any harder to solve the issue we have been having with profiles sharing modules were this patch to be committed.
Comment #58
sun@bleen18: In fact, this issue + patch here is a fundamentally required cornerstone to make #1617860: /profiles/all folder for distributions that include multiple install profiles possible in the first place. The suggested change over there is incomplete; it doesn't account for the all the plumbing that is additionally required to make the installer and install profile discovery aware of the additional profile folders. That's what this patch here is doing. :)
Comment #59
RobLoach#44: platform.profiles.44.patch queued for re-testing.
Comment #60
catchIs install_find_profiles() really run more than once during the same request? I didn't see any discussion of this in the issue.
The order of modules, themes and profiles is inconsistent. Also there is "module and themes" twice without the associated "profiles" - we should add that consistently.
Comment #61
sund783e7d Fixed install_find_profiles() is only called once and can be removed.
691916f Fixed inconsistent order of modules/themes/profiles.
Comment #62
catchLooks good.
Comment #63
catchStill looks good and unblocks some other issues. Committed/pushed to 8.x.
Comment #64
sunChange notice: http://drupal.org/node/1724216