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).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cburschka’s picture

Status: Active » Needs review
FileSize
2.58 KB

Surprisingly 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.

Status: Needs review » Needs work

The last submitted patch failed testing.

cburschka’s picture

Status: Needs work » Needs review
FileSize
5.11 KB

Had 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.

lrvv’s picture

The last submitted patch failed testing.

cburschka’s picture

... 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.

cburschka’s picture

Ah. 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.

mgifford’s picture

I 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.

cburschka’s picture

Other 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. :)

mgifford’s picture

FileSize
83.49 KB

I'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:

An error occurred.
Path: http://localhost:8888/drupal-cvs/install.php?profile=not_default&locale=en&id=1&op=do
Message:
"http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">

SQLSTATE[HY000]: General error: 1366 Incorrect integer value: '' for column 'filectime' at row 1 

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.

cburschka’s picture

Are you working from a fully up-to-date HEAD? This error was occurring earlier today on clean installs due to a missing file....

mgifford’s picture

I 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.

adrian’s picture

a site can't have multiple install profiles, so it doesn't make sense to search for profiles in the site directory.

adrian’s picture

plus 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.

cburschka’s picture

sites/all is not an instance.

adrian’s picture

profiles/$profile is not an instance either.

jmiccolis’s picture

@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.

Status: Needs review » Needs work

The last submitted patch failed testing.

Dave Reid’s picture

Assigned: Unassigned » Dave Reid

Subscribing, 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.

David_Rothstein’s picture

I 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.

RobLoach’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
Issue tags: +Needs issue summary update, +Platform Initiative
FileSize
53.27 KB
Move /profiles to /core/profiles
This reflects the structure introduced in #22336: Move all core Drupal files under a /core folder to improve usability and upgrades, and means that people will not mess around with Drupal core when they want to add additional install profiles.
Allow use of sites/all/profiles
This gives users the ability to add more install profiles for any domain they want.
Allow use of sites/example.com/profiles
This gives the user the ability to expose install profiles to specific domains they want.

Adding 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.

scor’s picture

Status: Needs review » Needs work

Good stuff Rob! Could you use git diff -C -M so the patch is easier to review?

RobLoach’s picture

Status: Needs work » Needs review

Good stuff Rob! Could you use git diff -C -M so the patch is easier to review?

Unfortunately 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.

scor’s picture

try git format-patch -C -M origin/8.x --stdout > patch.patch

rfay’s picture

You have my support!

RobLoach’s picture

Status: Needs review » Needs work

Also just thought it might be a good idea to add a sites/all/profiles/README.txt and core/profiles/README.txt.

scor’s picture

FileSize
14.19 KB

same as #20 with the file move hunks hidden.

quicksketch’s picture

Also just thought it might be a good idea to add a sites/all/profiles/README.txt and core/profiles/README.txt.

I 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:

  * @param $profile
- *   Name of profile.
+ *   Either the profile name, or the profile object itself.

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.

sun’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 562042_20_C_M.patch, failed testing.

RobLoach’s picture

Since 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.

boombatower’s picture

indeed.

sun’s picture

I 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 proposed sites/all/profiles here (i.e., don't do sites/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).

RobLoach’s picture

Status: Needs work » Postponed

Let'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

sun’s picture

Assigned: Dave Reid » Unassigned
Status: Postponed » Needs work

#1661468: Move install profiles to core/profiles has been marked as duplicate, because the proposed strategy would have suboptimal consequences:

[...] if we do it this way, then aforementioned issue has to be bumped to a critical task and release blocker. I guess I'd rather prefer to do both in one to prevent that critical release blocker scenario.

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.

glennpratt’s picture

@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.

RobLoach’s picture

FileSize
12.78 KB

@glenn The current patch is attached. Support for sites/all/profiles and sites/example.com/profiles is missing from it, as outlined in #20.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
17.89 KB

Hmmmm.

RobLoach’s picture

FileSize
16.18 KB

Forgot to remove the standard2 testing profile.

Status: Needs review » Needs work

The last submitted patch, 562042.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
Issue tags: -Needs design review, -Review swap
FileSize
8.8 KB
18.13 KB

Jesus. 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. ;)

