Problem/Motivation

Following #2161591: Change default active config from file storage to DB storage now the DB storage is the default storage for configuration. However default.settings.php still says:

 * By default, Drupal configuration files are stored in a randomly named
 * directory under the default public files path....
 *
 * @todo Flesh this out, provide more details, etc.

Followed by

 * By default, the active configuration is stored in the database in the
 * {config} table. To install Drupal with a different active configuration

Both cannot be true at the same time. Also fleshing out the file storage docs would indeed be in order to explain when it is used at all. I think it would be used in bootstrap but not sure(?).

Proposed resolution

Fix the docs.

Remaining tasks

Update the documentation to reflect the change in #2241633: Simplify site-specific service overrides.

User interface changes

None.

API changes

None.

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch Instructions
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

Follow-up

Files: 
CommentFileSizeAuthor
#54 interdiff-2243439-49-54.txt3.4 KBamitgoyal
#54 2243439_54.patch4.83 KBamitgoyal
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,483 pass(es). View
#30 2243439-30.patch4.21 KBpwolanin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,742 pass(es). View
#30 increment.txt1.18 KBpwolanin
#23 2243439-23.patch4.26 KBscor
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,795 pass(es). View

Comments

pwolanin’s picture

Assigned: Unassigned » pwolanin

I'll try to make a patch today

Gábor Hojtsy’s picture

@pwolanin: I fear this may fall of the radar soon if not done quickly. Any suggestions? :)

pwolanin’s picture

Component: configuration system » documentation
Assigned: pwolanin » Unassigned
Status: Active » Needs review
FileSize
2.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,036 pass(es). View

This is really a doc issue - just changing comments or commented-out lines.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/sites/default/default.settings.php
@@ -216,12 +216,15 @@
+ * By default, Drupal's active configuration is stored in the database.
+ * By default, configuration import occurs using files stored in in a randomly
+ * named directory under the default public files path. On install the named
+ * directory is created in the default files directory. For enhanced

- "in in"

- This does not explain how the staging and active directories relate, there is just one directory mentioned. Is the mentioned directory the active one? Then this text should say that is the active directory, and what staging is used for no?

pwolanin’s picture

Status: Needs work » Needs review
FileSize
1.59 KB
2.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,040 pass(es). View

added more explanation

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/sites/default/default.settings.php
@@ -216,12 +216,18 @@
+ * However, configuration import occurs using files. By default these are found
+ * in a randomly named directory under the default public files path. On install
+ * the directory is created in the default files directory. For enhanced
+ * security, you may set the path to a location outside your docroot.

Yay! Now I think I understand it all. The only thing this paragraph does not mention is this is the staging directory. Since the second para talks about the active directory, I think one little change to say this one is about the staging directory should clear everything up.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
1.67 KB
2.48 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,122 pass(es). View
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

I think I understand all of this now. Good updates. Thanks a lot.

sun’s picture

+ # $GLOBALS['conf']['container_yamls'][] = $conf_path . '/services.yml';

Hm. Not sure whether we want to document that, given #2241633: Simplify site-specific service overrides

As a follow-up to that issue, I'd actually recommend to remove the $GLOBALS['conf']['container_yamls'] variable entirely, and instead, make the kernel additionally check for a environment-specific services.$env.yml file.

Aside from that, this looks relatively good to me. I only wondered whether there's a possibility to shorten the text in any way?

sun’s picture

mtift’s picture

This is a huge improvement! Two minor nitpicks:

+++ b/sites/default/default.settings.php
@@ -216,12 +216,21 @@
+ * By default, Drupal's active configuration is stored in the database.
+ * However, configuration import occurs using files. By default the staging

These should probably both start, "By default," (with a comma).

+++ b/sites/default/default.settings.php
@@ -216,12 +216,21 @@
+ * configuration. See below the active configuration settings documentation.

I'd change this to "See the active configuration settings documentation below."

pwolanin’s picture

FileSize
1.62 KB
2.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,152 pass(es). View

Minor wording tweaks as suggested.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

I think this still needs some work. Starting with the first paragraph:

+ * By default, Drupal's active configuration is stored in the database, while
+ * configuration import occurs using files. By default, the staging  directory
+ * used for configuration import is found in a randomly named directory under
+ * the default public files path. On install the directory is created in the
+ * default files directory. For enhanced security, you may set the path to a
+ * location outside your docroot.

