Problem/Motivation

Drupal distributions can't hide other core install profiles and/or skip the profile selection page on install.
For instance this core patch was needed in Drupal 7 for Commerce Kickstart to skip the profile select screen which would give the user the choice to install Drupal Core without Kickstart.
Many distributions will want the user to always install their install profile.

Proposed resolution

Provide a new "exclusive" property for install profile .info files. If this property is set to TRUE in 1 and only 1 profile then only that profile would be available(skipping the profile selection screen). If 2 profiles have this set then the property would be ignored.

Remaining tasks

I am not sure if tests can be written for install tasks. Docs would need to be written to let distribution creators know how this property works.

User interface changes

If there is an exclusive profile then the user would not see the profile selection screen.

API changes

_install_select_profile would be the only function that would need to be changed.
Example info would start of like this:

name = Exclusive Profile
description = Install a Distro with choice to install other profiles
core = 8.x
exclusive = TRUE

Original report by amontero

// Text of original report here.
(for legacy issues whose initial post was not the issue summary)
Useful if a distribution wants to hide Drupal's core profiles at install time as in Kickstart:
#1630110: [Meta] Core patches specific to Kickstart

If there is any profile defining them, either default to it in the install profile screen or plain hide it as Kickstart does. Seems a feauture many other distributions may benefit from.
In case >1 profile defines any of them, just ignore them gracefully.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amontero’s picture

tedbow’s picture

Status: Active » Needs review
FileSize
907 bytes

I have attached a patch that handles that "exclusive" property in install profiles .info file.
It will only work if 1 and only 1 profile is marked as exclusive.
Otherwise it will ignore the property.

I have manually tested it with making the standard profile exclusive.
I also test with both the standard and minimal profile being exclusive to make sure it was ignored.

I have created an Exclusive Distribution sandbox project.
I plan on putting a test install profile there that will use this property.

amontero’s picture

Status: Needs review » Reviewed & tested by the community

Tested exclusive flag, both when only 1 profile is marked as such and when two of them are.
Also tested flagging both standard and minimal as "exclusive = 0", just in case.
Works as described. Ran installation thru the end without a hitch.
Complies with coding standards.

To commiters: this patch seems a backport eligible one, isn't it?

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs backport to D7

This looks like a backportable feature to me also.

I think the patch looks good, but the new property should be documented in the PHPDoc of install_profile_info() (along with the other properties that are already documented there). And maybe for consistency it should be added to the $defaults array inside that function (to guarantee that it will always be defined)? Not sure about that last one.

I think the code comments also need a bit of work, e.g.:

+          // Found more that 1 exclusive profile ignore property

This should use "one" rather than "1" and should be a complete sentence with a period at the end. Something more like:

'We found more than one profile with an "exclusive" property. There's no way to choose between them, so we ignore the property altogether.'

webchick’s picture

Status: Needs work » Needs review
FileSize
1.82 KB

So I sat down to roll the patch with David's recommended changes, and it occurred to me that "exclusive" is kind of a weird name for this flag. What it really means is "As long as there is only one of these, it will auto-select this option in the installer so you don't get prompted for an install profile choice." Therefore, something like "default" or "autoselect" (1 vote for each on IRC, which doesn't help :P) seems to make more sense to me. And while I don't want to derail a really awesome feature on naming bikeshedding, if we want to backport it to D7 (and I think we do) then we actually do need to get the property name right on the first try. :\

Then millette brought up that really, what you want is the general behaviour that if there is only one non-core profile, it should just be the default, by default (heh). That would be the Commerce Kickstart/Spark/OpenPublish/etc. case. If there are multiple non-core profiles to choose from, then none of them should be default, and menu of options should be shown, as it is now. That's the Using Drupal use case. Obviously, we couldn't backport an "opt-out" mechanism, though; it has to be "opt-in" for D7.

