Define config directory in settings

This is a good cleanup IMO.

Just going to throw this out there for those who use the platform, but in my experience Acquia Cloud Site Factory no longer provides a mechanism to detect what the installed site profile is at the settings.php file at run time. If this change was added I'm pretty sure this would hose a lot of ACSF subscriptions.

EDIT: I am in no way implying that Drupal should be built around ACSF ;)

Here's a attempt - I duplicated this issue at #3018015: Move the $config_directories to $settings so it can be accessed via the Settings singleton

So #5 moves the $config_directories global to $settings['config_directories']. If we are going to do this I think it is also worth discussing whether or not we make it $settings['config_sync_directory'} and stop supporting multiple directories in core. I think this could make sense but it would be great to have more opinions. One advantage of the current situation is that you can write code that deals with config and it can scan all the config_directories and provide generic functionality. If we make it a single value and not an array then we lose that functionality.

Updated the IS to reflect the 2 solutions and the choice we should be making on this issue.

Discussed the multiple config directories vs the single configuration sync directory with @bircher in Slack. Here's our discussion:


bircher [11:07 AM]

@alexpott I think we should not have it be an array. core uses one sync storage, and should use a setting for that, I am not against other workflows with other storages, but then core should also support that, in my opinion it is a disservice to users and developers to think that there can be more than one when you can't.

alexpott [11:09 AM]

@bircher yeah but… the array does mean as a contrib maintainer you can write generic modules that can deal with whatever storages are defined their. Removing the array means we no longer support that.

bircher [11:17 AM]

hmm, yea I think there are some modules that do that now, I don't really know what for though
@alexpott the only thing I can think of is multisite/site-factories where you want to maintain several sites with one code base and easily switch which site is active in your environment

and for that you can just put some logic around how you set the settings

alexpott [11:22 AM]

@bircher I’m on the fence. I’m not sure I care too much :slightly_smiling_face: but I think if we move to a single value we need to make the decision knowing what contrib / custom flows we would affect.

bircher [11:29 AM]

@alexpott that is true. but if we want to give a standardised way to deal with multiple config directories then we should also have a service for them.. otherwise we might as well not have it.. the way we have it now feels like it is not really done on purpose and more just happened by accident

Here's a patch that only supports the sync directory and moves it to $settings['config_sync_directory']

I support the switch to one directory. Its much clearer for the common use case.

@andypost yep but I don't think a general refactor of Settings and how we do that means we should not try to centralise our current globals on it. Once everything is in Settings is becomes easier to consider how to fix all the global application settings in one go.

Here's some more work. We can deprecate some other methods that don't make much sense.

@alexpott yep, totally agree - iterative approach makes the whole picture clear

I've reverted some changes to deprecated functions because if a function is deprecated then there is no point taking the risk to update the code. Also updated default.settings.php. This shows us that some people were at one point very very keen to keep active configuration in the filesystem. With this change that probably comes trickier but CONFIG_ACTIVE_DIRECTORY has been deprecated for a very long time.

We can do a slightly better deprecation in Settings so it has less unrelated impact.

Here is a reroll against latest 8.7.x. My Mac interdiff utility gives an error when trying to create it but I can't pin down why. But there were some changed in tests so #28 failed to apply. Also, calls to `file_prepare_directory()` were removed in core and replaced with `\Drupal::service('file_system')->prepareDirectory()` so I made that change to the new tests also.

1. +++ b/core/lib/Drupal/Core/Installer/Form/SelectProfileForm.php
   @@ -134,10 +135,11 @@ public function buildForm(array $form, FormStateInterface $form_state, $install_
   +      // @todo inject and handle case where config_sync_directory is not set.
   +      $sync = new FileStorage(Settings::get('config_sync_directory'));
   

   Not sure about this @todo anymore either. Not sure why we'd both injecting anything here. So early in the installer. I guess the current HEAD behaviour here is that we would throw an exception if the directory was not configured. I guess we should continue to do that.

2. +++ b/core/modules/config/src/Form/ConfigImportForm.php
   @@ -52,7 +64,8 @@ public function getFormId() {
   -    $directory = config_get_config_directory(CONFIG_SYNC_DIRECTORY);
   +    // @todo what about the unset case?
   +    $directory = $this->settings->get('config_sync_directory');
   

   I guess the thing is before we would have thrown an exception if config_get_config_directory() was unable to determine a directory. Now we don't. Which makes things a bit more fragile because we're not throwing an early error.

1. +++ b/core/globals.api.php
   @@ -62,7 +62,11 @@
     * @see drupal_install_config_directories()
   

   Do we still need to reference drupal_install_config_directories() since it's being deprecated and the issue reference should cover it?

2. +++ b/core/includes/bootstrap.inc
   @@ -137,6 +138,10 @@
   + * @see https://www.drupal.org/node/2501187
   

   Is this the correct reference? Others here are for https://www.drupal.org/node/3018145

3. +++ b/core/includes/bootstrap.inc
   @@ -137,6 +138,10 @@
     * @see config_get_config_directory()
   

   Do we still need to reference config_get_config_directory() here?

4. +++ b/core/includes/install.core.inc
   @@ -380,7 +380,7 @@ function install_begin_request($class_loader, &$install_state) {
   +    $sync_directory = Settings::get('config_sync_directory', FALSE);
   

   We reference 'config_sync_directory' a LOT now. Should this be a new constant? (We can't reuse CONFIG_SYNC_DIRECTORY though).

5. +++ b/core/lib/Drupal/Core/Config/FileStorageFactory.php
   @@ -23,9 +25,18 @@ public static function getActive() {
   +   *   In case the sync directory does not exist or is not set up.
   

   What do we mean by "or is not set up"? Could change to "In case the sync directory does not exist or is not defined in $settings['config_sync_directory']"

6. +++ b/core/lib/Drupal/Core/Config/FileStorageFactory.php
   @@ -23,9 +25,18 @@ public static function getActive() {
   +      throw new \Exception("The sync configuration directory does not exist or is not set up.");
   

   See above comment for improved exception message.

7. +++ b/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php
   @@ -225,12 +227,36 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +    // If settings.php does not contain a config sync directory name we need to
   +    // configure one.
   +    if (empty(Settings::get('config_sync_directory'))) {
   ...
   +    }
   

   Previously we encapsulated this code into the drupal_install_config_directories() function. With that function deprecated, I still wonder about splitting this code into a helper function for readability. Perhaps a protected drupal_install_config_directory().

8. +++ b/core/lib/Drupal/Core/Site/Settings.php
   @@ -181,4 +191,11 @@ public static function getApcuPrefix($identifier, $root, $site_path = '') {
   +  /**
   +   * Destroys the settings singleton instance.
   +   */
   +  public static function destroy() {
   +    self::$instance = NULL;
   +  }
   +
   

   Is this related to the config_sync_directory change or some other bug fix?

9. +++ b/core/modules/system/system.install
   @@ -423,17 +423,19 @@ function system_requirements($phase) {
   +  // Find the site path. Kernel service is not always available at this point,
   +  // but is preferred, when available.
   +  if (\Drupal::hasService('kernel')) {
   +    $site_path = \Drupal::service('site.path');
   +  }
   +  else {
   +    $site_path = DrupalKernel::findSitePath(Request::createFromGlobals());
   +  }
   +
   

   This doesn't seem related to this issue. The check for $phase of "runtime" around this was removed and not sure how that's related to the config_sync_directory.

10. +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerCustomConfigDirectoryCreateTest.php
    @@ -0,0 +1,44 @@
    + * Tests the installer when a custom config_directory set up but does not exist.
    

    This is the same comment as for InstallerConfigDirectorySetNoDirectoryTest.php, so one of the comments is probably incorrect.

11. +++ b/core/tests/Drupal/KernelTests/Core/Config/FileStorageFactoryTest.php
    @@ -0,0 +1,45 @@
    +      $this->assertEquals("The sync configuration directory does not exist or is not set up.", $exception->getMessage());
    

    If changing the exception message above, change it here too.

1. +++ b/core/globals.api.php
   @@ -62,7 +62,10 @@
   + * @deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use
   + *   \Drupal\Core\Site\Settings::get('config_sync_directory') instead.
   

   We need to update all the deprecations to 8.8.0 - and lets use the new format.
   @deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use...

   There's more than just this one... also the @trigger_error()'s use the same format.

Updated patch with an interdiff.

Most of this looks really good already, I found some nitpicks but haven't read everything yet.

1. +++ b/core/includes/install.inc
   @@ -479,10 +477,13 @@ function _drupal_rewrite_settings_dump_one(\stdClass $variable, $prefix = '', $s
   + * @deprecated in drupal:8.8.0 and is removed from drupal:9.0.0.
   + *   There is no replacement.
   

   I did not know this, but we have a precedent of doing this.

2. +++ b/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php
   @@ -241,4 +257,26 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +  private function createRandomConfigDirectory() {
   

   This should probably be protected instead of private?

3. +++ b/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php
   @@ -241,4 +257,26 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +      // Put a README.txt into the sync config directory. This is required so that
   +      // they can later be added to git. Since this directory is auto-created, we
   +      // have to write out the README rather than just adding it to the drupal core
   +      // repo.
   

   80 cols.

4. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigImporterTest.php
   @@ -749,15 +749,19 @@ public function testInstallProfileMisMatch() {
   +    // @todo this is a legacy test.
   

   I think this todo can be removed?

Comments addressed in #46 & added an interdiff as well.

As far as I understand it, this seems like a good way to deprecate the old way of doing things.

Back to rtbc, looks like that was a random fail.

+++ b/core/lib/Drupal/Core/Config/FileStorageFactory.php
@@ -23,9 +25,19 @@ public static function getActive() {
+      throw new \Exception('In case the sync directory does not exist or is not defined in $settings["config_sync_directory"]');

This exception message reads wrong to me - shouldn't it just say 'The config sync directory is not defined in $settings['config_sync_directory']'?

The exception was changed to @catch's suggestion, back to rtbc.

Retesting because the error message indicated that the failure probably had nothing to do with this patch.

PDOStatement::execute(): MySQL server has gone away

I came back to check if the test was green after I put it on rtbc earlier this morning, I noticed that this is introducing 3 new CS errors, we should fix those as well, back to NW for that small change.

Fixed up the coding standards

Committed 1a2babd and pushed to 8.8.x. Thanks!

This has been reported to be a disruptive change; can we get a release note? (Edit: And/or update the CR to clarify that there isn't actually a break and merely a deprecation, depending on what's actually the case here.)

Thanks @bircher. So based on that note, the old API will continue to work and there's no impact on existing sites? (Is the same also true of the related functions and constants?) If so (if there's no actual disruptions in D8) let's instead update the CR to clarify that for all of them. (Deprecation is mentioned for some but not all of the affected APIs.)

I think this is worth mentioning. The important change here isn't the API changes, those are mostly internal and probably almost nobody uses them except some config-related modules.

The change that affects every site is that site owners need to update their settings.php file and change the generated PHP code (or the one they adjusted themself) to something else. And that seems worth mentioning. It doesn't break D8 in any way, but users need to do this before updating to D9.

The change that affects every site is that site owners need to update their settings.php file and change the generated PHP code (or the one they adjusted themself) to something else.

Just a reminder that we're trying really hard to make Drupal 9 easy to upgrade to. We should try and minimize these kinds of "all site owners need to..." changes outside of major/critical issues, IMO. (Not arguing for rolling this back, per se, just a general plea to consider "downstream" impact of DX improvements in general, especially this late in D8's lifetime.)

if not in a major version, when are we supposed to make a change like this? IMO we should be making more cleanups in Drupal 9, not less.