a) The word "default" and phrase "by default" occur way too many times. It's kind of hard to read.
b) There's an extra space near the end of the 2nd line here.
c) This isn't actually accurate. The config directly is the random-named directly that is created, and under that, there's "active" and "staging".
d) "docroot" is also not a word. Say "document root" please.
e) This paragraph is a mix of talking about config storage and config import and config directories. It's very confusing. Start at the beginning (what this setting is for: the config directories), and then explain what the config directories are used for, maybe?

Second paragraph:

+ *
+ * On install, Drupal will also set up an active configuration directory next
+ * to the staging directory. The active directory is expected to be empty unless
+ * you change the configuration system to use file storage for the active
+ * configuration. See the active configuration settings documentation below.

f) What does "next to" mean?

Third paragraph:

+ *
+ * If you use files for the active configuration in production you may enhance
+ * your security by setting the path to the active configuration to a location
+ * outside your docroot.

g) See (d).

Last section of patch:

+ * module or profile, or by adding to the container YAML files for DrupalKernel.
+ *
  *
  * The 'bootstrap_config_storage' setting needs to be a callable that returns

h) Extra blank line has been added.

Very end of patch:

  * The 'bootstrap_config_storage' setting needs to be a callable that returns
- * core.services.yml.
+ * an instance implementing \Drupal\Core\Config\StorageInterface.
  */
  # $settings['bootstrap_config_storage'] = array('Drupal\Core\Config\BootstrapConfigStorageFactory', 'getFileStorage');
+ # $GLOBALS['conf']['container_yamls'][] = $conf_path . '/services.yml';

i) That GLOBALS line is not explained, is it?

And then...
j) The active and staging config directories are given README files when you install. These I think also need an update in this patch.

cilefen’s picture

Status: Needs work » Needs review
FileSize
2.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,626 pass(es). View
2.61 KB

Attempting a-h. Thoughts?

cilefen’s picture

FileSize
4.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,694 pass(es). View
1.29 KB

j

jhodgdon’s picture

Status: Needs review » Needs work

Much better thanks!

Still some things to fix:

a) install.inc
... By default, this directory will be empty. To move this configuration between environments,...
So by default it is empty, and then you can move the configuration??? Maybe say "If you are using files to store configuration, and you want to move it between environments..." or something like that?

b) default.settings.php

  * By default, the active configuration is stored in the database in the
  * {config} table. To install Drupal with a different active configuration
- * storage, you need to override the setting here, in addition to overriding
- * the config.storage.active service definition in a module or profile.
+ * storage, you need to override the 'bootstrap_config_storage' setting here, in
+ * addition to overriding the config.storage.active service definition in a
+ * module or profile, or by adding to the container YAML files for DrupalKernel.
  *
  * The 'bootstrap_config_storage' setting needs to be a callable that returns
- * core.services.yml.
+ * an instance implementing \Drupal\Core\Config\StorageInterface.
  */
  # $settings['bootstrap_config_storage'] = array('Drupal\Core\Config\BootstrapConfigStorageFactory', 'getFileStorage');
+ # $GLOBALS['conf']['container_yamls'][] = $conf_path . '/services.yml';

I'm still confused here. If someone uncommented these two lines, what else would they need to do in order to get this to work, so that they are using files as their active config storage? And shouldn't it be pretty easy to do that, since I think the services/classes to do this already exist in Core, don't they??

So can we instead give explicit instructions here, like "Uncomment these two lines, change X to Y, and they in a module, do Z"? I just don't understand the instructions that are in this patch (or the ones that were in the file before the patch, for that matter). And I have no idea whether or not the $GLOBALS line is correct or not either.

cilefen’s picture

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

This is just the README change.

sun’s picture

I'd prefer to wait until #2241633: Simplify site-specific service overrides has landed, so we do not have to document how to futz with global variables here, because futzing with global variables is something we shouldn't document.

cilefen’s picture

@sun Makes sense.

pwolanin’s picture

This issue is in, so we should be able to update the patch now: #2241633: Simplify site-specific service overrides

Gábor Hojtsy’s picture

17: 2243439-17.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 17: 2243439-17.patch, failed testing.

scor’s picture

FileSize
4.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,795 pass(es). View

straight reroll of #17, I haven't addressed #18.

amitgoyal’s picture

Status: Needs work » Reviewed & tested by the community

#23 looks good to me.

scor’s picture

Status: Reviewed & tested by the community » Needs review

@sun, #18 still needs to be addressed, right? or did it go away?

jhodgdon’s picture

Status: Needs review » Needs work

