It currently *looks* like the config system would depend on the public:// path configuration, but it actually does not. See discussion in #1496480-108: Convert file system settings to the new configuration system and following comments.

In #112, I proposed to use the full relative path from DRUPAL_ROOT in the config directories, basically making the absolute flag the default and removing it. Combined with adding the file_path_public as a setting to settings.php only, this should make it easier to understand how these two configurations relate to each other.

Because right now, you might think that if you change the location of the public files, you also need to move the config folder. Which you actually are not supposed to do.

Alternatively, once file_public_path is a setting, CMI could actually depend on it and use it. But I would prefer a complete path in settings.php

Files: 
CommentFileSizeAuthor
#22 1887750.22.patch7.47 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 60,036 pass(es). View
#22 21-22-interdiff.txt2.44 KBalexpott
#21 1887750.21-interdiff.txt2.02 KBBerdir
#21 1887750.21.patch5.74 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 59,977 pass(es). View
#18 1887750.18.patch4.04 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 24,666 pass(es), 9,601 fail(s), and 6,913 exception(s). View
#18 12-18-interdiff.txt906 bytesalexpott
#12 config-directories-structure-1887750-12.patch3.97 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#5 config-directories-structure-1887750-5.patch3.85 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config-directories-structure-1887750-5.patch. Unable to apply patch. See the log in the details link for more information. View
#5 config-directories-structure-1887750-5-interdiff.txt595 bytesBerdir
#3 config-directories-structure-1887750-3.patch3.64 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 47,574 pass(es), 73 fail(s), and 9 exception(s). View
#1 config-directories-structure-1887750-1.patch2.77 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 28,308 pass(es), 11,724 fail(s), and 3,938 exception(s). View

Comments

Berdir’s picture

Status: Active » Needs review
FileSize
2.77 KB
FAILED: [[SimpleTest]]: [MySQL] 28,308 pass(es), 11,724 fail(s), and 3,938 exception(s). View

This seems to work fine. Will, of course, break all existing installations in interesting ways ;)

Not converted to $settings yet, as drupal_write_settings() does not support it yet...

Status: Needs review » Needs work

The last submitted patch, config-directories-structure-1887750-1.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.64 KB
FAILED: [[SimpleTest]]: [MySQL] 47,574 pass(es), 73 fail(s), and 9 exception(s). View

Updated default.settings.php and fixed the reference in TestBase. This should fix most if not all test failures.

Status: Needs review » Needs work

The last submitted patch, config-directories-structure-1887750-3.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
595 bytes
3.85 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config-directories-structure-1887750-5.patch. Unable to apply patch. See the log in the details link for more information. View

This should fix the DUBT tests.

heyrocker’s picture

Issue tags: +Configuration system
Berdir’s picture

Tagging.

