Profile selection form not skipped if there is only one visible profile

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.

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.

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.

Retest passed fine, so putting NR

Marked #1468282: Skip the installation profile installer step if there's only one visible choice as a duplicate.

No changes, just porting the previous patch to d8.

+++ 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?

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.

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?

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.

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

Tagging.

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

Reroll

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

one more review is needed.

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

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.

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.

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

However, the latest patch no longer applies to D8.

Rerolled + it should work fine for D8

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.

Rerolled #37.

I assigned this task to me (lack of updates in past 5 months). I'm working on this.

Patch doesn't apply on latest version of code.

I rewritten this patch. Installation forms were rewriten with FormAPI, and it changed lot of codebase (#2245249: Convert installer forms to FormInterface)

if it's only one visible profile, selection page is skipped. Patch works ok and if code is still not obsolete after implementation exclusive = TRUE can be implemented.

I'm going to reroll this patch.

Verified that bug still exists.

Rechecked and doesn't need reroll.

Reformatted array filter and removed unnecessary comment.

Verified locally that patch still works.

It should be possible to write tests for this, right? Sorry for being a bummer about that, but unless it affects the specific the testbot installs Drupal these things are never caught and since even duct-tape is a compliment for the installer at this point I wouldn't be surprised if we break this again in two weeks fixing some other bug that crops up.

For ideas on how to test this take a look at InstallerTestBase. It might even be possible to simply add an assertion to InstallerTestBase::setUpProfile() (or otherwise in a subclass).

Damn, I'm so much of an InstallerTestBase fanboy, that I did not think it through, when I requested tests. Hiding install profiles is indeed not easily doable. You found a way, though, and - while being hacky - I think is the best we can do and doesn't actually harm anyone. Thanks a lot for figuring this out and sorry again for mindlessly requesting tests! pwolanin++ tstoeckler--

Interesting that you had to create an empty .profile file, I thought that should no longer be needed? Did the tests fail with that?

Assuming the answer is yes, so marking RTBC.

+++ b/core/modules/system/src/Tests/Installer/SingleVisibleProfileTest.php
@@ -0,0 +1,69 @@
+  public function testInstalled() {
+    $this->assertUrl('user/1');
+    $this->assertResponse(200);
+    // Confirm that we are logged-in after installation.
+    $this->assertText($this->root_user->getUsername());
+  }

How about a call to drupal_get_profile to ensure we've installed minimal?

Added a call to drupal_get_profile() per #61 and verified that the empty .profile file wasn't necessary per #60.

This issue is a minor bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed f28179d and pushed to 8.0.x. Thanks!

Reattaching the patch from #16.

I don't think that there's any way to test this in D7.

So to manually test this, all that's needed is to apply the patch, delete one of the default profiles and see if a step is skipped in the installation?

Ok, I applied the patch, deleted a profile and it skipped the step where I would choose that in the installation. Good stuff!

Now I can't see how this would affect an existing installation, so there really should be no legacy issues here.

The code for D7 is also more complicated than it is for D8. @Devin Carlson wrote them both, so it shouldn't be a problem. But it might make sense for someone who is more familiar with this piece of the code in D7 should look over the code to verify it all makes sense before marking it RTBC.

It works fine though and acted like I expected it would.