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.
Comment | File | Size | Author |
---|---|---|---|
#66 | 1074108-skip-profile-16-7.x.patch | 1.79 KB | Devin Carlson |
#62 | 1074108-62.patch | 2.95 KB | Devin Carlson |
#62 | interdiff-1074108-54-62.txt | 961 bytes | Devin Carlson |
#59 | increment.txt | 631 bytes | pwolanin |
#59 | 1074108-54.patch | 2.9 KB | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedThere 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.
Comment #2
David_Rothstein CreditAttribution: David_Rothstein commentedThat 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.
Comment #3
pwolanin CreditAttribution: pwolanin commentedAs 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.
Comment #4
David_Rothstein CreditAttribution: David_Rothstein commentedThe 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.
Comment #5
David_Rothstein CreditAttribution: David_Rothstein commentedMaybe 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.
Comment #6
rfayJust 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.
Comment #7
pwolanin CreditAttribution: pwolanin commented@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.
Comment #8
rfayRetest passed fine, so putting NR
Comment #9
pwolanin CreditAttribution: pwolanin commentedDavid's patch seems to work fine in at least the all-but-one hidden case.
Comment #10
David_Rothstein CreditAttribution: David_Rothstein commentedMarked #1468282: Skip the installation profile installer step if there's only one visible choice as a duplicate.
Comment #11
HazaNo changes, just porting the previous patch to d8.
Comment #12
scor CreditAttribution: scor commentedshouldn't this @todo be addressed in this D8 patch? or is it left for another issue?
Comment #13
webchickHm. 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.
Comment #14
webchickHere'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?
Comment #15
webchickOops. :P How about a patch that doesn't hide the minimal profile? :P
Comment #16
webchickYeah, confirmed that #15 (applied with patch -p2) solves the problem in D7 as well. Here's that.
Comment #17
amontero#15: 1074108-skip-profile-15.patch queued for re-testing.
Comment #19
amonteroTagging.
Comment #22
amonteroRestoring lost "needs reroll" tag. Also tagging as "novice".
Comment #23
oriol_e9gReroll
Comment #24
henk CreditAttribution: henk commentedComment #25
henk CreditAttribution: henk commented#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.
Comment #26
henk CreditAttribution: henk commentedone more review is needed.
Comment #27
henk CreditAttribution: henk commentedComment #28
jsobiecki CreditAttribution: jsobiecki commentedFrom 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 :)
Comment #29
jsobiecki CreditAttribution: jsobiecki commentedComment #30
alexpottI'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
Comment #31
GuGuss CreditAttribution: GuGuss commented@alexpott you're right and I agree that this change is obsolete now.
Marking a profile as exclusive like this:
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.
Comment #32
pwolanin CreditAttribution: pwolanin commentedok, so back to 7.x for this? Or we should be looking to backport the exclusive feature to 7.x instead?
Comment #33
David_Rothstein CreditAttribution: David_Rothstein commentedThe "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.
Comment #34
David_Rothstein CreditAttribution: David_Rothstein commented23: 1074108-skip-profile-23.patch queued for re-testing.
Comment #35
webchickHooray! I've wanted this in for a long time.
However, the latest patch no longer applies to D8.
Comment #36
c_lehel CreditAttribution: c_lehel commentedComment #37
c_lehel CreditAttribution: c_lehel commentedRerolled + it should work fine for D8
Comment #38
David_Rothstein CreditAttribution: David_Rothstein commentedRelatively 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.
Comment #40
jayeshanandani CreditAttribution: jayeshanandani commentedRerolled #37.
Comment #41
jayeshanandani CreditAttribution: jayeshanandani commentedComment #43
jsobiecki CreditAttribution: jsobiecki commentedI assigned this task to me (lack of updates in past 5 months). I'm working on this.
Comment #44
jsobiecki CreditAttribution: jsobiecki commentedPatch doesn't apply on latest version of code.
Comment #45
jsobiecki CreditAttribution: jsobiecki commentedI rewritten this patch. Installation forms were rewriten with FormAPI, and it changed lot of codebase (#2245249: Convert installer forms to FormInterface)
Comment #46
jsobiecki CreditAttribution: jsobiecki commentedComment #47
jsobiecki CreditAttribution: jsobiecki commentedComment #48
henk CreditAttribution: henk commentedif 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.Comment #50
BlakeLawson CreditAttribution: BlakeLawson commentedI'm going to reroll this patch.
Verified that bug still exists.
Comment #51
BlakeLawson CreditAttribution: BlakeLawson commentedRechecked and doesn't need reroll.
Comment #52
pwolanin CreditAttribution: pwolanin commentedminor - I think needs this doesn't match our usual format for using lambdas, like:
Also, the 2nd code comment is not needed and
=>
is not normalComment #53
BlakeLawson CreditAttribution: BlakeLawson commentedReformatted array filter and removed unnecessary comment.
Verified locally that patch still works.
Comment #54
pwolanin CreditAttribution: pwolanin commentedI think this is ready to commit, assuming the tests pass.
Comment #55
tstoecklerIt 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 toInstallerTestBase::setUpProfile()
(or otherwise in a subclass).Comment #56
pwolanin CreditAttribution: pwolanin commented@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.
Comment #57
pwolanin CreditAttribution: pwolanin commentedIf 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?
Comment #58
pwolanin CreditAttribution: pwolanin commentedOk, 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.
Comment #59
pwolanin CreditAttribution: pwolanin commentedfix code comment.
Comment #60
tstoecklerDamn, 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.
Comment #61
alexpottHow about a call to drupal_get_profile to ensure we've installed minimal?
Comment #62
Devin Carlson CreditAttribution: Devin Carlson commentedAdded a call to
drupal_get_profile()
per #61 and verified that the empty .profile file wasn't necessary per #60.Comment #63
pwolanin CreditAttribution: pwolanin commentedThanks Devin - I'm think I copied the profile creation from DistributionProfileTest, and probably it's extraneous there as well now.
Comment #64
alexpottThis 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!
Comment #66
Devin Carlson CreditAttribution: Devin Carlson commentedReattaching the patch from #16.
I don't think that there's any way to test this in D7.
Comment #67
mgiffordSo 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?
Comment #68
pwolanin CreditAttribution: pwolanin commented@mgifford - yep, that's generally how I was smoke-testing it
Comment #69
mgiffordOk, 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.
Comment #70
jsobiecki CreditAttribution: jsobiecki commented