Allow a site-specific profile to be installed from existing config

Not sure that this is related #2747179: Import config depending on custom profile modules/themes crashes.

Initial work to support the profile key. Nothing to test because there's no changes yet to anything under test.

So to use this you need a profile with config_install: true in its .info.yml and then a full config export in the profile's config/sync directory.

I think an improvement would be to rename the .info.yml config_sync</code and instead of a boolean allow it to be a directory so the name and location of the config sync directory can be specified. This would allow it to be outside of webroot.

Beginning to add test coverage - no real tests yet just the necessary fixtures. Manual testing of multi-lingual (the most complex core case) shows we need to set the install language to the already selected default language in config. No interdiff just it'd be huge.

Re #8: I wonder why we need so many config YAMLs just to test this. Probably we can stick to a minimum default configs.

I think an improvement would be to rename the .info.yml config_sync

Hm, anyway, config/sync doesn't make much sense. Why not directly config/install? If a site is installed from the existing config stored in a profile, when a developer updates and export the config he will get diffs in config/install. Then he decides what will be committed. I don't see the reason of having different directories in the case when the site is installed from a profile containing the default config. Or I'm missing something.

Using langcode from existing config is great challenge!

Why not directly config/install?

because better prevent mix of signed/unsigned config, otherwise we will endup with a mess in profiles

PS: looks related https://github.com/previousnext/drush_cmi_tools

Fix coding standard issue (stray space between ][).
In install_begin_request(), don't set $install_state['parameters']['langcode'] only to immediately overwrite it.

Ok, so I spent most of Friday of the DCdublin sprint on manually testing this patch and I found some discrepancies.

First of all it only works when settings.php is relatively vanilla. For example it doesn't work when the $config_directories['sync'] is set to a different folder than the profiles config/sync folder, but I guess that is a limitation that when documented we can live with for now, and fix later.

On the other hand there is something going wrong with the configuration synchronisation.
I wrote a test to demonstrate it. The language negotiation for example has some quirks, and when manually testing it I also found that the translations get messed up.

I am setting it to needs review so that the test bot picks it up and puts it back to needs work.

Another issue with this approach is that the config sync directory then becomes predictable when an install profile was used. If this is obfuscated normally for security purposes, then why is it acceptable to deobfuscate it in this instance?

We definitely shouldn't have @see references to config_installer functions in core.

Great work guys!

I've tested the patch #6 with a simple custom profile, with no multi-languages, the only issue that I had was when I exported the "system.site.yml" without the UUID information, during the installation I received the Notices:

Notice: Undefined index: uuid in install_config_import_batch() (line 2225 of core/includes/install.core.inc).
install_config_import_batch(Array) (Line: 593)
install_run_task(Array, Array) (Line: 542)
install_run_tasks(Array) (Line: 119)
install_drupal(Object) (Line: 44)

