CMI: don't ship with a default "active" directory that is empty in most Drupal installations

We need an beta evaluation. I've tried to do this is a bc compatible way that ensures that config_devel should still work.

Test name	Pass	Fail	Exception
CollapsedDrupal\system\Tests\Entity\EntityFieldTest	1023	1	0
Message	Group	Filename	Line	Function	Status
The test did not complete due to a fatal error.	Completion check	EntityFieldTest.php	526	Drupal\system\Tests\Entity\EntityFieldTest->testDataStructureInterfaces()	

Very odd file.

Eh, you can break config_devel. We will figure it out. We can write the name of the directory into state and create a dir in sites/default/files. Leave me an IRC tell when this happens, thanks!

+1 to this. I will update Drush status accordingly.

@Schnitzel it's a new deprecation (i.e. it is not already marked for removal) so we should leave it in and say we'll remove in 9.x which is exactly what the patch does.

@Schnitzel good point re change notice. Actually the draft should be written before - be my guest :)

Hurray, it's green ... I reviewed the patch and it like a sane place to leave D8.

Rerolled - also include the change from #2503015: Remove usages of conf_path() in InstallerExistingSettingsNoProfileTest so we don't get any more rerolling fun - adding contributors to that patch to the suggested commit message.

Suggested commit message

git commit -m 'Issue #2487592 by Schnitzel, alexpott, cilefen, joshi.rohit100: CMI: don'\''t ship with a default "active" directory that is empty in most Drupal installations'

Edited change record to point to this issue :).

Nice! A big +1 for this change.

1. +++ b/core/includes/bootstrap.inc
   @@ -100,6 +100,9 @@
   + * @deprecated Drupal 8.0.0 no longer creates an active directory. This will be
   + *   removed in Drupal 9.0.x
   
   +++ b/core/lib/Drupal/Core/Config/FileStorageFactory.php
   @@ -15,6 +15,9 @@ class FileStorageFactory {
   +   * @deprecated Drupal 8.0.0 no longer creates an active directory. This will
   +   *   be removed in Drupal 9.0.x
   

   I'd use the more standard/specific form:

   @deprecated in Drupal 8.0.x and will be removed before 9.0.0. Drupal core no longer creates an active directory.

   Also make sure to put the final period on the last sentence. :)