It looks like the patch still adds a GLOBALS and according to sun/comment #18 this should go away, so marking Needs Work. Someone needs to read the other issue (or its change notice) and figure out what changed, or maybe sun can make a suggestion?

cilefen’s picture

Status: Needs work » Needs review

23: 2243439-23.patch queued for re-testing.

cilefen’s picture

Issue summary: View changes
Issue tags: +Novice

Getting ready for the Austin DrupalCon sprint, following http://www.hook42.com/blog/prepping-drupalcon-austin-sprints-sprint-lead...

jhodgdon’s picture

Status: Needs review » Needs work

Can we please leave this issue at Needs Work until the issues in #18/#26 are addressed, thanks!

pwolanin’s picture

Status: Needs work » Needs review
FileSize
1.18 KB
4.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,742 pass(es). View

How about this?

jhodgdon’s picture

Status: Needs review » Needs work

Better! Now I think we can bikeshed about the quality of the documentation. I have a few thoughts:

a) In install.inc:

To make this configuration active, see admin/config/development/configuration/sync on the target server.

I don't think we "see" URLs.... How about "To import this configuration, visit ..." ?

b) First chunk of default.settings.php:

+ * The $config_directories array specifies the location of file system
+ * directories used to store Drupal's configuration. By default, the active
+ * configuration is stored in the database and configurations to be imported are
+ * stored in a staging directory. On install, the staging directory is created
+ * inside a randomly-named directory in the public files path.
+ *
+ * On install, Drupal will also set up an active configuration directory inside
+ * the same randomly-named directory in the public files path. The active
+ * directory is expected to be empty unless you change the configuration system
+ * to use file storage for the active configuration. See the active
+ * configuration settings documentation below.
+ *
+ * If you use files for the active configuration in production you may enhance
+ * your security by setting the path to the active configuration to a location
+ * outside your document root.

There is a ton of redundancy in the first two paragraphs, and I don't think "configuration" should be pluralized... How about:

The $config_directories array specifies the location of file system directories used for configuration data. On install, "active" and "staging" directories are created for configuration. The staging directory is used for configuration imports; the active directory is not used by default, since the default storage for active configuration is the database rather than the file system (this can be changed; see "Active configuration settings" below).

The default location for the active and staging directories is inside a randomly-named directory in the public files path; this setting allows you to override these locations. If you use files for the active configuration, you can enhance security by putting the active configuration outside your document root.

c) Last chunk in default.settings.php (I omitted the - lines for clarity):

  * By default, the active configuration is stored in the database in the
  * {config} table. To install Drupal with a different active configuration
+ * storage, you need to override the 'bootstrap_config_storage' setting here, in
+ * addition to overriding the config.storage.active service definition such as
+ * in a services.yml file in the same directory as settings.php which is used to
+ * register site-specific service definitions.
  *
  * The 'bootstrap_config_storage' setting needs to be a callable that returns
+ * an instance implementing \Drupal\Core\Config\StorageInterface.

I found this pretty hard to scan... How about:

By default, the active configuration is stored in the database in the {config} table. To use a different storage mechanism for the active configuration, do the following prior to installing:
- Override the 'bootstrap_config_storage' setting here. It must be set to a callable that returns an object that implements \Drupal\Core\Config\StorageInterface.
- Override the service definition 'config.storage.active'. Put this override in a services.yml file in the same directory as settings.php (definitions in this file will override service definition defaults).

Thoughts?

cilefen’s picture

Can we leave the edits for a novice to attempt on the June 6 Austin sprint please?

jhodgdon’s picture

Maybe, but I am usually against "reserving" issues for sprints... I understand wanting to have issues that are appropriate for novices to work on at a sprint, but there is no guarantee that if we ask someone not to work on an issue today, that it will get fixed tomorrow at the sprint.

I'd prefer not to discourage anyone from fixing any issue at any time. There are also people at DrupalCon all week getting mentored, not necessarily at a formal "sprint", and doing great work on issues, so reserving this for a particular sprint session doesn't seem like the right thing for those people either?

mark.labrecque’s picture

Assigned: Unassigned » mark.labrecque

I'm a novice, I will check this out :)

mark.labrecque’s picture

FileSize
5.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2243439-35_0.patch. Unable to apply patch. See the log in the details link for more information. View
5.51 KB

How does this look to you all?

mark.labrecque’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 35: 2243439-35.patch, failed testing.

mark.labrecque’s picture

35: 2243439-35.patch queued for re-testing.

The last submitted patch, 35: 2243439-35.patch, failed testing.

mark.labrecque’s picture