Status: Needs review » Needs work

The last submitted patch, platform.profiles.40.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
2.11 KB
18.57 KB

acd7228 Fixed install profile URI is lost when rebuilding module info.

Status: Needs review » Needs work

The last submitted patch, platform.profiles.42.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
2.71 KB
19.57 KB

d135a0f Fixed install_profile_info() / drupal_get_filename() fails to locate any profiles during installation.

RobLoach’s picture

This 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:

  1. Copied core/profiles to sites/all/profiles/standardtwo
  2. Replaced all instances of "standard" with "standardtwo" in the new standardtwo profile
  3. Visited core/install.php and saw Standard Two in the profile listing
  4. Installed the new site with the standard two profile

I'd consider this RTBC, but one question....

+++ b/core/includes/bootstrap.incundefined
@@ -888,6 +889,12 @@ function drupal_get_filename($type, $name, $filename = NULL) {
       }
+      // Profiles are converted into modules in system_rebuild_module_data().
+      // @todo Remove false-exposure of profiles as modules.
+      elseif ($original_type == 'profile') {
+        $dir = 'profiles';

Do we have a follow up set up for this? When does this case happen?

sun’s picture

I 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

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

Done!

sun’s picture

Title: Search for profiles in site folders » Search for install profiles in sites/[all|site]/profiles folders, and move core profiles into /core/profiles

Speaking of ambiguity, let's clarify this issue title... ;)

sun’s picture

RobLoach’s picture

sun’s picture

Assigned: Unassigned » sun
sun’s picture

Issue tags: +API change, +API clean-up

Tagging.

bleen’s picture

bleen’s picture

Status: Reviewed & tested by the community » Needs work

Sorry 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:

This may work (more or less) when the profile and code-base are distributed together as a third-party package

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?

RobLoach’s picture

Status: Needs work » Reviewed & tested by the community

Instructing 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.

boombatower’s picture

I 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).

bleen’s picture

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).

This 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.

sun’s picture

@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. :)

RobLoach’s picture

#44: platform.profiles.44.patch queued for re-testing.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/install.core.incundefined
@@ -1098,7 +1100,12 @@ function install_settings_form_submit($form, &$form_state) {
 function install_find_profiles() {
-  return file_scan_directory('./profiles', '/\.profile$/', array('key' => 'name'));
+  $profiles = &drupal_static(__FUNCTION__);
+
+  if (!isset($profiles)) {
+    $profiles = drupal_system_listing('/^' . DRUPAL_PHP_FUNCTION_PATTERN . '\.profile$/', 'profiles');
+  }
+  return $profiles;

Is install_find_profiles() really run more than once during the same request? I didn't see any discussion of this in the issue.

+++ b/sites/all/README.txtundefined
@@ -1,7 +1,7 @@
+files. Place contributed and custom modules and themes in the sites/all/modules,

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.

sun’s picture

Status: Needs work » Needs review
FileSize
1.78 KB
19.84 KB

d783e7d Fixed install_find_profiles() is only called once and can be removed.
691916f Fixed inconsistent order of modules/themes/profiles.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

catch’s picture

Title: Search for install profiles in sites/[all|site]/profiles folders, and move core profiles into /core/profiles » Change notification Search for install profiles in sites/[all|site]/profiles folders, and move core profiles into /core/profiles
Category: feature » task
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

Still looks good and unblocks some other issues. Committed/pushed to 8.x.

sun’s picture

Title: Change notification Search for install profiles in sites/[all|site]/profiles folders, and move core profiles into /core/profiles » Search for install profiles in sites/[all|site]/profiles folders, and move core profiles into /core/profiles
Category: task » feature
Priority: Critical » Normal
Status: Active » Fixed

Automatically closed -- issue fixed for 2 weeks with no activity.