2. +++ b/core/includes/bootstrap.inc
   @@ -163,7 +166,7 @@ function conf_path($require_settings = TRUE, $reset = FALSE, Request $request =
     * @param string $type
     *   (optional) The type of config directory to return. Drupal core provides
   - *   'active' and 'staging'. Defaults to CONFIG_ACTIVE_DIRECTORY.
   + *   'staging'. Defaults to CONFIG_ACTIVE_DIRECTORY.
   

   This is confusing now. We say that core only provides staging but then that it defaults to active? Is that to avoid a BC break? I think we need to at least explain this a little better, but if core is no longer providing an active directory, would it really be that much more disruptive to stop defaulting to something we don't provide?

@xjm thanks for the review.

1. Yep fixed.
2. You're right this was to avoid a BC break. I agree stopping using a default we don't provide is preferable. In fact if we remove the default altogether then any usages will get an exception and it'll be simple to fix.

I've also added some missing explicit test coverage of config_get_config_directory() and the exception behaviour.

I can't speak to the architectural changes, but I want to run the testbot again, since it's blocking #2457469: Remove usages of conf_path()

Here's a superficial review:

1. +++ b/core/includes/bootstrap.inc
   @@ -161,14 +164,17 @@ function conf_path($require_settings = TRUE, $reset = FALSE, Request $request =
   + *   The type of config directory to return. Drupal core provides the
   + *   CONFIG_STAGING_DIRECTORY constant to access the staging directory.
     *
     * @return string
     *   The configuration directory path.
     */
   -function config_get_config_directory($type = CONFIG_ACTIVE_DIRECTORY) {
   +function config_get_config_directory($type) {
   

   Should say @throws \Exception

2. +++ b/core/lib/Drupal/Core/Config/BootstrapConfigStorageFactory.php
   @@ -48,7 +48,13 @@ public static function getDatabaseStorage() {
   +   * If there is no active configuration directory calling this method will
   +   * result in an error.
   +   *
       * @return \Drupal\Core\Config\FileStorage
   +   *
   +   * @deprecated Drupal 8.0.0 no longer creates an active directory. This will
   +   *   be removed in Drupal 9.0.x.
       */
      public static function getFileStorage() {
   

   Since it mentions an error, it should say @throws \Exception

Back to RTBC, both issues seem pre-existing normal documentation bugs to me and should not affect RTBC status as this just moves the code.

Could be fixed on commit though.

RTBC as per #28.

Thanks @alexpott, @Mile23, and @Fabianx.

I've found a few more things in my latest review, mostly minor:

1. +++ b/core/includes/bootstrap.inc
   @@ -100,6 +100,9 @@
   + * @deprecated in Drupal 8.0.x and will be removed before 9.0.0. Drupal core no
   + *   longer creates an active directory.
   
   +++ b/core/lib/Drupal/Core/Config/BootstrapConfigStorageFactory.php
   @@ -48,7 +48,13 @@ public static function getDatabaseStorage() {
   +   * @deprecated Drupal 8.0.0 no longer creates an active directory. This will
   +   *   be removed in Drupal 9.0.x.
   
   +++ b/core/lib/Drupal/Core/Config/FileStorageFactory.php
   @@ -15,6 +15,9 @@ class FileStorageFactory {
   +   * @deprecated Drupal 8.0.0 no longer creates an active directory. This will
   +   *   be removed in Drupal 9.0.x
   

   One of these got fixed but not the others. :P
    

2. +++ b/core/includes/install.inc
   @@ -486,36 +480,25 @@ function drupal_install_config_directories() {
   +  // Put a README.txt into each config directory. This is required so that
   +  // they can later be added to git. Since these directories are auto-
   +  // created, we have to write out the README rather than just adding it
   +  // to the drupal core repo.
   

   This comment still refers to plural config directories.
    

3. +++ b/core/lib/Drupal/Core/Config/BootstrapConfigStorageFactory.php
   @@ -48,7 +48,13 @@ public static function getDatabaseStorage() {
   +   * If there is no active configuration directory calling this method will
   +   * result in an error.
   

   Is the error an exception? If so there should be an @throws as @Mile23 says. Regarding #28, the behavior that it's not expected to have an active directory is new, so it's not as simple as moved docs.
    

4. +++ b/core/modules/config/src/Tests/ConfigImporterTest.php
   @@ -648,4 +648,21 @@ public function testMissingCoreExtension() {
   +  /**
   +   * Tests config_get_config_directory().
   +   */
   +  public function testConfigGetConfigDirectory() {
   +    $directory = config_get_config_directory(CONFIG_STAGING_DIRECTORY);
   +    $this->assertEqual($this->configDirectories[CONFIG_STAGING_DIRECTORY], $directory);
   +
   +    $message = 'Calling config_get_config_directory() with CONFIG_ACTIVE_DIRECTORY results in an exception.';
   +    try {
   +      config_get_config_directory(CONFIG_ACTIVE_DIRECTORY);
   +      $this->fail($message);
   +    }
   +    catch (\Exception $e) {
   +      $this->pass($message);
   +    }
   +  }
   

   This is a bit odd as a test. Is the idea to ensure that callers get a clear BC break?
    

5. Also there's a couple usages that don't seem to be covered by the patch and might need fixing:

   core/includes/install.inc: *     CONFIG_ACTIVE_DIRECTORY => (object) array(
   sites/default/default.settings.php: *     CONFIG_ACTIVE_DIRECTORY => '/some/directory/outside/webroot',

    

Patch no longer applied cleanly. Created straight reroll.

Regarding #33.

1. Done.
2. Done.
3. Done.
4. Not sure why this test was added.
5. Done. Removed unused usages of CONFIG_ACTIVE_DIRECTORY.

Patch attached.

Fixed failing test.

I needed a reroll.

Thanks for the reroll.

@deprecated because this is blocking removal of conf_path (because that would cause a reroll for this patch). If there are concerns that make this unlikely to make RC1 then we can just remove conf_path, but I didn't want to disrupt this issue.

Committed/pushed to 8.0.x, thanks!

Re #33.4 Yes it was - and it is also testing calling the code with a config directory key that does not exist.

The installer is still checking for an active directory as a condition for skipping the db configuration page:

```
$install_state['config_verified'] = install_ensure_config_directory(CONFIG_ACTIVE_DIRECTORY) && install_ensure_config_directory(CONFIG_STAGING_DIRECTORY);
```

Should this be fixed here? Or in a follow up issue? Or was that intentional?

As it is right now, we have lost the ability to pre-configure db and config dir information in settings.php and have the installer skip the database configuration page.

#47: Follow-up I think, lets open one?

Well wtf was I looking at. Disregard. Sorry for the noise everyone.