FileSize
16.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2243439-35_1.patch. Unable to apply patch. See the log in the details link for more information. View
mark.labrecque’s picture

Status: Needs work » Needs review
cilefen’s picture

@mark.labrecque If the patch doesn't apply, try a simple git diff instead of git format-patch.

Status: Needs review » Needs work

The last submitted patch, 40: 2243439-35.patch, failed testing.

mark.labrecque’s picture

Thanks for the tip! I will do that

cilefen’s picture

Also be sure to use unique patch file names that match the comment number.

mark.labrecque’s picture

FileSize
4.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,751 pass(es). View

Fingers crossed...

mark.labrecque’s picture

Status: Needs work » Needs review
pwolanin’s picture

Looks generally good, but not sure why this is indented:

+ *      The $config_directories array
mark.labrecque’s picture

FileSize
4.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,753 pass(es). View

Ah, I missed that. Thanks for pointing that out.

mpv’s picture

Assigned: mark.labrecque » Unassigned
Status: Needs review » Reviewed & tested by the community

Patch looks good to me.

jonreid’s picture

Assigned: Unassigned » jonreid

Looking at this now as a second review.

jonreid’s picture

Assigned: jonreid » Unassigned

Patch applies. Documentation looks good.

One caveat:

In applying the patch, I received a permissions issue on the second file change (default.settings.php). I had to roll back and manually change the rights on the default folder for the patch to apply. This won't be an issue for new installs once the changes are rolled into core.

Webchick mentioned this is a known issue and suggests:

sudo git checkout -- sites/default
OR
chmod ug+w sites/default

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

Looking better, thanks for the patch(es)!

A few notes:
a) install.inc

... If you are using files to store the active configuration, and you want to move it between environments, contents from this directory should be placed ...

I don't think "contents from this directory" makes sense... maybe "files from this directory"?

b) default.settings.php

  * For enhanced security, you may set this variable to a value using the
+ * contents of a file outside your document root that is never saved together
  * with any backups of your Drupal files and database.

Could we fix up the wording here? "never saved together with" ?!? "value using the contents of a file" ?!? What is this actually saying? And... this is actually out of scope for the issue here (it is in the "salt" section)... but if we are going to change this line anyway, could we fix it so it actually makes sense? My suggestion:

For enhanced security, you may set this variable to the contents of a file outside your document root; you should also ensure that this file is not stored with backups of your database.

c) default.settings.php

+ * {config} table. To use a different storage mechanism for the active
+ * configuration, do the following prior to installing:
+ *   - Override the 'bootstrap_config_storage' setting here. It must be set to a
+ *     callable that returns an object that implements
+ *     \Drupal\Core\Config\StorageInterface.
+ *   - Override the service definition 'config.storage.active'. Put this
+ *     override in a services.yml file in the same directory as settings.php
+ *     (definitions in this file will override service definition defaults).

List formatting: the - in list items should line up with the text above. So the entire list needs 2 spaces less indentation.

Other than that, looks good, thanks!

amitgoyal’s picture

Status: Needs work » Needs review
FileSize
4.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,483 pass(es). View
3.4 KB

Please review revised patch with fixes in #53.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

  • Commit 6664c6a on 8.x by jhodgdon:
    Issue #2243439 by amitgoyal, jonreid, mark.labrecque, mpv, pwolanin,...
jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks again everyone! Committed to 8.x.

YesCT’s picture

What does:

Override the service definition 'config.storage.active'. Put this
 *   override in a services.yml file in the same directory as settings.php

actually mean to do?

what exactly should I put in the yml file?

config.storage.active: true

?

vijaycs85’s picture

actually mean to do?

haven't tried, in theory yes.

what exactly should I put in the yml file?

We need to define active storage. Say, if we want to use fileStorage by default:

# sites/default/services.yml

services:
  config.storage.active:
    class: Drupal\Core\Config\FileStorage
    factory_class: Drupal\Core\Config\FileStorageFactory
    factory_method: getActive

jhodgdon’s picture

YesCT: Do you want to reopen this issue, or file a new one? If you think the docs need to be clarified, we can do that. But I think if someone is going to do this, they'll hopefully realize they need to figure out how to override a service and look at the Services and Dependency Injection Container topic on api.drupal.org (linked from the api.d.o landing page) and figure out how to define a service.

YesCT’s picture

Status: Fixed » Closed (fixed)

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

davidwbarratt’s picture

Related Question:
http://drupal.stackexchange.com/questions/151604/is-there-any-sensative-...

Please post an answer if you know it. :)