Notice: Undefined index: uuid in Drupal\Core\Config\StorageComparer->validateSiteUuid() (line 395 of core/lib/Drupal/Core/Config/StorageComparer.php).
Drupal\Core\Config\StorageComparer->validateSiteUuid() (Line: 75)
Drupal\system\SystemConfigSubscriber->onConfigImporterValidateSiteUUID(Object, 'config.importer.validate', Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('config.importer.validate', Object) (Line: 726)
Drupal\Core\Config\ConfigImporter->validate() (Line: 513)
Drupal\Core\Config\ConfigImporter->initialize() (Line: 2244)
install_config_import_batch(Array) (Line: 593)
install_run_task(Array, Array) (Line: 542)
install_run_tasks(Array) (Line: 119)
install_drupal(Object) (Line: 44)

Since the site UUID is a required config, should we validate and throw an exception during the installation if this information is not present?

If #18 make sense, follow the patch including the Site UUID validation.

the only issue that I had was when I exported the "system.site.yml" without the UUID information, during the installation I received the Notices:

So you hacked the system.site file to get in this situation. That said since we're depending on it this validation makes sense. Also we can't automatically populate it unless the installer can write back the changes to the sync config.

I had a discussion with @dixon_ recently where he was wondering whether or not the install profile sync config can come without UUIDs. If it does and we can't write back to it then the moment you've installed you'll not be able resync. If we want to support that then I think we should discuss that in a followup and not part of this issue.

Rerolled and brought some more fixes over from the config_installer.

The array syntax change is fun!

Fixing one bug... stuff still fails though.

Abstract test base classes are not supposed to run...

Here is a minor review. The patch looks quite solid to me. It has tests now so going to remove the tag. I think we are moving https://www.drupal.org/project/config_installer to the core. Do we need some approvals here? @alexpott is already working on this so I think subsystem and framework reviews are not required. Maybe, he should comment to state his reasons. I think we do need a release manager review here but before that let's update the issue summary.

1. +++ b/core/includes/install.core.inc
   @@ -445,10 +449,16 @@ function install_begin_request($class_loader, &$install_state) {
   +  if (isset($install_state['profile_info']['config_install']) && $install_state['profile_info']['config_install']) {
   

   Why do we need the second part of the condition? Can it be just !empty()?

2. +++ b/core/includes/install.core.inc
   @@ -789,6 +799,25 @@ function install_tasks($install_state) {
   +      // @todo add a load of commentary about what is happening.
   

   Good idea. It took me while to understand the code the docs will help a lot here.

3. +++ b/core/includes/install.core.inc
   @@ -1239,11 +1268,16 @@ function _install_select_profile(&$install_state) {
   +  if (!Settings::get('extension_discovery_scan_tests')) {
   

   Is there an existing issue for this? It seems like a bug. Do we need a test coverage for this?

4. +++ b/core/includes/install.core.inc
   @@ -1239,11 +1268,16 @@ function _install_select_profile(&$install_state) {
   +      return !isset($profile_info['hidden']) || !$profile_info['hidden'];
   

   !(isset($profile_info['hidden']) && $profile_info['hidden']) Would be more efficient here, imo.

5. +++ b/core/includes/install.core.inc
   @@ -2217,3 +2251,190 @@ function install_write_profile($install_state) {
   +    \Drupal::service('string_translation')
   ...
   +      'finished' => 'install_config_import_batch_finish',
   +      'title' => t('Synchronizing configuration'),
   +      'init_message' => t('Starting configuration synchronization.'),
   +      'progress_message' => t('Completed @current step of @total.'),
   +      'error_message' => t('Configuration synchronization has encountered an error.'),
   ...
   +    drupal_set_message(\Drupal::translation()->translate('The configuration synchronization failed validation.'));
   

   Let's create a local variable for translation service.

6. +++ b/core/includes/install.core.inc
   @@ -2217,3 +2251,190 @@ function install_write_profile($install_state) {
   +      drupal_set_message(\Drupal::translation()->translate('The configuration was imported with errors.'), 'warning');
   ...
   +    $message = \Drupal::translation()
   

   This can also be converted to a local variable.

7. +++ b/core/includes/install.core.inc
   @@ -2217,3 +2251,190 @@ function install_write_profile($install_state) {
   +      \Drupal::service('string_translation')
   ...
   +      drupal_set_message(\Drupal::translation()->translate('The configuration synchronization failed validation.'));
   

   Here as well.

8. +++ b/core/includes/install.inc
   @@ -482,12 +483,20 @@ function _drupal_rewrite_settings_dump_one(\stdClass $variable, $prefix = '', $s
   +    // @todo Should the info key be config_install or config_sync? The directory
   +    //   can't be config/install because that would clash with module install.
   ...
   +      $config_directories[CONFIG_SYNC_DIRECTORY] = $install_state['profiles'][$profile]->getPath() . '/config/sync';
   

   config/sync seems alright. Can we remove the @todo now?

9. +++ b/core/lib/Drupal/Core/Installer/Form/SelectProfileForm.php
   @@ -30,7 +31,7 @@ public function buildForm(array $form, FormStateInterface $form_state, $install_
   -      if ($details['hidden'] === TRUE && !drupal_valid_test_ua()) {
   +      if ($details['hidden'] === TRUE && !(drupal_valid_test_ua() || Settings::get('extension_discovery_scan_tests'))) {
   

   Once again this seems bug to me. We should move this to a separate issue and tests for this.

10. +++ b/core/profiles/testing_config_install/config/sync/README.txt
    @@ -0,0 +1 @@
    \ No newline at end of file
    

    EOF missing.

Thanks for the review @jibran.

1. I think you are right - !empty() is fine
2. Still @todo
3. The test coverage here is the test coverage :) This is the type of thing that is really hard to test because the entire test system relies on it being TRUE
4. Or even just return empty($profile_info['hidden']);
5. Hmmm... that's probably not a good idea because of potx extraction. I'd rather just use t() everywhere and leave the t() in procedural code issues for other issues.
6. Same as 5 - lets use t()
7. Same as 5 - lets just use t() like alot of the rest of the installer
8. That @todo is about the key in the profile's info.yml and whether it should be config_install: true or config_sync: true or even some_key: DIR. I don't think the last one works for what it is worth. I think it still worth getting opinions. At the moment the is using config_install: true.
9. I don't know - this is the first issue that adds test profiles that need to be selected via the UI. Happy to make a blocking issue BUT adding a specific test for it when the tests added here will break the moment it break seems unnecessary.
10. Well this file is added by the config system by the code. Not sure we should be updating it. And if the sync directory is writable we're just going to replace it anyway. See drupal_install_config_directories()

Also adding the needs tests tag back - yes we have a couple of perfect path test cases but we need to test config validation and what happens when there is an error. This might lead me to change the way the test install profiles are created and instead do that doesn't require fixing the profile selection page for test profiles.

8) I think some_key: DIR is not a bad idea, it gives the flexibility of defining the sync directory to be wherever one wants it to be. And since the limitation of this approach here is anyway that it works only with custom profiles I don't think this is too much of a stretch. The code to detect the directory is added here anyway.

9) I would not change the profile selection UI much and instead add the test profiles similar to how config_installer does. Ie adding the test profile on the fly, this would also allow us to have the configuration zipped.

When thinking about the limitation this approach has over the dance we have to do with "any" profile (ie a distribution) I think we could simplify it even more and instead just change the UUID on install, if you have the profile be the site you could even add the uuid to the profiles info file. (Or detect it from an existing sync directory wherever that may be). Then the site is installed the same way it is now and the config sync will work (provided you chose the same profile). Following this train of thought we could also just add a checkbox to the config sync that when checked removes all content entities and sets the UUID to the one in the sync directory and then runs a normal config import (limited to sites installed with the same profile as the one in the config to sync).

Otherwise we add a limited functionality to install existing sites that will give developers false hopes and the illusion sites will be able to be re-installed. This will be especially true if the profile inheritance becomes a reality (because it will fail horribly when you uninstall modules the base profile expects).

So one day we will have to face the fact that we allow installation profiles to expect modules to be installed in the hook_install but allow site builders to uninstall modules the profile depended on.
There are two options, re-write the core provided distributions (standard) and tell other distribution maintainers to be aware of that, or execute the profiles hook_install in a try catch block and allow it to fail.

I don't want to derail this issue and I would be happy to use this for our projects and use custom install profiles.
After all it is very easy for a developer to change which profile a site was installed with in core.extension and re-install the site after the configuration is exported.

I will give this a spin next week or so and give more feedback on the actual code.

I tried out this patch yesterday with some custom profile.

It worked fine once I figured out that I have to get rid of the core.extension.yml file and put the list of modules into the $profile.info.yml.
I wonder what we could do about that?

@dawehner that's strange - neither the tests added here nor a project I use this patch on need to do that. In fact adding dependencies to info.yml is exactly what not to do because profile dependencies aren't real dependencies.

So I've implemented a new approach to testing - it creates a profile from a tarball - including the profile machine name so it is as simple as doing a config export and running tar -zcvf SOMETESTNAME.tar.gz * in the config export directory and placing that in a fixtures folder and writing a very simple test that extends \Drupal\system\Tests\Installer\InstallerExistingConfigTestBase(). This approach also makes it simple for contrib to use the same test functionality - just incase they do something funky on config import.

So I've managed to avoid fixing how we scan for profiles when $settings['extension_discovery_scan_tests'] = TRUE; - we probably should file another issue for that - but it is not related.

Plus I talked to @dawehner in real life and he didn't realise that he had to add config_install: true to his profile. The ensuing discussion resulted in the realisation that we can just detect the presence of a config/sync directory can go from there. No flag to add in the install profile :)

I think the next patch will move $install_state['profile_info']['config_install'] out of profile_info and make it a generic thing on install state.

So now the install_state has two new properties that are copied from the profile if the profile has a config/sync directory. This will play much nicer with the work that @bircher is doing in #1613424: [META] Allow a site to be installed from existing configuration - all that issue will have to do is set up the $install_state['config_install'] property (which is now a directory) and also read the system.site config into the $install_state['config'] property.

Fixing patch.

Whoops - inverted some logic by mistake.

The change in 42 seems to have broken something. Specifically as the $config_directories[CONFIG_SYNC_DIRECTORY] is now set to the value of $install_state['config_insta''] this creates a discrepancy in the path used to load the initial system.site and what is later used for the configuration import.

In my case all configuration is in the profile's config/sync folder. This properly triggers the installer to use this folder for configuration and I can see it loads the system.site file properly. However, once it comes time to import the config it'll now use the value of $config_directories[CONFIG_SYNC_DIRECTORY] which is ../config/sync. That folder references to the root config folder (install is in /web) which is empty as no site has ever been installed before.

This is probably fixed by prefixing $config_directories[CONFIG_SYNC_DIRECTORY] with $config_directories[CONFIG_SYNC_DIRECTORY] = again like was done in the patch of #41 around:

+++ b/core/includes/install.inc
@@ -488,9 +488,9 @@ function drupal_install_config_directories() {

This prefixing with the profile path is actually properly done in install_profile_info

EDIT:
I'm unsure why the test didn't fail but my best guess would be that testing is being done with a non-empty config/sync directory?

A follow-up issue could be that if this install method is used the config/sync folder no longer needs to exist (which is the case now).

@Kingdutch thanks for testing - I can see how that occurs we need to ensure that the config being imported from is in the profile and error earlier if not. Your use-case sounds like it will be fixed by #1613424: [META] Allow a site to be installed from existing configuration - which will allow a site to be installed from directory outside of webroot that is already declared in settings.php. This issue is going to get the use-case of an install profile have a full config export working.

Hi @alexpott, no problem! In my test I had actually moved the export from my website from config/sync (which is outside the web root) into web/profiles/myprofile/config/sync (which is inside the web root). The profiler seemed to error out because although it found the configuration in the install profile initially, when it came to actually importing the configuration it decided to look into the config/sync (outside of the web root) folder. Which of course was empty, causing the installation to crash.

I've also tried removing the config/sync (outside of web root) folder but then the installation won't start because it requires the folder needs to exist and/or be writable. That seems like it may be a good follow-up (no longer require a config/sync folder outside of the profile if you choose this installation path).

btw Having whole config/sync anywhere in web-root accessible from outside is a big question for me.

Guess changing docs and .htaccess also needed

@alexpott The issue looks a duplicate of #1613424: [META] Allow a site to be installed from existing configuration with a difference that no way here to bypass a directory with config snapshot

Once someone actually reads the manual this patch works great for me.

1. +++ b/core/includes/install.core.inc
   @@ -445,10 +449,16 @@ function install_begin_request($class_loader, &$install_state) {
   +  if (!empty($install_state['config_install'])) {
   +    $install_state['parameters']['langcode'] = $install_state['config']['system.site']['default_langcode'];
   

   Should we check whether this value is actually set?

2. +++ b/core/includes/install.core.inc
   @@ -2217,3 +2253,189 @@ function install_write_profile($install_state) {
   +  if ($success) {
   +    if (!empty($results['errors'])) {
   ...
   +  }
   +  else {
   +    // An error occurred.
   

   This is quite confusing ... how can $success be true, if there are errors? Oh I see this is about batch errors vs. config errors, maybe a comment would be helpful.

As per #47.

+++ b/core/includes/install.inc
@@ -1090,6 +1100,12 @@ function install_profile_info($profile, $langcode = 'en') {
+    // If the profile has a config/sync directory use that to install drupal.
+    if (is_dir($profile_path . '/config/sync')) {
+      $info['config_install'] = $profile_path . '/config/sync';
+      $sync = new FileStorage($profile_path . '/config/sync');

Can we allow profiles to nominate this directory in info.yml file? Default can stat '/config/sync'.

Can we allow profiles to nominate this directory in info.yml file? Default can stat '/config/sync'.

Is there a usecase in placing the config somewhere else? Otherwise it feels a bit like an overarchitecture ...

I'm planning to use this patch with #2793445: Allow BTB to test an existing, already installed Drupal site instead of installing from scratch on D8 site. My current workflow to run the tests is

drush -r webroot sql-sync @dev @self -y
drush -r webroot updb -y
drush -r webroot entity-updates -y
drush -r webroot cimy -y --skip-modules=module1,module2 --source=config-export --install=config-install --delete-list=config-delete.yml
./bin/phpunit

If I can nominate sync dir as config-export dir then after this patch it would be

drush -r webroot si my_profile -y
./bin/phpunit

Right now, I can do that with cd webroot/profiles/custom/my_profile/config;ln -s ../../../../../config-export sync but it'd be nice to nominate the directory.

Note: cimy command can be found at https://github.com/previousnext/drush_cmi_tools

It turns out my problems were caused by a comment in #14 that I missed.

First of all it only works when settings.php is relatively vanilla. For example it doesn't work when the $config_directories['sync'] is set to a different folder than the profiles config/sync folder, but I guess that is a limitation that when documented we can live with for now, and fix later.

Setting $config_directories[CONFIG_SYNC_DIRECTORY] = $app_root . '/profiles/myprofile/config/sync'; made the patch work like a charm.

It sounds like it should be possible to just not set this variable since we can't be sure where the install profile is located? (Although the user controls settings.php and/or the profile location). If it is on purpose that this should always point to the profile's sync folder then I don't understand all the logic in the patch for figuring out the config/sync folder's location relative to the install profile.

@Eli-T and I worked on a Drush command to create an install profile for an existing site that works with this patch - see https://www.drupal.org/project/install_profile_generator

We're very interested in this feature. But for us, #2792877: Symfony YAML parser fails on some strings when running PHP 7 blocks this feature :-(

@jibran and @Kingdutch we are explicitly reducing the scope of this change to only deal with install profiles containing configuration. Doing anything else is the scope of #1613424: [META] Allow a site to be installed from existing configuration. The point of the issue is to solve a very specific use-case and get the plumbing for configuration sync during import in place. This is already outlined in the issue summary. If it is not clear enough feel free to update it.

Patch attached adds non-happy-path testing to ensure that configuration is validated and the user receives the expected messages at the right moment and ensure that the user can't weirdly continue installation.

@alexpott thanks, sounds like a good plan.

I think we need a change record and big issue summary update.

So I tested it a couple of times and there is a slight complication if the settings.php contains the database settings but not the correctly set up $config_directories it will fail. Not a problem, just noting it here. It only works for its narrow use case, other ones can be covered in next steps.
This functionality is purely opt-in for custom profiles.

On the code level the patch uses: $install_state['config_install'] and $install_state['config'] but they are not documented with the default values in install_state_defaults(). Also there is still the todo for documenting. But other than that I think it is really close.

Should this include some extra documentation in default.settings.php?

Ok, So I added the documentation and now it should be good to go.
I don't think we need to change the documentation in default.settings.php but rather in the change notice.

This feature we add here is strictly limited to profiles that have their configuration in its own config/sync folder. (other locations in follow ups.) So the documentation needed for this is rather how to use this feature in the first place.

PS: I didn't mean to remove the tags..
PPS: I don't know why the patch is so much smaller I guess there is something going on with the fixtures. Lets see what the testbot has to say about it.

Btw having config in profile it makes it exposed to docroot, it needs htaccess changes & mention in change record

The patch #65 works fine :

• Patch applied on Drupal core 8.3.3
• drush site-install command
• multilingual
• config entities translations (configuration located in languages subfolder)

Everything is OK!

Do you think apply this patch on version 8.3.x?

+++ b/core/includes/install.core.inc
@@ -1464,6 +1501,12 @@ function install_load_profile(&$install_state) {
+    $install_state['config'] = $install_state['profile_info']['config'];

We should test $install_state['profile_info']['config'] is not empty.

In case no 'config' is defined in profile info, installation works but a notice is displayed in browser:
Notice: Undefined index: config in install_load_profile() (line 1508 of core/includes/install.core.inc).

Tested this a bit and it seemed cool at first. Until I realized that this doesn't make sense for "distribution" profiles where the config/sync for the profile is committed as part of the profile itself, because every site you install using this profile would have the SAME SITE UUID!

Doesn't that defeat the purpose of the site uuid? I mean, it makes sense for the dev/stage/prod environments of a site to have the same uuid, but when building a distro like Lightning, or Atrium where I expect lots of different sites to install using the profile, you wouldn't want them to have the same uuid. Am I missing something here? Maybe my use-case for profiles is different from what this patch addresses.

I was wondering if we could do this:

1. do a drush site-install with minimal profile
2. get the new site UUID
3. get the path of the existing config files either from settings.php or from command line arguments
4. replace the existing site UUID with the new site UUID, using this command grep -rl 'old-site-uuid' /PATH/TO/CONFIG | xargs sed -i 's/old-site-uuid/new-site-uuid/g'
5. import the existing configs, using drush config-import -y

If this workflow works, then we can update "drush site-install" to automate this workflow.

I haven't tested the workflow above. But, this would solve #70

Ok It seems that the fact that this is only for custom profiles is confusing for some.
I thought that we could move on with this and do the improvements I mentioned in a couple of comments above in a follow up.
But this issue is stalled, so attached is a patch which could get things moving again.

We still have to add a test for the new functionality if it passes now. (my git complained about line endings in the tar so I don't know if the patch was successful at all)

Basically the functionality with my patch is:
* A profile can declare that it can be installed from configuration by setting config_install: true in its info file or having a config/sync folder.
* When installing the profile settings.php is checked for having a $config_directories[CONFIG_SYNC_DIRECTORY] set and install the config from there if so.
* If the $config_directories is not set the config/sync is checked and installed from its configuration in case it is there.
* If the config/sync is not there or if $config_directories is not set and the config_install: true is not set in the info file, the profile is installed normally.

This should alleviate the concern for the sync directory in the docroot (#66) and it should be more clear that this also works for distributions (with limitations) (#70)

The config_install: true in the info file is basically a promise by the profile that it "behaves".
For example minimal behaves and standard does not.
The general case to import from configuration may never happen in Drupal 8 because it would "break the api". #1613424: [META] Allow a site to be installed from existing configuration
Because for some definition of API we allow profiles to depend on modules and expect these modules to be installed when running the profiles install hook, but then allow the dependencies to be uninstalled. These two things do not work together and this is why the config_installer project needs to do a whacky dance with installing modules and uninstalling them again.

Attached is the patch with the missing test I mentioned in #72

Added change notice: https://www.drupal.org/node/2897299

I tried testing the following three use-cases with your patch applied:

1. without config_install: true, without config/sync in profile & without sync-dir in settings.php
2. with config_install: true, with config/sync in profile but without sync-dir in settings.php
3. with config_install: true, without config/sync in profile but with sync-dir in settings.php

The cases 1 & 3 are working without a problem.
But case 2 does not work for me with this error:

Exception: The configuration directory type 'sync' does not exist in config_get_config_directory() (line 173 of core/includes/bootstrap.inc).
config_get_config_directory('sync') (Line: 28)
Drupal\Core\Config\FileStorageFactory::getSync()
call_user_func_array(Array, Array) (Line: 254)
Drupal\Component\DependencyInjection\Container->createService(Array, 'config.storage.staging') (Line: 177)
Drupal\Component\DependencyInjection\Container->get('config.storage.sync') (Line: 158)
Drupal::service('config.storage.sync') (Line: 2288)
install_config_import_batch(Array) (Line: 606)
install_run_task(Array, Array) (Line: 555)
install_run_tasks(Array) (Line: 120)
install_drupal(Object) (Line: 44)

When looking at the patch it seems like the expects the presents of a profile_sync as install_load_profile it reads:

if ($install_state['profile_info']['config_install'] && !empty($config_directories[CONFIG_SYNC_DIRECTORY])) {
  $install_state['config_install'] = TRUE;
  $install_state['config']['system.site'] = \Drupal::service('config.storage.sync')->read('system.site');
}
elseif (!empty($install_state['profile_info']['profile_sync'])) {
  // If the profile has a config/sync directory copy the information to the
  // install_state global.
  $install_state['config_install'] = TRUE;
  $install_state['profile_sync'] = $install_state['profile_info']['profile_sync'];

  if (!empty($install_state['profile_info']['config'])) {
    $install_state['config'] = $install_state['profile_info']['config'];
  }
}

I seem to have overlooked, that $install_state['profile_info']['profile_sync'] is being set in install_profile_info.
After realizing that I tested it some more. It seems to only be working if a CONFIG_SYNC_DIRECTORY is specified in settings.php. If it is not, the above error is thrown.

I just realized that I was reusing a settings.php file for the installations ...

After removing that & running a clean installation, it works in either of the cases mentioned above.
I also tried installing the site via drush & drupal-console, which also works.

After that I tried to find out why I got the error and observed the following:
Reusing a settings.php is only a problem when database-credentials are set, but no config-sync directory (Even without config import).
But testing this without the patch works, although I don't know if this is even a use-case officially supported by core.

Ignoring this though, it's working quiet well.

Hi! My use case is for reusable install profiles / distributions, so I'm concerned about the limitation in #70 -- thanks for articulating so well, @mpotter!

What if there are no UUIDs in the config files...? That is, what if I used Features to export the config files (and then moved them into the profile's config/sync dir), so there are no UUIDs? (only used Features to do this export/config file generation, not talking about using features generally) It would be cool if that could work -- i.e. that the destination site UUID is assumed for the config, like when config is imported from a module (or feature...).

Would that be in conflict with the other use cases (the original intended use cases) targeted by this patch, or?

Thanks Alumei for the review.

I tested again the scenarios, and you are right, when the database is set up and you use a custom profile with included config but leave the sync directory undefined then it will not get written to the settings.php because that normally happens when you submit the settings form.

I added a patch that solves this problem. As a side effect the .htaccess file is also always written to it. (RE #66)

RE #79: The limitation in #70 is lifted in the patches #72 onwards and this also works with traditional profiles.

Traditional profiles that want to benefit from this patch should just add config_install: true to their info file and make sure that the required modules can not be un-installed. That means make the real dependencies also dependencies of some core_module and enable the optional modules with some creativity in additional install tasks.

As such the sites configuration in the sync directory needs to contain UUIDs, some profiles that are developed for just one single site can choose to place the sync directory directly into the profile folder and can then be installed from there.

I tested the installation from drupal console with the use-cases Alumei mentioned:
1. without config_install: true, without config/sync in profile & without sync-dir in settings.php
2. with config_install: true, with config/sync in profile but without sync-dir in settings.php
3. with config_install: true, without config/sync in profile but with sync-dir in settings.php

In case 1 and 3, the installation is working but an message is shown:
console.config_delete - The configuration directory type 'sync' does not exist

In case 2, the installation is working but:
1. If configuration is okay, everything is okay
2. If something is wrong with the configuration (e.g.: missing core.extension or some dependencies) then an HTML is dumped describing the error.

@alisonjo2786 I *think* that would work (removing the uuid) but I need to think more about reusable distributions vs profiles. Let's keep this patch for profiles since I think that's the first step needed.

One problem with the current patch is it fails against Lightning because of the inherited profile patch: #1356276: Allow profiles to define a base/parent profile. For my use case (a site profile that uses Lightning as a base) I want to use the config_install: true in my site profile even though it needs to access modules within the Lightning base profile. Also, if we can get this patch into a place where Lightning can use it for down-stream profiles, that could help the adoption of this patch.

I'll see what I can contribute later this week.

Here is a version of the patch that applies to Lightning along with the inherited profile patch.

So in #83, I just moved the $profile_path down to where it is needed for this issue since the inherited profile patch removes that in favor of a service that creates the profile info file. Also removed the initialization of config_install: false and profile_sync: null which shouldn't technically be needed. This allows the patch to apply.

However, trying to actually use this patch on a profile that inherits from Lightning runs into some issues. If in my site I disable one of the lightning modules, such as "Lightning Roles", then export my config, and then try to install a site using that config, it complains that:

Unable to install the Lightning module since it requires the Lightning Roles module.

So this is another one of those issues where "dependencies" in profiles are not true dependencies and modules are allowed to be uninstalled. Using "dependencies" to simply list the default modules that should be installed in a profile always drives me crazy.

Hmm, yeah, failing tests because it looks like we assume the config_install key is always present rather than handling the case where it is empty. Can probably be improved by using !is_empty($install_state['config_install']) or something. Will look into it.

Also, I finally see the comment in #80 about

make sure that the required modules can not be un-installed. That means make the real dependencies also dependencies of some core_module and enable the optional modules with some creativity in additional install tasks.

So I guess the error is "by design" but still need a better way to handle this since in distributions like Lightning is is very common to disable a module from the profile and core actually allows that.

Added the !empty() tests.

I was able to fix the issue about allowing modules in a profile to be uninstalled in #1356276-360: Allow profiles to define a base/parent profile.

While that fix could also be added to this patch, I recommend against it:

1) Without sub-profiles, you can remove a module from your profile by simply removing it from the profile.info.yml file so it will match your exported config and then you won't get this error.

2) We don't want to apply the same fix in two patches because it would make testing both patches even more complicated and we'd then need a 3rd consolidated patch for Lightning to use.

I am recommending that Lightning adopt the patch in this issue (#87) to make it easier for sub-profiles to install themselves from config even though the parent distribution should not itself have a config/sync folder.

Tested the above patches on a client site and seems to be working well so far, so tempted to RTBC but not sure I can since I have contributed to both issues.

Thanks for the info and continued work @mpotter and @bircher -- I'll continue to watch/read/follow.

At first blush, #89 makes sense to me, esp. not having the same fix in two patches.

@mpotter "the above patches" = #87 and #1356276-360: Allow profiles to define a base/parent profile, yes? I will try to make time to test it tomorrow, but hopefully someone else beats me to it :)

@mpotter it seems like you lost a couple of tests on the way, I added them again.
Thanks for making it work with profile inheritance.
Attached is a patch that works with #1356276: Allow profiles to define a base/parent profile (provided you git apply --3way).

I also added all the interdiffs between 80 87 and 91.
Only $install_state['profile_info']['config_install'] needs to be tested for empty the rest is fine.

RE #81: Thanks for testing it! I tested it with drush and it works without a problem so I am not sure the problem is with the patch but maybe rather with console. Yes also drush prints the errors as html, but I guess that could be handled in those tools.

RE #82/#79: no we can not remove uuids. We need uuids for deploying configuration and the plan here is to install a site from existing configuration. Ie you install form the same configuration that you use while developing and deploying the site.

RE #89-1: Yes for profiles that are developed together with their configuration it is easy since you do not need any dependency in the info file at all.

RE #89-2: Yes we should focus on the issue at hand, if we can make both patches work in parallel it is a plus of course!

RE #86: Yes this patch is specifically only for "well-behaved" profiles or distributions. Fixing the fact that you can uninstall a "dependency" is out of scope. For the inheritance it can be solved by removing the parents dependencies in the sub-profile.

Our goal is to allow profiles to be installed from existing configuration defined in the sync folder of the site (distributions that set config_install: true) as well as "site-specific profiles" that are managed without needing to set up settings.php beforehand but contain the sites config in the profile (in {profile}/config/sync). If the profile inheritance can take care of not having the parents dependencies as dependencies then all distributions can be made to be installed from config by adding a site-specific profile that inherits from it for each site instance. Other distributions can just define themselves as "well behaved" and work as they do now. Of course distributions should NOT contain a sites configuration.

I've tested #91.

My scenario:
- Profile without the 'config_install' flag set.
- settings.php updated with the sync folder config pointing to: "profiles/profile_name/config/sync".
- Profile with the config/sync folder empty.

Results:
- Running drush site-install, returned the error (html output):
This import is empty and if applied would delete all of your configuration, so has been rejected.

I think if the profile doesn't have the 'config_install' flag, or the flag is FALSE, the installation should not check by the config.

#72

* A profile can declare that it can be installed from configuration by setting config_install: true in its info file or having a config/sync folder.

In my opinion, the installation by configuration should be arbitrary just based on the 'config_install' info, otherwise it may confuse things, whether or not add the 'config_install' info to my profile.

I have made a small change to cover the scenario pointed on #92 to be possible to store the config inside the profile folder (config/sync) and to don't have Drupal being installed using the config folder if the flag "config_install" is not set or if it is set to FALSE.

Please, let me know if that change make sense.

The error you see is because your profile has an empty config/sync folder. It is meant to fail.
After all this feature is meant for site-specific profiles that have already been installed once and need to be re-installed.
Ie when you create the profile the config/sync folder does not exist and you install the profile like a normal profile is installed now, then you export the config to the profiles config/sync folder and from now on you install that site. (not a new site any more like the normal installer without this patch does).

But I can get on board with always requiring config-installable profiles having to have the config_install flag. I will very likely never use the implicit feature anyway and keep the configuration out of the webroot. But there is an argument to be made to be able to infer it without having to set up settings.php before the installation so I am also happy to keep the config/sync folder option around.

We will likely have to update the tests though and add the config_install flag to the config install test profiles.

@bircher, I agree with you in that point, the profile config/sync folder option is a good thing to have, but again, only when the flag is true.

I'm fixing the tests for the last patch.

Thanks.

Trying to fix the tests for #93

setting back to needs-review.
Though if we decide to always have the flag to allow installing from config we need to update the change record to reflect that.

I've used this patch on several projects now and it really helps, especially early in the project with multiple developers re-installing the site all the site from config. What do we think is still needed for RTBC? Would really love to see this in 8.5.x.

I switched over my 8.3.x installation profile to use this functionality and now I get this error when the site is being built via "drush site-install":

Error: Call to a member function getCacheTags() on null in /home/vagrant/docroot/web/core/modules/shortcut/src/Entity/Shortcut.php on line 159 #0 /home/vagrant/docroot/web/core/modules/shortcut/src/Entity/Shortcut.php(100): Drupal\shortcut\Entity\Shortcut->getCacheTagsToInvalidate()

So it turns out that you get the above error if you forget to remove standard_install() from the new installation profile's hook_install() X-)

Once I removed those lines it works fine!

We did a bunch of effort to reduce globals, would it be possible to remove the global variable from this patch? That's my primary concern, generally a +1 for this.

What is the multilingual.tar.gz file in here for? I see very little mention to it in this issue comments.

I'm not a huge fan of the additional functionality being added by #72. Not that I think that we should ignore the use-case that is provided by the changes there I just think it is a question of scope. Patches before #72 were focussed on getting config install to work in the case where a profile provided configuration. After #72 the scope has increased to include the ability for a developer to set a flag and point to any old configuration directory. If we do this in two steps that it is much easier to add comprehensive test coverage of each ability.

The argument that #72 solves the problem for distributions does not make much sense to me. Because the big issue here is that you can't put a distribution on drupal.org with the config_install: true in the info.yml because there's no way to set the config directory in a settings.php. Therefore all this setting appears to be for is for custom distributions were you want to store configuration in some arbitrary location. I am all for solving this problem but as a follow-up.

What this patch desperately needs edge-case testing and improved robustness when the configuration is bogus.

@joelpittet the problem is that the bunch of effort to remove globals did not touch the installer. $install_state and $config_directories come from there and this patch should not attempt to rewrite the installer.

@joelpittet the multilingual.tar.gz is an export of configuration of a multilingual site so we can test installing a multilingual site from config. This is a really complex thing because of how the installer deals with multilingual-ness and therefore requires testing. For example, see how the keep_english thing works.

Just so we have it, here's #69 rerolled.

Well, since the distribution packaging on drupal.org doesn't really work these days, most distributions are hosted elsewhere. For me, the additions in #72 are what made this patch super interesting and useful for my projects. Just don't include the "config_install" in your info file and it works just like before.

But I understand the idea of keeping this simpler and focused. As long as the functionality in #72 can still be added easily later, I'm all for whatever path gets this functionality into core asap. Using this patch was the final piece of the puzzle for us in removing our need for Features. Being able to properly clone sites from custom profiles and supporting inherited base profiles like Lightning has greatly simplified our workflow.

Our standard workflow for installing a site on a new environment is to use the --config-dir option with drush site-install. Starting with Drupal core 8.4.x this is no longer possible because Drush8 doesn't support Drupal 8.4.x and Drush9 doesn't support using --config-dir during install with anything other than the minimal profile. Using the --config-dir option when installing is also problematic because some config will end up with conflicting UUIDs.

This issue would solve all of those problems. But only if the patch provides the ability to load the config from the $config_directories[CONFIG_SYNC_DIRECTORY] directory. (Current users don't want to have to move their site's config into [profile]/config/sync directory - and for some that have their profile in separate VCS, this isn't an option)

Considering #103, I think we have two options:

1. Move forward with the reroll in #105 and immediately start work on a followup to add the functionality in #72
2. Press to include the the config_install: true + $config_directories[CONFIG_SYNC_DIRECTORY] functionality here

My vote is certainly with #2 (because one patch is better than two!). I'm not suggesting that the urgency created by 8.4.x/drush9 should sway the decision, but I'd like to see how we can help push this along.

@alexpott, your comments in #94 indicate that maybe all we need are updated tests to get this RTBC. Do your comments in #103 supersede that? Or is it still up for debate?

I discussed this patch and several related issues with @bircher at Drupalcon. If I've understood correctly, the purpose of the boolean in the profile was to indicate that install profile plays nice with being installed by through configuration. There are two concerns that @bircher stated:

• If the core.extension doesn't have modules installed that listed as dependencies in the incoming core.extension
• If the install profile does crazy things in hook_install

An example of this type of problem is the standard profile. It's quite common to disable the contact module but standard_install assumes the contact.site_page menu link exists.

These are among the reasons I prefer the adding a config/sync to the install profile. Because it is very clear that the profile is going to be compatible with installing from config. Once any profile can be installed from configuration by just by specifying a path to any old configuration then this problem space gets much much bigger.

I think it is worth adding test coverage to ensure that the current code can cope with the standard / contact situation. I think what should happen is that when the install profile is installed it will install any dependencies not listed in core.extension.yml and then the install_config_fix_profile step will fire and fix up everything to match with the config sync directory.

An example of this type of problem is the standard profile. It's quite common to disable the contact module but standard_install assumes the contact.site_page menu link exists.

Doesn't the fact that Standard doesn't have config_install set to true actually preclude it from even being used in this way? I would only ever see this being used by a Custom profile - one that the developer has control over in their git repo.

Of course, those users could also move their config into their profile, but that would mean putting their config inside docroot (and changing their setting.php file), which is why I like the flag in the info file.

@balsama the point is that the config_install:TRUE thing isn't really the problem here and it feels like an addition that limits possible future system improvements. We want to make installing from config simple. This patch currently adds the ability to install when a profile supplies a config/sync directory. In this instance no core profile is going to use it and it is also not suitable for distributions because they are either products or starter kits. This patch is about the install profile being used to build a single site. Which is a super common use-case - perhaps the most common. If the outside of docroot thing is important then really all code not just config should be important. The core provided .htaccess file prevents access to all .yml so really serving these files is a misconfigured web server where you have a much bigger problem. Ie. your private files are probably not protected either. The next patch will add the ability to install from config if a user has set up the sync directory in settings.php. The patch after that will add a UI to make it easy for everyone.

Ok, thanks. That makes sense.

I'm coming at this from the perspective of teams that use a custom profile per project. Up until now, they have been able to use that custom profile + `drush si --config-dir` to install their site with the latest config. The patch in #91 pretty much allows them to continue with this same workflow. But, regardless, I guess that's not the intention/scope of this issue. So we'll need to work something else out.

@balsama

Up until now, they have been able to use that custom profile + `drush si --config-dir`

That's not at all going to be changed by this patch. Once we finish the issue that will allow us to use the config importer with whatever is set in the settings.php then drush's --config-dir option can just be changed to use this instead of doing a full install and then a config import.

Right, using Drush to install a site with config isn't going to be changed by this patch, but we were hoping we could use this patch to work around the fact that Drush 9 no longer supports installing a site with config (unless it's the Minimal profile).

See Impossible to site-install --config-dir with any profile other than minimal.

I've just run the following test with the patch applied:

1. Install standard
2. Export the configuration to core/profiles/standard/config/sync
3. Re-install using the standard profile

This crashes because of the Shortcut::create() calls in standard_install(). This is because these shortcuts expect the default shortcut set to be created at this point. It doesn't because the shortcut set is configuration entity and as such during a config import would be created after all the modules and the install profile have been installed.

I think we need to

• Add documentation to hook_install() to say that it is not good to write code that depends on the existence of configuration entities that might not exist during a configuration import. This should be a separate issue.
• Enforce that profiles do not implement a hook_install() if they are going to be used with config installation. This is because whilst it would be a bug against a module or theme - because it can not be installed during a config import - the ability to install install profiles via config import is what is being added here.

Oh yeah. There's an issue for that bug in Standard: #2583113: Config import from a different site fails because of shortcut.set.default.yml

I think that profiles that are going to be used with config install can still implement hook_install, they just need to bail out of that hook if a config sync is taking place. Lightning does this in all of it's install hooks: https://github.com/acquia/lightning/blob/8.x-2.x/lightning.install#L27

This crashes because of the Shortcut::create() calls in standard_install(). This is because these shortcuts expect the default shortcut set to be created at this point. It doesn't because the shortcut set is configuration entity and as such during a config import would be created after all the modules and the install profile have been installed.

One way to solve this is either add functionality like #2901418: Add hook_post_config_import_NAME after config import after module installation or implement hook_profiles_installed like hook_modules_installed but \Drupal::isConfigSyncing() can be TRUE during the execution of hook_modules_installed.

Add documentation to hook_install() to say that it is not good to write code that depends on the existence of configuration entities that might not exist during a configuration import. This should be a separate issue.

This is also discussed in #2762235-44: Ensure that ConfigImport is taking place without outstanding updates.5 we don't have a dedicated issue for this but we do need one.

Created #2914213: Don't create content in install hooks if module is installed during config import.

This crashes because of the Shortcut::create() calls in standard_install(). This is because these shortcuts expect the default shortcut set to be created at this point. It doesn't because the shortcut set is configuration entity and as such during a config import would be created after all the modules and the install profile have been installed.

This interdiff solve this issue.

But it would only work as long as the 'default' shortcut-set is still part of the config that is beeing imported, or is it enforced that that short-cut set can not be deleted?

So the solution would popably be the

Add documentation to hook_install() to say that it is not good to write code that depends on the existence of configuration entities that might not exist during a configuration import. This should be a separate issue.

And maybe even add a seperate 'config setup'-step that is called after default-config-setup / config-import is done respectively & suggest the developer to check against the existence of the defaul config?

But it would only work as long as the 'default' shortcut-set is still part of the config that is beeing imported

True

or is it enforced that that short-cut set can not be deleted?

Nope, it is a config entity so it can be deleted until we lock it just like forum_install locks forum node type.

And maybe even add a seperate 'config setup'-step that is called after default-config-setup / config-import is done respectively & suggest the developer to check against the existence of the defaul config?

@Alumei That has already come up for update hooks in #2901418: Add hook_post_config_import_NAME after config import. Maybe we need a hook after module install as well.

Well after I saw the patch in #2914213: Don't create content in install hooks if module is installed during config import the code seemed needlessly redundant so my idea was that instead of having two places to put the code hook_install and hook_post_config_import_NAME / that event subscriber in the other issue it would be better to have a single point like hook_after_config_setup to consolidate both use-cases.
It didn't look to me that #2901418: Add hook_post_config_import_NAME after config import was aiming at something along thes lines, but maybe I overlooked something.

The problem is not really just about shortcuts it's about doing anything with configuration during a hook_install() - there are lots of other situations where things can go wrong. So coming up with a fix for standard / the whole of core isn't really going to fly cause contrib and custom install profiles can have the same problems with other things.

Add documentation to hook_install() to say that it is not good to write code that depends on the existence of configuration entities that might not exist during a configuration import. This should be a separate issue.

Created #2914213: Don't create content in install hooks if module is installed during config import

Issue #2914213 in someway from being a documentation task became a new feature. There was already an attempt to document this on #2906107: hook_install() is invoked before the module's default config is installed during config import , which is closed in favour of #2901418: Add hook_post_config_import_NAME after config import so that when saying "that it is not good" we can also say "what's the preferred way". But we can opening it back if there is an immediate urgency.

I've created #2914379: Remove minimal_install(), ship with default configuration instead that will make minimal compatible with a config install that doesn't allow install profiles to have a hook_install(). I think this is the only solution for robustness in Drupal 8. Future Drupal's might do something different.

Hey there! This feature sounds quite promising, love the idea :-)

Here's a first review of the parts I was able to understand with my current / limited knowledge of the D8 config/install systems:

1. +++ b/core/includes/install.core.inc
   @@ -188,6 +191,10 @@ function install_state_defaults() {
   +    // The path to the configuration to install when installing from config.
   +    'config_install' => NULL,
   

   If this variable holds a path, can we have "path" in its name?

2. +++ b/core/includes/install.core.inc
   @@ -445,10 +452,16 @@ function install_begin_request($class_loader, &$install_state) {
   +  if (!empty($install_state['config_install']) && $install_state['config']['system.site']) {
   

   The first condition technically doesn't need the empty() because the key is initialized with a default value (NULL).

   However, the second condition needs an empty(), because the existence of the system.site config is an assumption.

   --

   That said: This super-early initialization in install_begin_request() exists for real distributions only. — As soon as one installation profile declares itself as a distribution, it takes over the whole Drupal installer right from the very first page, including the language selection, before the user is able to select a profile.

   Therefore, this change here should be reverted, because this early interception is reserved for distribution profiles.

   Or alternatively, you may move your new condition into an inner condition of a wrapping check for ['profile_info']['distribution'] parameters. If there are any distribution parameters AND if the profile defines a config install path, then we can respect that configuration (if any).

   Not sure where that leaves us with a possibly predefined 'langcode' parameter in profile's .info.yml file.

3. +++ b/core/includes/install.core.inc
   @@ -445,10 +452,16 @@ function install_begin_request($class_loader, &$install_state) {
   +  else {
   +    // Otherwise, Use the language from the profile configuration, if available,
   +    // to override the language previously set in the parameters.
   +    if (isset($install_state['profile_info']['distribution']['langcode'])) {
   

   Should be an elseif

4. +++ b/core/includes/install.core.inc
   @@ -2214,3 +2259,192 @@ function install_write_profile($install_state) {
   +/**
   + * Creates a batch for the config importer to process.
   + *
   + * @see install_tasks()
   + */
   +function install_config_import_batch() {
   ...
   +function install_config_import_batch_process(ConfigImporter $config_importer, $sync_step, &$context) {
   ...
   +function install_config_import_batch_finish($success, $results, $operations) {
   ...
   +function install_config_download_translations(&$install_state) {
   ...
   +function install_config_fix_profile() {
   

   Shouldn't these callbacks live together in peace on… a class or similar?

5. +++ b/core/includes/install.inc
   @@ -483,12 +484,20 @@ function _drupal_rewrite_settings_dump_one(\stdClass $variable, $prefix = '', $s
   +      // Install profiles can contain a config sync directory. If they do,
   +      // 'config_install' is a path to the directory.
   +      $config_directories[CONFIG_SYNC_DIRECTORY] = $install_state['config_install'];
   +    }
        $settings['config_directories'][CONFIG_SYNC_DIRECTORY] = (object) [
          'value' => $config_directories[CONFIG_SYNC_DIRECTORY],
   

   Why would we want to save the config install path of the installation profile as the sync directory for the newly installed site?

   I can see how that might make sense for developers working on their own distributions… But won't custom sites have custom configuration, regardless of whether they were installed from a profile's config?

   If we associate more than an initial set of override configuration, and/or a particular workflow, with the config_install feature proposed here, then we should find a better name than "config_install" for it, because that name would be insufficient and misleading. (Just because many things in Drupal are named misleadingly we shouldn't introduce new cases. :-))

6. +++ b/core/lib/Drupal/Core/Installer/Form/SiteConfigureForm.php
   @@ -145,12 +146,14 @@ public function buildForm(array $form, FormStateInterface $form_state) {
          '#title' => $this->t('Site information'),
   +      '#access' => empty($install_state['config_install']),
   

   The mere existence of any kind of configuration does not necessarily mean that there is configuration for the basic site settings, or does it?

Just want to draw folks' attention to this issue which might well be related: #2926633: Provide a script to install a Drupal testsite

Thanks for the review @sun.

1. Sure fixed.
2. I disagree - if you are installing from configuration then it needs to the canonical source - it'll decide the install profile used anyway so the distribution part is irrelevant at this point.
3. Fixed
4. I'd consider that to be part of a general rewrite of the installer rather than done piecemeal here.
5. Well the purpose of this patch is to provide a way to install from configuration for sites that are using an install profile to manage themselves. The use-case where the sync directory would not be the location you installed configuration from is not really supported by this patch. And note if you have it set to something else already it is not overwritten.
6. Yes it does. You can't install a site without configuring it - we only support installing from a full set.

Still need to produce an install error if the install profile being used implements a hook_install().

Added a check to prevent installing from config when the profile has a hook_install() so we can avoid all the problems when that is the case.

So as I understand it, right now, using installation profiles does not work in Drupal 8. You are not able to place a Profile in/profiles and run the installation through the browser. Correct?

The project I'm currently working on requires that the client be able to install a Profile with prepared configs and a hook_install that will set all of the themes appropriately. So far, it's not working at all.

@tk421j: Incorrect. "Traditional" install profiles works jut fine in D8. That is not what this issues is about.

@tk421jag: You might want to check out the base/parent profile issue https://www.drupal.org/project/drupal/issues/1356276

Thanks for the direction guys.

@tk421jag: I appreciate you are frustrated but can I ask you to please stop raising general problems you are having with installation profiles on Drupal 8 on this particular issue? The purpose of this specific issue is to add a new capability to Install Profiles in Drupal core and raising unrelated queries here generates noise and makes it harder to track the progress of what people are trying to achieve here.

https://www.drupal.org/support lists many ways to try to get support for core Drupal issues. Good luck, I hope you find a way forward!

Re #129 and #120. I can understand why we don't want to call hook_install() while config is being imported. However, there *are* things a profile would like to do *after* the config has been imported.

Maybe #2924549: Invoke hook after a site install is complete is the solution for this. A post_install hook where the profile using this "config installer" method could perform cleanup or further customization based on the imported config.

Just wondering where to move the "non config" code from hook_install() since now #129 is going to fail the installer if this hook exists.

@mpotter good question and we have a really good answer :)

Have an event subscriber and subscriber to \Drupal\Core\Config\ConfigEvents::IMPORT and check drupal_installation_attempted() I think. We should add a test for this because I think it could solve quite a things.

IMHO another way would be within a custom installation task. There it would automatically be situated after the complete config import has finished.

If you find yourself wanting a hook, the answer is to broadcast an event.

If we had a more-or-less encapsulated installation process used by core and drush and drupalconsole and the test system, this wouldn't be hard to implement, and profiles (and other extensions) could then respond with whatever they need to do.

#2926633: Provide a script to install a Drupal testsite has a lot of promise in this regard.

A solution that has worked on a lot of multisite projects I've worked on needing to install site profiles from config has been to use alternative cache backends to override core's default service container backend when installing drupal since it uses the database by deafult during installs / bootstraps. I believe this is needed when the service definition must be refreshed during site install, when default configuration such as site language must be overridden at runtime, and to resolve install errors caused by cache race conditions, I've had to use this approach on multisite applications using the content_translation module since the service container contains negotiation methods that override the locked default language on site install.

Errors such as Shortcut::create() calls in standard_install() which are caused by conflicts between config entities and assumptions in which the order modules are installed can also be resolved by this method. A proposed update to the memcache mocule readme has the needed configuration to insert in settings.php: https://www.drupal.org/project/memcache/issues/2934916

What Lindsey proposes is a useful workaround in the short-term, and might also be a necessary problem to tackle long-term, but I don't think it should be viewed as a full alternative to this issue (installing profiles from existing config). Both problems should continue to be tackled in parallel.

The reason being that while the caching fix is likely to help with some of the immediate problems related to conflicts during the initial config import after install, it doesn't address the more fundamental problem that it's silly to be running install tasks at all when you're just going to blow away all of the config they create with a full config import.

Best case, normal install tasks are a waste of time if you already have a full config export. Worst case, they create unimaginable horrors with UUIDs and such. It sounds like the cache solution, while it might still be generally worth solving, only addresses the second issue.

Given the turn in direction of this ticket (no longer allowing installs from site config) , and the concerns specifically raised by balsama in #107 and subsequent comments, it seems like we need to have a larger discussion about how configuration can be managed in a real-world scenario. I've tried to capture this in #2939544: Sites can't be re-installed using core configuration management and would encourage more discussion there.

@Dane Powell there's already a follow up to allow install from an arbitrary full config export - see #1613424: [META] Allow a site to be installed from existing configuration. This issue is going to get the plumbing in to do that whilst solving a more limited use-case. That one can concentrate of the complexities of profile hook_install and any profile and config coming from anywhere.

nit picks

1. US spelling of "synchronisation" is "synchronization"
2. Capitalize UUID in comments. ("Match up the site UUIDs")
3. $profile_path . "/$profile.info.yml" is already using variable expansion inside a string, we might as well use it for both variables, or do more complex string concatenation for both.

IIUC from #72 to #104 the scope increased; in #105 the scope returned to it's original with f/u work in #1613424: [META] Allow a site to be installed from existing configuration.

Since we were using #97 (part of the increased scope), I tested the latest patch locally and it still works for our use-case (@jribeiro + @dixon_).

Resolved the conflict. Thanks for the fixes... not the first time :) #2388925: British again invade config sync Still need that githook.

@douggreen do you feel in a position to review this patch with a view to rtbc considering you are using this patch in production?

Updated issue summary and change record to describe the more limited scope of this patch.

@alexpott, sure, I've done mostly a code review, but I'll do a more thorough test and review tomorrow, with that goal in mind.

How do I use this patch the right way?

1. I create a new Drupal project and create a profile.
2. The initial configuration is placed in profile/config/install?
3. I install the site, and export configuration to profile/config/sync?

Once the site has been installed initially and the profile's configuration is in the sync folder (say Text editor, formats etc). Do I remove that configuration from config/install since it already exists in sync? (I think it should stay, right?) Or do I make sure they exists in both places?

@johndevman once a profile has configuration in config/sync it will install from that full config export and totally ignore any configuration in config/install. For me the best way to use this patch is to:

1. Start your project from minimal or standard (you need an installed Drupal)
2. Use https://www.drupal.org/project/install_profile_generator to create yourself a new profile for your project - this will export to your new profile's config/sync directory.
3. Start configuring and exporting and committing your new profile with it's config/sync directory
4. Then any one getting your code can install your profile and start working on your site. And doing step 3.

The profile you generate will never have any configuration in config/install

@alexpott Thanks for the explanation. I applied the patch and followed your suggested instructions and it worked. I also used another language than English per default which worked as excepted as well.

Since I replied that I'd try get it to RTBC, I wanted to say something now. This is not ready for us. We are currently running #97.

We ran into some problems trying this patch on all our sites. I haven't tracked it down what happened. We know partially that the empty sync directory threw an error. We can fix that on our sites easily enough. But something else also happened and we don't know what yet.

Since this is slated for 8.6.x and not 8.5.x, there's no hurry. I should be able to get this into our deploys in the next few weeks to month.

Another issue I've noticed is if a new dependency is added to a module the dependency will not be enabled because it's not in core.extension.yml

@timmillwood well that's because you've updated your code without updating you configuration. That's not the fault of this approach and that situation is generally true of any full site config export. That would be solved by #2628144: Ensure that ConfigImport is taking place against the same code base.

I found an issue in the contentacms issue queue, that is related to the patch here.

While investigating the issue, I found out, that the patch applies cleanly when using the patch -p1 < *.patch command. When using git apply it fails with

git apply 2788777-3-146.patch
2788777-3-146.patch:830: trailing whitespace.
1e?

rBkAҮ!???Q?ȍ,?RE"??CW>?:#?L???듿> ol"??m???
error: patch failed: core/includes/install.core.inc:458
error: core/includes/install.core.inc: patch does not apply
?>

in my mac environment using git version 2.13.0.

Basically a side effect when appling the patch with git is, that it creates weird subfolders in the core folder like web/core/b/core/modules/system/src/Tests/Installer. That is not happening with the patch command.

We're seeing the issue in contentacms as the patch is applied via cweagans/composer-patches and that is using git apply internally. I experienced this same behavior with patch #105 and #146. The issue is not contentacms related and also happens when I use the composer-patches with drupal-composer/drupal-project

Has anyone experienced the same issue?

@szeidler:

I get the following issue:

$ git apply -3 2788777-3-146.patch 
2788777-3-146.patch:830: trailing whitespace.
1e��>rBkAҮ!���TQ�ȍ,�RE"��CW>�:#�L��Q�듿>	ol"���m���
warning: 1 line adds whitespace errors.

This is related to the binary files in the patch, but it still applies cleanly. Are you applying the patch against 8.6.x?

I'm also not seeing the core/b/core issue.

I need to backpedal a bit. Thanks, you're right @timmillwood, applying to 8.6.x seems to work fine. The problem only happens with composer-patches for me now. Then I was confusing my earlier test.

Then it's just a notice for those, that are using the patch on a composer based workflow like contentacms, but doesn't affect the general progress on this core issue.

Our Drupal 8 projects are based off https://github.com/drupal-composer/drupal-project and we have been using config_installer.

We like to keep the configuration in the root directory of the project, or in other words in $config_directories["sync"] = "../config/sync";
and the Config Split configuration in config/dev etc.

We also have an installation profile nuvole_profile with configuration we commonly use (think of it as a customized standard). I mention it here because while this issue promotes a very specific use of profiles I am not too excited about, we use it in a broader way with our current workflow and so I feel comfortable to RTBC this.

The important part of the projects composer.json part looks like this:

  "require": {
    "drupal/core": "8.5.1",
    "nuvoleweb/nuvole_profile": "*",
    ...
  }
  "extra": {
    "patches": {
      "drupal/core": {
        "Config Installer": "https://www.drupal.org/files/issues/2788777-3-146.patch",
        ...
      },
      ...
    }
  }

In our build script we create a symlink of the config/sync to the profiles config/sync as this issue requires.

Here is the simplified extract of our gitlab-ci test output: (we use docker4drupal containers)

$ docker-compose exec -T --user 82 php composer install
...
$ docker-compose exec -T --user 82 php ./vendor/bin/robo project:setup-ci
 [Filesystem\FilesystemStack] symlink ["../../../../config/sync","./web/profiles/contrib/nuvole_profile/config/sync"]
$ docker-compose exec -T --user 82 php ./vendor/bin/robo project:install
 [Exec] Running ./vendor/bin/drush site-install nuvole_profile '--root=/var/www/html/web' '--db-url=mysql://drupal:drupal@mariadb:3306/drupal' '--site-name=Site name' '--site-mail=***' '--account-mail=***' '--account-name=***' '--account-pass=***' -y

 // You are about to create a sites/default/settings.php file and DROP all      
 // tables in your 'drupal' database. Do you want to continue?: yes.            

 [notice] Starting Drupal installation. This takes a while.
 [success] Installation complete.

Then the behat test pass and this patch is RTBC.

Maybe we can mention in the change record that all one has to do to make existing workflows based on config_installer continue to work with this patch is to remove hook_install and set a symlink of the config directory to the install profile. This is only needed for the installation (ie for CI or new developers) so people uncomfortable with the config directory in the webroot do not need to worry.

@bircher Nice idea and very helpful

Tagging for potential CMI 2.0, see #2957423: Proposal: Configuration Management 2.0 initiative.

@bircher great idea, I'm testing a slightly different version of that workflow. Basically, just create a custom profile (call it 'foo') that's a subprofile of whatever contrib profile you want to use (with patch from #1356276: Allow profiles to define a base/parent profile), then create a symlink from /docroot/profiles/custom/foo/config/sync to /config/default. This way there's no CI / symlink rewriting involved, and using subprofiles is the recommended method for some profiles such as Lightning.

The only problem is that the patch in #146 above conflicts with the 8.5.x patch in #1356276: Allow profiles to define a base/parent profile. So I rewrote #146 so it doesn't conflict. It shouldn't be functionally different, it's just less optimized / refactored, so it should not be patch that ultimately gets committed.

Re-uploading the rtbc patch. @Dane Powell please don't upload non-rtbc patches after an rtbc patch because that confuses the automatic rtbc retests and committers.