The interactive installer should skip the first form for selecting profiles if there is only one visible.

The current logic is wrong - it only skips the form if there is only one profile present, but with the addition of hidden profiles there may be ones like the testing profile that are present but not on the form.

Files: 
CommentFileSizeAuthor
#40 interdiff-37-40.txt717 bytesjayeshanandani
#40 1074108.40.patch1.7 KBjayeshanandani
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,407 pass(es), 0 fail(s), and 12 exception(s).
[ View ]
#37 1074108-skip-profile-37.patch1.71 KBc_lehel
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1074108-skip-profile-37.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#11 1074108-skip-profile-11.patch2.17 KBHaza
PASSED: [[SimpleTest]]: [MySQL] 35,659 pass(es).
[ View ]
#2 1074108-skip-profile-2.patch2.29 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 31,570 pass(es).
[ View ]
#1 1074108-skip-1-profile-1.patch3.25 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 31,560 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new3.25 KB
PASSED: [[SimpleTest]]: [MySQL] 31,560 pass(es).
[ View ]

There was no really elegant way to handle this case. This patch uses install_goto() and does a little cleanup on confusing variable names in the profile selection form.

StatusFileSize
new2.29 KB
PASSED: [[SimpleTest]]: [MySQL] 31,570 pass(es).
[ View ]

That patch seems like more of an API change than necessary. Plus, it doesn't work for command-line installs (less of a concern than the UI issue, but still worth fixing).

How about the attached patch? Since install_profile_info() is cached, it should be fine to call it twice per profile within the same install task.

As best as I can tell - command line installs are SUPPOSED to be able to select hidden profiles, right? What am I missing? How else will the tests run with the testing profile?

I think the logic in #2 is wrong - we only want to hide profiles on the form.

The testing framework does not use command line installs.

But more important, the patch in #2 does not prevent command line installs from selecting a hidden profile if they want to. Try it - it still works fine.

Maybe just to clarify, the reason this doesn't prevent anyone from using a hidden profile is that it all occurs within an if (empty($install_state['parameters']['profile'])) block. So if you specify a hidden profile on the command line (or by putting it in the URL, for that matter) it will still be used because the code we are adding here doesn't run in that case.

The code here only affects situations where you did not specify a profile at all, and are trying to rely on Drupal to select one.

Status:Needs review» Needs work

Just putting this in CNW because the testbots seem to be testing over and over. Trying to break whatever loop is going on. OK to retest later.

@David - it seemed to be part of the design that hidden profiles can be installed via CLI?

Anyhow, it sounds like there might in fact be issues with tests given rfay's comment.

Status:Needs work» Needs review

Retest passed fine, so putting NR

David's patch seems to work fine in at least the all-but-one hidden case.

StatusFileSize
new2.17 KB
PASSED: [[SimpleTest]]: [MySQL] 35,659 pass(es).
[ View ]

No changes, just porting the previous patch to d8.

Title:Profile selection form not skipped if there is only one visibile profileProfile selection form not skipped if there is only one visible profile

+++ b/core/includes/install.core.inc
@@ -1138,6 +1150,8 @@ function install_select_profile_form($form, &$form_state, $profile_files) {
+    // @todo: This duplicates logic in install_select_profile() and is here
+    //   only for backwards compatibility. Remove it in Drupal 8.

shouldn't this @todo be addressed in this D8 patch? or is it left for another issue?

Status:Needs review» Needs work

Hm. Given those comments are above a line of code that's unaffected by this patch, I think a separate issue is fine. I imagine it was probably one of those things that was noticed while David was deep in the guts of the installer trying to figure this out, and so he decided to leave breadcrumbs for later. (He could obviously speak for himself on this point. :))

This is a really nice patch for distributions because it allows them to apply a small patch like #1630110-1: [Meta] Core patches specific to Kickstart to start the installer directly at the language selection screen (important for non-native speakers), rather than confounding people with a strange choice from out of the gate.

While #1727430: Add 'exclusive' flag to install profiles to auto-select them during installation. is also in progress, and that seems like ultimately a better solution for this specific problem, I would still argue that the behaviour here is buggy and should be fixed. My first attempt at fixing this in Spark was to delete the standard and minimal profiles, and then I was completely confused why it was still showing "Spark" as a selection, given there were no other visible choices.

Patch in #11 no longer seems to apply to 8.x, though.

Status:Needs work» Needs review
StatusFileSize
new2.25 KB
PASSED: [[SimpleTest]]: [MySQL] 41,246 pass(es).
[ View ]

Here's a re-roll. This also allowed me to read the code the comment was referencing, and what it basically meant was that since $install_state['profiles'] already had hidden profiles removed from it, don't bother checking for them again. So I removed that check from the code as well.

However, since D8 changed a bit and sends a full $install_state to install_select_profile_form() (not just a list of profiles), I actually think we can solve it by modifying the contents of $install_state['profiles'] after removing the hidden ones in install_select_profile(), and it seems like that would apply in both D7 and D8 the same?

StatusFileSize
new1.82 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1074108-skip-profile-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Oops. :P How about a patch that doesn't hide the minimal profile? :P

Yeah, confirmed that #15 (applied with patch -p2) solves the problem in D7 as well. Here's that.

Issue tags:-needs backport to D7

#15: 1074108-skip-profile-15.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+needs backport to D7

The last submitted patch, 1074108-skip-profile-15.patch, failed testing.

Issue tags:+Needs reroll

Tagging.

Issue tags:+Novice

Restoring lost "needs reroll" tag. Also tagging as "novice".

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new1.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,053 pass(es).
[ View ]

Reroll

Assigned:Unassigned» henk
Issue summary:View changes

Status:Needs review» Reviewed & tested by the community

#23 patch is working with current Drupal 8 version and skip the profile selection if is only one profile or only one is not hidden.

Status:Reviewed & tested by the community» Needs review

one more review is needed.

Assigned:henk» Unassigned

From functional point of view:

I confirm described issue. Patch applies cleanly, and fixes issue. Selection step was omited, when only one profile is visible.

From implementation point of view

Change is minor, and It's look clean for me. No comments here.

Summary

It fixes problem and is good written :)

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs review

I'm not sure I really get this change now that we have #1727430: Add 'exclusive' flag to install profiles to auto-select them during installation. - how do you get only one profile without hacking core?

If you have a distribution and you want this step skippable use the exclusive setting in the distribution profile's .info.yml

@alexpott you're right and I agree that this change is obsolete now.

Marking a profile as exclusive like this:

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

allows you to skip all the other profiles and directly go to the choose language step of the installer. This was the point of that issue.

Version:8.x-dev» 7.x-dev

ok, so back to 7.x for this? Or we should be looking to backport the exclusive feature to 7.x instead?

Version:7.x-dev» 8.x-dev
Priority:Normal» Minor
Status:Needs review» Reviewed & tested by the community

The "exclusive" feature was already backported to Drupal 7.

But none of this makes it any less of a bug, just lower priority to fix. The existing code in core is still incorrect, and relies on a coincidence elsewhere in the codebase (the fact that core currently happens to ship with two profiles rather than one) that it shouldn't be relying on.

Code still looks good, and I did a quick manual test to verify that #23 behaves correctly.

23: 1074108-skip-profile-23.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Hooray! I've wanted this in for a long time.

However, the latest patch no longer applies to D8.

Assigned:Unassigned» c_lehel

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new1.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1074108-skip-profile-37.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled + it should work fine for D8

Status:Needs review» Reviewed & tested by the community

Relatively straightforward reroll, and manual testing indicates the patch still works. Back to RTBC.

I noticed that perhaps the code in this patch that removes hidden profiles from $install_state['profiles'] should move earlier in the bootstrap (that would allow, for example, the "Choose profile" step to not even show up in the task list in this case; see install_tasks()). But that would be fine as a followup.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 37: 1074108-skip-profile-37.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,407 pass(es), 0 fail(s), and 12 exception(s).
[ View ]
new717 bytes

Rerolled #37.

Assigned:c_lehel» jayeshanandani

Status:Needs review» Needs work

The last submitted patch, 40: 1074108.40.patch, failed testing.