Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #3624: Change usage of '--config-dir' site-install option. #3625

Merged
merged 4 commits into from
Aug 20, 2018

Conversation

vpeltot
Copy link
Contributor

@vpeltot vpeltot commented Jul 20, 2018

Fix #3624

Regarding https://www.drupal.org/project/drupal/issues/2788777 that allow a site to be installed from an existing configuration from 8.6.x version, here an update to change the usage of --config-dir option.

Up to now, this option allowed to import a full set of configuration after installation.
From now, configuration does not import anymore after installation, but during installation.

@vpeltot vpeltot changed the title #3624: Change usage of '--config-dir' site-install option. fix #3624: Change usage of '--config-dir' site-install option. Jul 20, 2018
@weitzman
Copy link
Member

Thanks. We do need to remain compatible with 8.5 sites for a good while. if needed, you can tell what version of Drupal we are dealing with via

$drupal_root = Drush::bootstrapManager()->getRoot()
$version = Drush::bootstrap()->getVersion($drupal_root);

@vpeltot
Copy link
Contributor Author

vpeltot commented Jul 23, 2018

@weitzman I forgot that there is a lot of Drupal not up to date 😄

So, here a new commit to keep the backward compatibility + add new option to allow the installation from the config sync directory (--existing-config).

@bircher
Copy link
Contributor

bircher commented Jul 23, 2018

I tested the PR branch in one of my projects with Drupal 8.6.0-alpha1 and it worked. 🎉

We are using https://github.com/openeuropa/task-runner on that project, and the drush option would have to be added there too.

For others finding this, for now I just added the following to runner.yml for the normal run drupal:site-install to work:

drupal:
  pre_install:
    - { task: "symlink", from: "../../../../../config/sync", to: "${drupal.root}/core/profiles/minimal/config/sync" }
  post_install:
    - { task: "remove", file: "${drupal.root}/core/profiles/minimal/config/sync" }

@weitzman
Copy link
Member

The code looks fine, but it is confusing to see two options which do nearly the same thing. Would it be too thorny to reuse same option and have its behavior be different with 8.6+?

@neclimdul
Copy link
Contributor

I don't know, they seem different to me but we're probably thinking about it differently.

  1. --existing-config
  • No value.
  • Reads from $config_directories[CONFIG_SYNC_DIRECTORY] like cex and cim
  1. --config-dir='foo'
  • Has a value
  • Installs from the exact path given which can be separate from other config commands.

@weitzman
Copy link
Member

I think those are too similar and will confuse

@mike-potter
Copy link

Tested the --existing-config option. If I don't specify the "minimal" profile Drush assumes "standard" even though the existing config has "minimal" set in core.extensions.yml. The same is true when using --config-dir option. Does Drush need to know the profile when installing via config? If so, then it should probably read from config. But this is minor enough that I'd RTBC the current version if it will expedite getting this released before 8.6.0. People are going to need this asap to start using 8.6 since config-installer no longer exists.

Regarding the --existing-config vs --config-dir: they do different things. I agree they are somewhat similar but I also think they are properly named. --config-dir is the drush-only compatibility option for previous users. I don't expect people to use this much in 8.6+ workflow as the config sync should be set in settings.php already. --existing-config is the name of the new option in core so you don't want Drush changing this.

@weitzman
Copy link
Member

Thanks for testing. I'm warming up to the two option solution. Maybe we explicitly say that --existing-config is recommended for Drupal 8.6+

Where is --existing-config declared in core?

@weitzman
Copy link
Member

I bet that Drush does not need to pass a profile to install_drupal() when using the new --existing-config code path. Thats a bug we should fix in this PR. Its probably wise to leave the --config-dir code path alone as changes can mess up existing sites.

@mike-potter
Copy link

existing_config was added in the patch committed to 8.6 here: https://www.drupal.org/project/drupal/issues/2980670

@neclimdul
Copy link
Contributor

Yes, both methods where explicitly added as part of that patch.

@weitzman
Copy link
Member

Yes, it looks like Drush is passing Standard. Maybe we can just remove the fallback to Standard and let Drupal handle it? See

protected function determineProfile($profile, $options, $class_loader)
{
// --config-dir fails with Standard profile and any other one that carries content entities.
// Force to minimal install profile.
if ($options['config-dir']) {
$this->logger()->info(dt("Using 'minimal' install profile since --config-dir option was provided."));
$profile = 'minimal';
}
if (empty($profile)) {
$boot = Drush::bootstrap();
$profile = $boot->getKernel()->getInstallProfile();
}
if (empty($profile)) {
// If there is an installation profile that acts as a distribution, use it.
// You can turn your installation profile into a distribution by providing a
// @code
// distribution:
// name: 'Distribution name'
// @endcode
// block in the profile's info YAML file.
// See https://www.drupal.org/node/2210443 for more information.
require_once DRUSH_DRUPAL_CORE . '/includes/install.core.inc';
$install_state = ['interactive' => false] + install_state_defaults();
try {
install_begin_request($class_loader, $install_state);
$profile = _install_select_profile($install_state);
} catch (\Exception $e) {
// This is only a best effort to provide a better default, no harm done
// if it fails.
}
}
if (empty($profile)) {
$profile = 'standard';
}
return $profile;
}
.

@weitzman weitzman merged commit 5b27047 into drush-ops:master Aug 20, 2018
@weitzman
Copy link
Member

Lets keep working on option descriptions and default profile in new PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants