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
#66 1074108-skip-profile-16-7.x.patch1.79 KBDevin Carlson
PASSED: [[SimpleTest]]: [MySQL] 41,138 pass(es).
[ View ]
#62 1074108-62.patch2.95 KBDevin Carlson
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,992 pass(es).
[ View ]
#62 interdiff-1074108-54-62.txt961 bytesDevin Carlson
#59 increment.txt631 bytespwolanin
#59 1074108-54.patch2.9 KBpwolanin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,747 pass(es).
[ View ]
#58 1074108-53.patch2.9 KBpwolanin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,449 pass(es).
[ View ]
#53 interdiff-46-53.txt932 bytesBlakeLawson
#53 1074108-profile-selection-53.patch1.04 KBBlakeLawson
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,420 pass(es).
[ View ]
#46 1074108-profile-selection-45.patch1.12 KBharijari
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,440 pass(es).
[ View ]
#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

pwolanin’s picture

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.

David_Rothstein’s picture

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.

pwolanin’s picture

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.

David_Rothstein’s picture

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.

David_Rothstein’s picture

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.

rfay’s picture

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.

pwolanin’s picture

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

rfay’s picture

Status:Needs work» Needs review

Retest passed fine, so putting NR

pwolanin’s picture

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

David_Rothstein’s picture

Haza’s picture

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

No changes, just porting the previous patch to d8.

scor’s picture

Title:Profile selection form not skipped if there is only one visibile profile» Profile 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?

webchick’s picture

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.

webchick’s picture

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?

webchick’s picture

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

webchick’s picture

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

amontero’s picture

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.

amontero’s picture

Issue tags:+Needs reroll

Tagging.

amontero’s picture

Issue tags:+Novice

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

oriol_e9g’s picture

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

henk’s picture

Assigned:Unassigned» henk
Issue summary:View changes
henk’s picture

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.

henk’s picture

Status:Reviewed & tested by the community» Needs review

one more review is needed.

henk’s picture

Assigned:henk» Unassigned
harijari’s picture

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

harijari’s picture

Status:Needs review» Reviewed & tested by the community
alexpott’s picture

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

GuGuss’s picture

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

pwolanin’s picture

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?

David_Rothstein’s picture

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.

David_Rothstein’s picture

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

webchick’s picture

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.

c_lehel’s picture

Assigned:Unassigned» c_lehel
c_lehel’s picture

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

David_Rothstein’s picture

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.

jayeshanandani’s picture

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.

jayeshanandani’s picture

Assigned:c_lehel» jayeshanandani

Status:Needs review» Needs work

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

harijari’s picture

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

harijari’s picture

Issue tags:+Needs a reroll

Patch doesn't apply on latest version of code.

harijari’s picture

Status:Needs work» Needs review

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

harijari’s picture

StatusFileSize
new1.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,440 pass(es).
[ View ]
harijari’s picture

Issue tags:-Needs a reroll
henk’s picture

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.

BlakeLawson’s picture

I'm going to reroll this patch.

Verified that bug still exists.

BlakeLawson’s picture

Rechecked and doesn't need reroll.

pwolanin’s picture

Status:Needs review» Needs work

minor - I think needs this doesn't match our usual format for using lambdas, like:

  $configurable_fields = array_keys(array_filter($field_definitions, function ($definition) {
    return $definition->isConfigurable();
  }));

Also, the 2nd code comment is not needed and => is not normal

BlakeLawson’s picture

Status:Needs work» Needs review
StatusFileSize
new1.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,420 pass(es).
[ View ]
new932 bytes

Reformatted array filter and removed unnecessary comment.

Verified locally that patch still works.

pwolanin’s picture

Status:Needs review» Reviewed & tested by the community

I think this is ready to commit, assuming the tests pass.

tstoeckler’s picture

Status:Reviewed & tested by the community» Needs work

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

pwolanin’s picture

@tstoeckler - good question. I was considering this also and it seems a little tricky since core ships with 2 visible profiles.

How do we make one of the core profiles hidden, or install core with only one profile?

My original use case was a repackaged distro where only 1 profile is present or visible.

pwolanin’s picture

If we want to test this, I think something closer to a unit test. Note that we have a special case for a distribution flag, the test for tat creates and extra profile in DistributionProfileTest, but that's not a trick we can play to remove profiles?

Or maybe we can put one with the same name as a core one but marked hidden?

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new2.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,449 pass(es).
[ View ]

Ok, this adds a test. Seems to work, though it's a bit of a hack to override a core profile?

The only change is the added test.

pwolanin’s picture

StatusFileSize
new2.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,747 pass(es).
[ View ]
new631 bytes

fix code comment.

tstoeckler’s picture

Status:Needs review» Reviewed & tested by the community

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.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
+++ 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?

Devin Carlson’s picture

Status:Needs work» Needs review
StatusFileSize
new961 bytes
new2.95 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,992 pass(es).
[ View ]

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

pwolanin’s picture

Status:Needs review» Reviewed & tested by the community

Thanks Devin - I'm think I copied the profile creation from DistributionProfileTest, and probably it's extraneous there as well now.

alexpott’s picture

Version:8.0.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

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!

  • alexpott committed f28179d on 8.0.x
    Issue #1074108 by pwolanin, webchick, BlakeLawson, jayeshanandani, Devin...
Devin Carlson’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new1.79 KB
PASSED: [[SimpleTest]]: [MySQL] 41,138 pass(es).
[ View ]

Reattaching the patch from #16.

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

mgifford’s picture

Assigned:harijari» Unassigned

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?