backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

The `install.sh` script takes badly constructed option as a `profile` argument #4819

Open alanmels opened 3 years ago

alanmels commented 3 years ago

Description of the bug

Trying to install Backdrop through the command line, I've run the following command:

./core/scripts/install.sh --db-url='mysql://user:user@db/default' --account-mail="admin@example.com" --account-name="admin" --account-pass="admin" --site-mail="admin@example.com" -site-name='AltaGrade Test'

just to get the error message:

An error occurred. Output of installation attempt is as follows:
Sorry, the profile you have chosen cannot be loaded.

Additional information

The function responsible for the above failure is in install.core.inc and looks like:

function install_load_profile(&$install_state) {
  $profile_file = _install_find_profile_file($install_state['parameters']['profile']);
  if ($profile_file) {
    include_once $profile_file;
    $install_state['profile_info'] = install_profile_info($install_state['parameters']['profile'], $install_state['parameters']['langcode']);
  }
  else {
    throw new Exception(st('Sorry, the profile you have chosen cannot be loaded.'));
  }
}

Checking the content of $install_state just after executing the CLI command shows it constructs incorrect profile name from the -site-name option:

Array
(
    [parameters] => Array
        (
            [profile] => sitenameAltaGradeTest
            [locale] => en
            [langcode] => en

Omitting the -site-name= option allows the command execute normally ending with successful install.

alanmels commented 3 years ago

The culprit was about missing hyphen in the option. With --site-name='AltaGrade Test' makes the ./core/scripts/install.sh script work as expected. However, the question why the script takes an incorrectly constructed option as the profile argument still holds.

alanmels commented 3 years ago

Turns out the code block that starts from line 66 and looks like

// Parse provided options.
while ($param = array_shift($_SERVER['argv'])) {
  if (strpos($param, '--') === 0) {
    $param = substr($param, 2);
    if (strpos($param, '=')) {
      list($key, $value) = explode('=', $param);
      $options[$key] = $value;
    }
    else {
      $options[$param] = array_shift($_SERVER['argv']);
    }
  }
  else {
    $arguments[] = $param;
  }
}

puts -site-name='AltaGrade Test' with a single hyphen into $arguments[0] and since that moment the whole script goes awry. I'll try to offer PR soon.

indigoxela commented 3 years ago

Shouldn't that be --site-name='AltaGrade Test' with two hyphens? If the command line you provided is exactly the one you used, the first thing to try is to add that missing hyphen.

alanmels commented 3 years ago

@indigoxela, right, and here is the quote from my post of 14 hours ago:

The culprit was about missing hyphen in the option. With --site-name='AltaGrade Test' makes the ./core/scripts/install.sh script work as expected. However, the question why the script takes an incorrectly constructed option as the profile argument still holds.

indigoxela commented 3 years ago

the question why the script takes an incorrectly constructed option ...

Ah, thanks, I think now I got it. :wink:

Wouldn't it be a better approach then to either discard any invalid option or completely fail (stop) if incorrect params are provided?

alanmels commented 3 years ago

Not sure what's the best solution. That's why my PR contains minimal change not to mess with the core in https://github.com/backdrop/backdrop-issues/issues/4819#issuecomment-748665458

indigoxela commented 3 years ago

elseif ($arguments[0] != 'testing' && $arguments[0] != 'minimal') {

What worries me a bit with this is, that there's no way to use a custom (different) profile then. Maybe one of the core committers wants to chime in?

alanmels commented 3 years ago

You probably are right and another PR needs to be created by more experienced Backdrop experts. I just can add that it's not clear to me why profile needs to be fed as a separate argument instead of another option. What I mean is that a new option --profile could be introduced and then the whole issue would become much simpler: only correctly constructed options, including --profile would be taken in and all other parameters - ignored.

ghost commented 2 years ago

Not sure what the answer is here, since a parameter that doesn't start with a double hyphen (--) could be a profile name or a key/value argument...

Unless we can definitively state that profiles and form keys cannot start with a hyphen, I'm not sure we can do anything about that here. Can profiles/form keys start with a hyphen?