I think what I'd recommend is finishing off this patch in an "opt-in", backportable state (which means deciding on a name), and then opening a follow-up to discuss whether we want to change that default behaviour for D8.

Question about the logic. Would it make sense to just bail after finding the first exclusive/default/autoselect profile, rather than looping through all of the options and killing the exclusiveness if we find more than one? The latter kind of feels like babysitting broken code to me.

At any rate, here's a re-roll with David's suggestions. And FWIW, "default" is what I keep accidentally saying without thinking about it, so that would be my vote, although it could be misconstrued as a property that simply sets the default choice in the form to profile A.

Cameron Tod’s picture

Status: Needs review » Needs work

Love the concept, but I'm going to bikeshed "default" over "exclusive" for the property name.

webchick’s picture

Status: Needs work » Needs review

Cross-post.

tedbow’s picture

Therefore, something like "default" or "autoselect" (1 vote for each on IRC, which doesn't help :P) seems to make more sense to me. And while I don't want to derail a really awesome feature on naming bikeshedding, if we want to backport it to D7 (and I think we do) then we actually do need to get the property name right on the first try. :\

I don't like "default" because it doesn't convey the concept that other profiles will be excluded. It sounds to me like it will be the default choice in the profile select screen. I think the same is true for "autoselect". Also I think there could be another property called "default" that would just make it the default value of the radio group on the profile select screen. I don't think we should add the other "default" property now but I don't think we should use the word incase wanted to implement that feature at some point in the future.

I am not married to "exclusive" just don't think the other options are better.

tedbow’s picture

Question about the logic. Would it make sense to just bail after finding the first exclusive/default/autoselect profile, rather than looping through all of the options and killing the exclusiveness if we find more than one? The latter kind of feels like babysitting broken code to me.

I think it some cases this would not be broken code. Especially if we think of the property as really acting like exlusive_if_only_exclusive.

Here is how I see it:

If a distribution like Commerce Kickstart or OpenPublish has 2 profiles marked exclusive I would see that as broken code because it would have no effect.

On the other hand if at a web development shop they have a version of Drupal that they use to start that have both Commerce Kickstart and OpenPublish(that they don't want to have to patch) as available install profiles the user should be given a choice of which one they want to use.

So the distributions correctly uses the exclusive property. It allows them be exclusive when used as part of the distribution but gives the user the choice when they are in other version of Drupal with multiple exclusive profiles.

webchick’s picture

Well those are all definitely good points. I couldn't come up with any obviously better names that don't also have a connotation of being default-selected on the form ("force_select"? :P "choo_choo_chooose me"? ;)). I'll do some more thinkering on it, but if we don't have anything obviously better in a few days, I'd say it's probably fine to go with it. It's at least documented now. ;)

webchick’s picture

In fact, maybe we keep it exclusive, and just update the docs to more or less what tedbow said in #9? That makes the name/intention of the property really clear IMO.

amontero’s picture

+1 for keeping "exclusive" as tedbow reasoned in #9 and #10. He made good points about conflicting exclusive flags not necessarily being broken code.
"default" would be better left out for another feature like "present profile screen with profile selected" as suggested, also.

webchick’s picture

Status: Needs review » Needs work

Ok, then this patch needs work for comments. tedbow, would you be able to take a stab at it?

tedbow’s picture

Assigned: Unassigned » tedbow

@webchick, yep can do. About to go out. I will get to it tonight or tomorrow morning(EST)

tedbow’s picture

Status: Needs work » Needs review
FileSize
2.8 KB

Ok I have attached a patch with added documentation for this function and install_profile_info

I also noticed that is no documentation for the "hidden" property in install_profile_info. Should that be documented? Separate issue?

I know that it is used for the testing profile but would potentially be used by other profiles? Maybe not

Cameron Tod’s picture

Status: Needs review » Needs work

Thanks tedbow. I noticed a few small style errors and typos that would be great to cleanup before we get this in. :)

