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'

Problem/Motivation

We used to have an active CMI config directory which actually contained configuration, but with #2161591: Change default active config from file storage to DB storage we have the active configuration in the DB.
But still we ship with an active directory, which is empty all the time, unless you install a contrib module like https://www.drupal.org/project/config_devel that saves the configuration again in a directory.

But not a lot of Sites will have that installed.
And also I've seen a lot of developers being confused about an empty active configuration directory and wondering that something is broken or so.

Proposed resolution

Don't create an active directory per default, let contrib modules that need that work via the requirements system to tell the users that they should create a new config directory

Remaining tasks

- Agree (done)
- Write Patch (done)

User interface changes

-

API changes

-

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it is not an actual programming Bug. More a DX Bug. We create an active directory but never use it by default, which can be confusing and misleading for Developers. Actually this already happened to Amazee Labs and our environments.
Issue priority Major because CMI is a new system that nobody really knows yet, the less confusion be bring with it, the better.
Disruption This will break some contrib modules that expect an active config, like config_devel or drush. But both maintainers of these contrib modules already agreed to the change and will update their modules.

Change Notice

https://www.drupal.org/node/2501187

Files: 
CommentFileSizeAuthor
#40 cmi_don_t_ship_with_a-2487592-40.patch21.23 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,674 pass(es). View
#37 interdiff-35-37.txt448 bytesJeroenT
#37 cmi_don_t_ship_with_a-2487592-37.patch21.23 KBJeroenT
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch cmi_don_t_ship_with_a-2487592-37.patch. Unable to apply patch. See the log in the details link for more information. View
#35 interdiff-34-35.txt5.36 KBJeroenT
#35 cmi_don_t_ship_with_a-2487592-35.patch20.67 KBJeroenT
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,174 pass(es), 2 fail(s), and 0 exception(s). View
#34 cmi_don_t_ship_with_a-2487592-34.patch18.29 KBJeroenT
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,175 pass(es). View
#25 2487592-schnitzel.25.patch18.29 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,216 pass(es). View
#25 22-25-interdiff.txt2.21 KBalexpott
#22 2487592-schnitzel.22.patch16.95 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,397 pass(es). View
#22 18-22-interdiff.txt1.1 KBalexpott
#18 2487592.16.patch16.4 KBSchnitzel
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2487592.16.patch. Unable to apply patch. See the log in the details link for more information. View
#16 2487592.1.patch16.4 KBSchnitzel
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2487592.1_0.patch. Unable to apply patch. See the log in the details link for more information. View
#1 2487592.1.patch16.4 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2487592.1.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

alexpott’s picture

Status: Needs work » Needs review
Issue tags: -CMI +Configuration system, +Needs issue summary update
FileSize
16.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2487592.1.patch. Unable to apply patch. See the log in the details link for more information. View

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

Status: Needs review » Needs work

The last submitted patch, 1: 2487592.1.patch, failed testing.

alexpott’s picture

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.

Status: Needs work » Needs review

alexpott queued 1: 2487592.1.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: 2487592.1.patch, failed testing.

chx’s picture

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!

Status: Needs work » Needs review

anavarre queued 1: 2487592.1.patch for re-testing.

moshe weitzman’s picture

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

Schnitzel’s picture

Priority: Normal » Major
Issue summary: View changes

so we are green and the maintainers of config_devel @chx and drush @moshe both agreed to do this change, which will be most disruptive for them. Added Beta Evaluation.

One question though, not for the patch itself, more for the process of changes during Beta:

+++ b/core/includes/bootstrap.inc
@@ -99,6 +99,9 @@
+ *
+ * @deprecated Drupal 8.0.0 no longer creates an active directory. This will be
+ *   removed in Drupal 9.0.x
  */
 const CONFIG_ACTIVE_DIRECTORY = 'active';

so we leave that in?
Because the beta evaluation has a line which states "code already marked for removal by 8.0.0" are marked as prioritized and should happen during beta.
If I understand that correct, should we remove that then completely?

alexpott’s picture

@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’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

ah! allright @alexpott thanks for clarification.

Well then I think we are ready with that.
I'm happy to write the ChangeNotice for that after it is committed.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

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

Status: Needs work » Needs review

Schnitzel queued 1: 2487592.1.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: 2487592.1.patch, failed testing.

Schnitzel’s picture

Issue summary: View changes
Schnitzel’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
16.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2487592.1_0.patch. Unable to apply patch. See the log in the details link for more information. View

reroll.

Also created Change Notice Draft
https://www.drupal.org/node/2501187

Status: Needs review » Needs work

The last submitted patch, 16: 2487592.1.patch, failed testing.

Schnitzel’s picture

Status: Needs work » Needs review
FileSize
16.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2487592.16.patch. Unable to apply patch. See the log in the details link for more information. View

wrong patch *g*

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

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

catch queued 18: 2487592.16.patch for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: 2487592.16.patch, failed testing.

alexpott’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
FileSize
1.1 KB
16.95 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,397 pass(es). View

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'
Fabianx’s picture

Issue tags: -Needs change record

Edited change record to point to this issue :).

xjm’s picture

Status: Reviewed & tested by the community » Needs work

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?

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.21 KB
18.29 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,216 pass(es). View

@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.

Mile23’s picture

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

Mile23 queued 25: 2487592-schnitzel.25.patch for re-testing.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 25: 2487592-schnitzel.25.patch, failed testing.

Status: Needs work » Needs review

JeroenT’s picture

Status: Needs review » Reviewed & tested by the community

RTBC as per #28.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

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',

     

JeroenT’s picture

Status: Needs work » Needs review
FileSize
18.29 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,175 pass(es). View

Patch no longer applied cleanly. Created straight reroll.

JeroenT’s picture

FileSize
20.67 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,174 pass(es), 2 fail(s), and 0 exception(s). View
5.36 KB

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.

Status: Needs review » Needs work

The last submitted patch, 35: cmi_don_t_ship_with_a-2487592-35.patch, failed testing.

JeroenT’s picture

Status: Needs work » Needs review
FileSize
21.23 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch cmi_don_t_ship_with_a-2487592-37.patch. Unable to apply patch. See the log in the details link for more information. View
448 bytes

Fixed failing test.

Status: Needs review » Needs work

The last submitted patch, 37: cmi_don_t_ship_with_a-2487592-37.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
FileSize
21.23 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,674 pass(es). View

I needed a reroll.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the reroll.

ianthomas_uk’s picture

Issue tags: +@deprecated

@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.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 02569d1 on 8.0.x
    Issue #2487592 by alexpott, JeroenT, Schnitzel, cilefen: CMI: don't ship...
alexpott’s picture

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

cweagans’s picture

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.

Fabianx’s picture

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

Schnitzel’s picture

mhh, I don't see that in HEAD:

http://cgit.drupalcode.org/drupal/tree/core/includes/install.core.inc#n365

// Determine whether base system services are ready to operate.
  $install_state['config_verified'] = install_ensure_config_directory(CONFIG_SYNC_DIRECTORY);
cweagans’s picture

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