This should make clearer:

- that its purpose is to find whether the installer should force a profile, i.e. not offer the user any choice.
- that returning NULL means the user will get the choice

Additionally, this sentence could be improved to be clearer (and made into a proper sentence):

> A discovered profile that is a distribution.

CommentFileSizeAuthor
#12 interdiff.txt1.19 KBfoxtrotcharlie
#12 improve-docs-for_install_select_profile-3005689-12.patch2.99 KBfoxtrotcharlie
#10 interdiff.txt2.99 KBfoxtrotcharlie
#10 improve-docs-for_install_select_profile-3005689-10.patch2.91 KBfoxtrotcharlie
#3 improve-docs-for_install_select_profile-3005689-3.patch1.62 KBfoxtrotcharlie

Comments

joachim created an issue. See original summary.

foxtrotcharlie’s picture

foxtrotcharlie commented
Assigned: Unassigned » foxtrotcharlie
foxtrotcharlie’s picture

foxtrotcharlie commented
Status: Active » Needs review
StatusFileSize
new improve-docs-for_install_select_profile-3005689-3.patch1.62 KB

Documentation added:

  • Tried to make it clear that in certain cases a profile will automatically be selected and subsequently installed.
  • And if no profile is selected and NULL is returned, then the user will be able to choose a profile.
  • Changed the return parameter type to "string|null"
  • Improved wording for the distribution profile selection condition.
joachim’s picture

joachim commented
Status: Needs review » Reviewed & tested by the community

Thanks for the patch! looks good to me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/includes/install.core.inc
@@ -1268,22 +1268,28 @@ function install_select_profile(&$install_state) {
- * Determines the installation profile to use in the installer.
+ * Determines whether or not to automatically install a profile by default.

This is not quite right.

This method does two things:
1. Automatically selects the profile under certain conditions
2. Returns the profile the user selected in the form by way of $install_state['parameters']['profile'] value.

The dual usage occurs because this is called on every request in the installer by install_begin_request()

I feel that the original sentence covers this better and the the additional docs you added give the nuance that's needed.

joachim’s picture

joachim commented

> 2. Returns the profile the user selected in the form by way of $install_state['parameters']['profile'] value.

I don't think that's the case.

_install_select_profile() is called twice.

1. by install_begin_request(), before install tasks have been collected, the first time the install page is shown. This is before any user input has been taken. This is the call that can decide to hide the profile selection task, and force a distribution profile or a singleton profile.

2. in the install task callback install_select_profile(), where the comments show that this is called before the form is output:

    // Try to automatically select a profile.
    if ($profile = _install_select_profile($install_state)) { <--- that's the call here.
      $install_state['parameters']['profile'] = $profile;
    }
    else {
      // The non-interactive installer requires a profile parameter.
      if (!$install_state['interactive']) {
        throw new InstallerException(t('Missing profile parameter.'));
      }
      // Otherwise, display a form to select a profile.
      return install_get_form('Drupal\Core\Installer\Form\SelectProfileForm', $install_state); <-- that's the form
    }
  }
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

install_begin_request() is called on every request made to the installer. An interactive install is made up of quite a few requests to the installer. The profile typically is selected on the third request, after the GET and POST on the language selection step. Each time it runs this method to determine what profile has been selected. If user selection was necessary, it will return the users choices based on the $install_state['parameters']['profile'] value.

foxtrotcharlie’s picture

foxtrotcharlie commented

Thanks @joachim and @alexpott. @alexpott is right - I've been going through the process in my IDE debugger, and once a profile is selected either automatically - which could happen on the first request - or by the user once the profile select form has been shown, that profile is then re-confirmed by _install_select_profile() during each request made during the install (from within install_begin_request()), and this happens many times after the initial selection.

So the method does serve to confirm a prior selection, be it automatic or from the user.

I will adjust my initial patch to reflect this for further comment.

joachim’s picture

joachim commented

> that profile is then re-confirmed by _install_select_profile() during each request made during the install (from within install_begin_request()), and this happens many times after the initial selection.

Huh! That sounds crazy! We should maybe change that in a separate issue.

But for now, let's document what the code does :)

foxtrotcharlie’s picture

foxtrotcharlie commented
Status: Needs work » Needs review
StatusFileSize
new improve-docs-for_install_select_profile-3005689-10.patch2.91 KB
new interdiff.txt2.99 KB

I've tried to add a general description as to the purpose of this method, without tying it too much to the context from which it is called, so in effect I attempt to indicate what it could be used for without making too many assumptions about the calling context.

The main use case appears to be in the context of install_select_profile() where it is used to either automatically select an installation profile, or display a form allowing the user to choose one of the visible profiles. That said, it is called many more times called from install_begin_request(). I'm not quite up to speed on the overall purpose of this just yet. Mostly it seems to load the selected profile.

Back to the documentation: I don't know if I've got it quite right yet, but hopefully this is a step in the right direction.

joachim’s picture

joachim commented 
  1. +++ b/core/includes/install.core.inc
@@ -1270,48 +1270,58 @@ function install_select_profile(&$install_state) {
+ * Depending on the context from which it's being called, this method
+ * may be used to:
+ * - Automatically select a profile under certain conditions.
+ * - Indicate which profile has already been selected.
+ * - Indicate that a profile still needs to be selected.

    This is good! 

  2. +++ b/core/includes/install.core.inc
@@ -1270,48 +1270,58 @@ function install_select_profile(&$install_state) {
  * A profile will be selected in the following order of conditions:
...
+ * If none of the above conditions are met, then this method will return NULL.
+ *

    Instead of that paragraph at the end, I would change the intro to the list to say something like:

    > A profile will be selected automatically if one of the following conditions is met. They are checked in the given order.

Other than that, this is looking great.

foxtrotcharlie’s picture

foxtrotcharlie commented
StatusFileSize
new improve-docs-for_install_select_profile-3005689-12.patch2.99 KB
new interdiff.txt1.19 KB

Thanks @joachim.

I removed the last line:

 * If none of the above conditions are met, then this method will return NULL.

Updated the line before the list of conditions to read:

 * A profile will be selected automatically if one of the following conditions
 * is met. They are checked in the given order:

I wasn't sure whether we needed a full stop or a colon at the end of this, went with the colon.

joachim’s picture

joachim commented
Status: Needs review » Reviewed & tested by the community

This looks good to me.

I see what you mean about colon or full stop, and ideally, the first sentence should mention the order so the second sentence can say 'the following', but I couldn't figure out a way when I wrote my comment yesterday. I think this is good enough now. It's readable, it's clear, it's comprehensive, it doesn't have to be Literature ;)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed d9ccf86f12 to 8.7.x and 7877694845 to 8.6.x. Thanks!

  • alexpott committed d9ccf86 on 8.7.x 
    Issue #3005689 by foxtrotcharlie, joachim, alexpott: improve docs for...

  • alexpott committed 7877694 on 8.6.x 
    Issue #3005689 by foxtrotcharlie, joachim, alexpott: improve docs for...

Status: Fixed » Closed (fixed)

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