Berdir’s picture

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -918,14 +918,14 @@ protected function prepareConfigDirectories() {
       // Assign the relative path to the global variable.

The comment here needs to be updated/removed.

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, config-directories-structure-1887750-5.patch, failed testing.

alexpott’s picture

Priority: Normal » Major
Issue summary: View changes
Issue tags: +beta blocker, +Configuration system

Discussed with @berdir on IRC and agreed that this was important due to the fact that:

because right now, it's weird to have it still within drupal but outside of the files path

So for example if you want to store configuration in sites/all/configM and keep the staging under git you'd have to use an absolute path. With this patch you got use a path relative to DRUPAL_ROOT. This makes transporting settings.php easier.

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.97 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Re-roll, will update the issue summary and so on asap.

Status: Needs review » Needs work

The last submitted patch, 12: config-directories-structure-1887750-12.patch, failed testing.

sun’s picture

Hm. The original idea of #1741804: Implement a config import/staging directory was to allow to move the configuration directories outside of the public document root. For example, for security reasons.

Looking at the configured paths in settings.php in this patch, it appears we'd be removing support for that option?

That said, I was never really happy with the extra 'absolute' key. Normally, you'd expect file paths to work exactly as in any other PHP code; i.e.:

sites/default/files/config_active → DRUPAL_ROOT/sites/default/files/config_active
/var/drupal/config → /var/drupal/config
public://config_active → DRUPAL_ROOT/sites/default/files/config_active

The focus is on natural absolute vs. relative paths. The last stream wrapper example can be ignored, if it's not possible due to bootstrap dependencies.

alexpott’s picture

re #14 as far as I understand that is exactly what Berdir is proposing :)

Berdir’s picture

Yes, the absolute flag was necessary because we enforced the config path, with this, you can specify a relative or absolute path simply by doing so, just like the public files path.

sun’s picture

I assume that this change is the error:

+++ b/core/includes/install.inc
@@ -452,17 +452,10 @@ function drupal_install_config_directories($mode = NULL) {
     $settings['config_directories'] = array(
-      CONFIG_ACTIVE_DIRECTORY => array(
-        'path' => (object) array(
-          'value' => 'config_' . $config_directories_hash . '/active',
-          'required' => TRUE,
-        ),
-      ),
-      CONFIG_STAGING_DIRECTORY => array(
-        'path' => (object) array(
-          'value' => 'config_' . $config_directories_hash . '/staging',
-          'required' => TRUE,
-        ),
+      'value' => array(
+        CONFIG_ACTIVE_DIRECTORY => conf_path() . '/files/config_' . $config_directories_hash . '/active',
+        CONFIG_STAGING_DIRECTORY => conf_path() . '/files/config_' . $config_directories_hash . '/staging',
+        'required' => TRUE,
       ),
     );

Unfortunately, I'm not familiar with the structure/features of install_rewrite_settings().

But it looks like 'required' is placed in the wrong spot and the 'value' + 'required' actually need to be properties of an anonymous object?

alexpott’s picture

FileSize
906 bytes
4.04 KB
FAILED: [[SimpleTest]]: [MySQL] 24,666 pass(es), 9,601 fail(s), and 6,913 exception(s). View

The patch attached should fix it.

After applying this patch the values in my settings.php are:

$config_directories['active'] = 'sites/default/files/config_BZwtrpONxujo5cY2DwnJzOvHuTpNg5NBRzNssQBYDo0/active';
$config_directories['staging'] = 'sites/default/files/config_BZwtrpONxujo5cY2DwnJzOvHuTpNg5NBRzNssQBYDo0/staging';
alexpott’s picture

Status: Needs work » Needs review

Go testbot go!

Status: Needs review » Needs work

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
5.74 KB
PASSED: [[SimpleTest]]: [MySQL] 59,977 pass(es). View
2.02 KB

Thanks, this should fix the web tests...

alexpott’s picture

FileSize
2.44 KB
7.47 KB
PASSED: [[SimpleTest]]: [MySQL] 60,036 pass(es). View

I think we need to fix the installer tests too

Status: Needs review » Needs work

The last submitted patch, 22: 1887750.22.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

22: 1887750.22.patch queued for re-testing.

alexpott’s picture

Status: Needs work » Needs review

22: 1887750.22.patch queued for re-testing.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, let's move forward here.

chx’s picture

So config was not using absolute paths before but because file_get_contents used in FileStorage::read defaults to $use_include_path = false this does not pose a performance problem. Then, I guess we need to file a separate issue for listAll not supporting outside of webroot paths, right?

xjm’s picture

22: 1887750.22.patch queued for re-testing.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

This patch breaks drush si:

is_dir() expects parameter 1 to be string, array given install.inc:522                  [warning]
strpos() expects parameter 1 to be string, array given file.inc:267                     [warning]
rtrim() expects parameter 1 to be string, array given file.inc:532                      [warning]
strpos() expects parameter 1 to be string, array given file.inc:267                     [warning]
rtrim() expects parameter 1 to be string, array given file.inc:532                      [warning]
WD php: Exception: File system:                                                         [error]

The directory  does not exist. The directory  does not exist. An automated attempt to
create this directory failed, possibly due to a permissions problem. To proceed with the
installation, either create the directory and modify its permissions manually or ensure
that the installer has the permissions to create it automatically. For more information,
see INSTALL.txt or the online handbook. in install_display_requirements() (line 2569 of
/Users/webchick/Sites/8.x/core/includes/install.core.inc).
Exception: File system: 

The directory <em class="placeholder"></em> does not exist. The directory <em class="placeholder"></em> does not exist. An automated attempt to create this directory failed, possibly due to a permissions problem. To proceed with the installation, either create the directory and modify its permissions manually or ensure that the installer has the permissions to create it automatically. For more information, see INSTALL.txt or the <a href="http://drupal.org/server-permissions">online handbook</a>. in install_display_requirements() (line 2569 of /Users/webchick/Sites/8.x/core/includes/install.core.inc).
Drush command terminated abnormally due to an unrecoverable error.                      [error]

This might be unavoidable, or it might be pointing to a bug. Setting back to needs review to find out.

alexpott’s picture

@webchick drush si works fine for me. I suspect that you did not re-create your settings.php and therefore had the $config_directories global declared with the old structure as that would cause the error you are seeing.

What this does mean (and it is unavoidable) is that this patch will break every existing install of Drupal 8.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Yes, I think the testbot uses drush si now as well, so it would not be able to install there.

As @alexpott mentioned, the real problem is probably that you have an old settings.php with the old $config_directories variable.

Yes, this does break every install completely, it is however very easy to fix.

Simply convert this:

$config_directories['active']['path'] = 'config.....';
$config_directories['staging']['path'] = 'config.....';

To this:

$config_directories['active'] = 'sites/default/files/config.....';
$config_directories['staging'] = 'sites/default/files/config.....';

Make sure that you do this before you attempt to load the site again, or it might result in inconsistent caches.

While working on this, I had both versions in my settings.php and just commented the one that I didn't need.

damiankloip’s picture

Also, unless a patch stops the testbot from installing with drush (we have no choice then) we should never hold up patches on that. It should be up to drush to keep up with core.

catch’s picture

Title: Use path relative to DRUPAL_ROOT in configuration directories » Change notice: Use path relative to DRUPAL_ROOT in configuration directories
Status: Reviewed & tested by the community » Active

Committed/pushed to 8.x, thanks!

Needs change notice (or update to one) for the changed $settings.

Berdir’s picture

Title: Change notice: Use path relative to DRUPAL_ROOT in configuration directories » Use path relative to DRUPAL_ROOT in configuration directories
Priority: Major » Critical
Status: Active » Fixed

Updated https://drupal.org/documentation/administer/config, was the only place on d.o that I could find that references this and isn't an issue.

Also created https://drupal.org/node/2164727.

Status: Fixed » Closed (fixed)

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