+++ b/core/includes/install.core.incundefined
@@ -1164,7 +1164,16 @@ function install_select_profile(&$install_state) {
 /**
- * Selects an installation profile from a list or from a $_POST submission.
+ * Selects an installation profile.
+ * A profile will be selected if there is only one available profile

The first line in a function docblock needs a blank line after it.

+++ b/core/includes/install.core.incundefined
@@ -1164,7 +1164,16 @@ function install_select_profile(&$install_state) {
+ * If more that one profile is set to "exclusive" then no profile will be selected.
+ * @param array $profiles
+ *   An associative array of profiles where the keys equal the machine-readable
+ *   names of the profiles.
+ * @return
+ *   Either the machine-readable name of the selected profile or no value returned.

A few issues:

- Typo ("more that" instead of "more than")
- Last line of function comment is >80 characters.
- There needs to be a line break before the first @param, and before the @return.
- Could you reflow or rework the parameter and return comments to under 80 chars?

+++ b/core/includes/install.core.incundefined
@@ -1175,6 +1184,26 @@ function _install_select_profile($profiles) {
+          // There's no way to choose between them, so we ignore the
+          // the property.

Two "the"s, and can we get "property" on the same line?

+++ b/core/includes/install.incundefined
@@ -887,6 +887,12 @@ function drupal_check_module($module) {
+ *   this property will have no effect and the profile selection screen will ¶

Trailing whitespace here.

tedbow’s picture

Ok thanks for the feedback. I ran coder this time too which I should have done last time. Here is the updated patch.

tedbow’s picture

Status: Needs work » Needs review

Changing to needs review

Cameron Tod’s picture

Status: Needs review » Needs work

Looks great! Just one small thing...

+++ b/core/includes/install.core.incundefined
@@ -1164,7 +1164,21 @@ function install_select_profile(&$install_state) {
+ * A profile will be selected if there is only one available profile
+ * or if a profile was provided in the $_POST submission.
+ * or if one of the profiles has the exclusive property set to TRUE.
+ * If more than one profile is set to "exclusive"

Looks like you used a period here instead of a comma ("submission. or")

Maybe we could block out the cases when a profile is selected to a list?

Something like

A profile will be selected if:
  - blah;
  - foo;
  - bar.
tedbow’s picture

Status: Needs work » Needs review
FileSize
2.81 KB

Changes made.

amontero’s picture

Patch applies OK. Tested exclusive flag, both when only 1 profile is marked as such and when two of them are.
Everything works as described.
If docs are OK (they seem to me) it's RTBC.

tedbow’s picture

Just checking back on this. Is it RTBC? Seems to work and I think docs are okay.

I would love to get this in.

btopro’s picture

Love this idea, curious -- is there documentation anywhere more or less telling people that they shouldn't run a single multisite using multiple distributions? I know that they CAN work together but generally speaking it's not the easiest / most maintainable thing. The introduction of the exclusive flag would more or less imply that you will only be running one distro but might be confusing if someone downloads kickstart, then openpublic and mashes them together only to find that they always get kickstart.

This would then see every distribution developer saying "mine is the most important" and adding the exclusive flag as not to confuse their use-base that merges things into a single Drupal stack. Wouldn't it be better to add a flag that implies a distribution? such as distribution = 1.

In this use-case if 1 distribution is noticed it will select it, if multiple it'll present the screen to display all. Either the property or the logic of defaulting to a single option should be worked on imo since I stack many "distros" for quick starting points and would hate to have to stumble upon this magic issue queue to realize why it wasn't letting me use other ones in the event that I grabbed kickstarter.

tedbow’s picture

@btopro

might be confusing if someone downloads kickstart, then openpublic and mashes them together only to find that they always get kickstart.

This won't happen because the exclusive flag is ignored if more than 1 install profile has it.

For how it works look at #2

Further explanation about how it would work with 2 distros see #9

In this use-case if 1 distribution is noticed it will select it, if multiple it'll present the screen to display all.

This how it works now.

Wouldn't it be better to add a flag that implies a distribution? such as distribution = 1.

We had discussions above about the best name for the flag and decided on "exclusive" b/c of reasons mentioned.

I don't think "distribution" is a good b/c

  1. It maybe that not all distributions would want this behavior. If the flag was named this then it seems logic to a developer that a distribution would need this flag.
  2. An install profile is not the same as a distribution. I could see cases where a development shop would want to have a install profile that skips the other install profile but this would not really be a distribution. Or they may just patch an existing core profile to add this flag.
btopro’s picture

So the distributions correctly uses the exclusive property. It allows them be exclusive when used as part of the distribution but gives the user the choice when they are in other version of Drupal with multiple exclusive profiles.

No I get what your saying, I'm saying that puts the burden on all distribution developers to add this property. You add the exclusive flag to kickstart, I don't add it to MOOC. Someone puts both in the same batch it ignores mine because I forgot/didn't know about that flag. This could create a support nightmare when we get into how code/drupal-adverse some of those installing distros potentially are because it's a near undiagnosable error.

I like the idea, I just think maybe the logic of detecting when to go the exclusive route should be "this is the only non-core profile" instead of "this is the only profile to use exclusive". This way there's no penalty for using the exclusive flag. It would still meet your original use-case of someone downloads kickstart and they skip that screen but account for someone manually taking kickstart and sticking it in a stack with other install profiles. The developer / user knows why they are structuring the project in this way (having 2 distros in one rig) and so the exclusive flag shouldn't matter imo.

Another solution is that the d.o. packaging script appends the exclusive flag to all distributions built by d.o.; This would solve both your problem and the potential one I'm pointing out.

tedbow’s picture

I just think maybe the logic of detecting when to go the exclusive route should be "this is the only non-core profile" instead of "this is the only profile to use exclusive".

Another solution is that the d.o. packaging script appends the exclusive flag to all distributions built by d.o.;

Both of these case assumes that ALL distros would want to have this behavior. I don't think that is the case.

You add the exclusive flag to kickstart, I don't add it to MOOC. Someone puts both in the same batch it ignores mine because I forgot/didn't know about that flag. This could create a support nightmare when we get into how code/drupal-adverse some of those installing distros potentially are because it's a near undiagnosable error.

If you are putting 2 distros together and/or making your own distro I don't think it is big burden to know about this flag. Obviously if this patch gets in we could add this to the docs about how to make a distro./install profile.

it's a near undiagnosable error.

I don't see how this would be undiagnosable

btopro’s picture

if downloading a distro from d.o. I think it's a pretty safe assumption that you want it exclusively; otherwise you would have checked out the repo / make file and self assembled. I like the idea this just seems like a another nuance in Drupal setups / internal logic along the lines of module weights where some projects need to be weighted relative to others and outside of code diagnosis / hunting through docs there's nothing obvious.

Even a status message saying something like "Installing Kickstarter (click here to select another option)". I do a lot of UX work so I never like making assumptions about what the end-user wants that would require dipping into code to alter.

tedbow’s picture

For others following this thread I had an offline conversation with @btopro I think he is ok with way the patch works now but I will let speak for himself.

Another solution is that the d.o. packaging script appends the exclusive flag to all distributions built by d.o.; This would solve both your problem and the potential one I'm pointing out.

The patch with allow the this to happen..but would be an issue for the infrastructure team(so I think separate but dependent on this)

btopro’s picture

We are in agreement, once this is committed (which I think it's ready) another issue could be opened up w/ the infrastructure team about setting this property in d.o. packaged distros. This would utilize this UX improvement for developers outside of d.o. as well as users of distros that want as stream-lined as possible 1-click solution to a problem.

fubhy’s picture

FileSize
2.71 KB

Just did a micro-change on the documentation. All credit goes to the provider of the patch of course. I just thought it wasn't worth a comment and did the re-roll myself quickly. Patch is looking good. "exclusive" also makes sense imo. So if noone objects this should be RTBC.

If you agree with the documentation changes feel free to set it to RTBC @tedbow.

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

@fubhy, looked at the differences and they look good to me. Thanks.

webchick’s picture

Title: Add "default" and/or "exclusive" flags to .info file in install profiles (improve distribution creation) » Add 'exclusive' flag to install profiles to auto-select them during installation.
Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

I see what btopro's saying, and indeed if distro A knew about this flag and added it, and distro B didn't, and then distro C pulled in distros A and B as "sub" profiles, you could get into a sticky situation if you actually wanted to install distro B through the UI because, effectively, you couldn't. You in fact won't even know that distro B was ever part of distro C to begin with without trawling through the source code.

That seems a bit of an edge-case, however, given that (at least on d.o) we don't support the idea of "sub" distributions. And while it would be fairly trivial to add this "auto-exclusive" logic to the d.o packaging system, we'd need to be careful to only do so in distributions that contain only a single profile. For Using Drupal, for example, we ship with a profile per chapter and none of them would be "exclusive." I believe the correct queue to file that in is http://drupal.org/project/issues/project (project release module), at least as a start.

I really like this patch, because it is a one-line API addition over a core hack (actually, two of them; one to fix #1074108: Profile selection form not skipped if there is only one visible profile and another to hack minimal/standard to hidden) that distros currently have to do to achieve the same thing.

Committed and pushed to 8.x! Thanks!

Moving down to 7.x.

tedbow’s picture

I see what btopro's saying, and indeed if distro A knew about this flag and added it, and distro B didn't, and then distro C pulled in distros A and B as "sub" profiles, you could get into a sticky situation if you actually wanted to install distro B through the UI because, effectively, you couldn't. You in fact won't even know that distro B was ever part of distro C to begin with without trawling through the source code.

Thats a good way of explanation of the problem but yes, seems like an edge case. I guess all the more reason to make sure this flag is well documented somewhere else besides the source code.

Thanks @webchick for commtting and amontero, David_Rothstein, cam8001,btopro, and fubhy for working on this!

tedbow’s picture

Ok, I am starting on the D7 port.

tedbow’s picture

Ok, that was easy. The install function was the same.

In addition to the patch I have attached 2 other patches for testing.

1 that makes the standard profile exclusive - It should be automatically selected.
1 that makes both the standard and minimal profiles exclusive - Since 2 are marked as exclusive you should have to choose.

tedbow’s picture

BTW just read this about setting a patch to do-not-test. Which is what I would have done for the other 2 patches if I had known. oh well

amontero’s picture

Status: Needs review » Reviewed & tested by the community

Patch applies OK. Tested exclusive flag, both when only 1 profile is marked as such and when two of them are.
Everything works as described.

David_Rothstein’s picture

Title: Add 'exclusive' flag to install profiles to auto-select them during installation. » Change notification for: Add 'exclusive' flag to install profiles to auto-select them during installation.
Category: feature » task
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/3ed0414

(Not sure if I should have committed it since this is a feature request and we are currently over thresholds, but it's been sitting here for a long while, and in any case, for very small Drupal 7 features I no longer see the point of caring about the thresholds anyway.)

I also think we could use a change notification here; especially in light of the issue @btopro brought up, it seems like it would be a really good idea to have some documentation that explains to install profile authors when they should (and, more important, shouldn't) use this new parameter.

I don't think it's quite as critical as most change notifications though (this is not an API change in any way, just a new feature) so I'm not changing the issue priority to "critical" as would normally be done for that.

David_Rothstein’s picture

Drupal 7.18 was a security release only, so this issue is now scheduled for Drupal 7.19 instead.

Fixing tags accordingly.

webchick’s picture

I don't object to this patch going in, but in general, we hold D8 features up for D7 bugs, so it seems like we would do the same for D7 features. But it's possible this was RTBCed while we were under thresholds at some point (though it's been awhile since that was the case, unfortunately... :\).

David_Rothstein’s picture

Short answer: I think this issue most likely was RTBC at some point while we were under thresholds - though the period during which we were under thresholds was possibly more like hours rather than days :)

Longer answer: #1810428-76: [Policy, no patch] Adjust major and critical task thresholds during code thaw

David_Rothstein’s picture

Drupal 7.19 was a security release only, so this issue is now scheduled for Drupal 7.20 instead.

Fixing tags accordingly.

David_Rothstein’s picture

Drupal 7.20 was a security release only, so this issue is now scheduled for Drupal 7.21 instead. For real this time... I think :)

Fixing tags accordingly.

David_Rothstein’s picture

Issue tags: +7.22 release notes

Fixing tags since Drupal 7.21 only contained a fix to deal with fallout from the Drupal 7.20 security release. Hopefully this is really the last time I do this and this will be released in Drupal 7.22 for real :)

Gábor Hojtsy’s picture

Note that #1351352: Distribution installation profiles are no longer able to override the early installer screens proposes a more elaborate solution for Drupal 8, including the ability to skip language selection for foreign language (or single language) profiles. However people there were far from happy to see any of this end up in the profile info file itself. Ironic.

Gábor Hojtsy’s picture

Title: Change notification for: Add 'exclusive' flag to install profiles to auto-select them during installation. » Add 'exclusive' flag to install profiles to auto-select them during installation.
Category: task » feature
Status: Active » Fixed
Issue tags: -Needs change record

Change notice added at http://drupal.org/node/1961012. I don't quite understand how/what to document for the issue @btopro brought up, so please extend the docs as appropriate.

btopro’s picture

I reviewed the change notice, makes sense as is. My concern was merely communicating that this was possible to people. Thanks for pushing this out!

theohawse’s picture

Status: Fixed » Closed (fixed)

The patch looks like its been committed to latest 7.22 core, works for me, closing this issue finally after 8 months.

sun’s picture

I wasn't aware of this issue.

This change makes no sense. You've essentially enabled a taxonomy term to declare that it's the only term in a vocabulary. Two or more terms declaring the same? Who wins? And why?

The proper answer for this scenario is #1351352: Distribution installation profiles are no longer able to override the early installer screens

Not re-opening, since 7.22 is out already, so the ship has sailed.

Aforementioned issue needs to additionally revert this change for D8 (only).

David_Rothstein’s picture

Who wins? And why?

As described in the change notification, neither wins. The user would be presented with the normal install profile selection screen in that case.

To me that makes sense - the idea is that for any distribution that adds this, if you go to drupal.org and download the distribution tarball it will install using that distribution automatically. Whereas if you build up a more complicated codebase with a few different install profile choices, the person installing Drupal will still be asked to pick one.

As discussed above, there are some edge cases where that doesn't necessarily work well, and either #1351352: Distribution installation profiles are no longer able to override the early installer screens or forcing the drupal.org packaging system to add this property to the distro tarballs might be a solution for that. But I don't see why it's a problem as an interim solution?

David_Rothstein’s picture

Status: Closed (fixed) » Fixed

I made a few edits to the change notification to clarify it and to specifically encourage Drupal distributions to use the new property (since that was the goal mentioned above): http://drupal.org/node/1961012/revisions/view/2640890/2648362

I'll also go back and link to this in the Drupal 7.22 release notes and CHANGELOG.txt.

Let's also leave this issue as "fixed" (rather than moving it directly to "closed") so it stays visible in the issue queue while discussion is ongoing... It will automatically close itself when the discussion is finished.

btopro’s picture

not sure how these two issues couldn't coexist, this is for priority if none else are selected, the rest select options within. very simple api modification worked through by a large group to increase usability via single line.

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

Anonymous’s picture

Issue summary: View changes

wrote fuller description