Problem/Motivation

If you want the installer to create the active configuration directory (currently complete used) but specify a staging directory - you can't. This situation is likely to be quite common as the staging directory is the one we advise people to keep in git.

This issue will allow a user to add

$config_directories['staging'] = 'staging2';

to settings.php before running the installer.

Proposed resolution

Update the active&stage directory creation code, so that it creates new directory if not set already.

Remaining tasks

Commit

User interface changes

None

API changes

None

Original report by @alexpott

CommentFileSizeAuthor
#5 2384853.3.patch2.4 KBalexpott
#5 1-3-interdiff.txt1.45 KBalexpott
#1 2384853.1.patch2.05 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Active » Needs review
FileSize
2384853.1.patch2.05 KB

This is especially helpful if using https://www.drupal.org/project/config_installer

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Issue summary: View changes
vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented 
+++ b/core/includes/install.inc
@@ -466,18 +466,26 @@ function drupal_install_config_directories() {
+  if (empty($config_directories[CONFIG_ACTIVE_DIRECTORY])) {
     $settings['config_directories'] = array(
...
+  if (empty($config_directories[CONFIG_STAGING_DIRECTORY])) {
+    $settings['config_directories'] = array(

Don't we want to do this:

$settings['config_directories'][CONFIG_ACTIVE_DIRECTORY] =  (object) array(
        'value' => conf_path() . '/files/config_' . $config_directories_hash . '/staging',
        'required' => TRUE,
      ),

As $settings['config_directories'] = removes other elements?

Status: Needs review » Needs work

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review
FileSize
1-3-interdiff.txt1.45 KB
2384853.3.patch2.4 KB

Yep you're right :)

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

Just one final question - Do we need to worry this code in terms of security?

+++ b/core/modules/system/system.install
@@ -395,8 +395,9 @@ function system_requirements($phase) {
+    foreach ($GLOBALS['config_directories'] as $type => $directory) {
+      $directories[] = config_get_config_directory($type);
+    }
   }

as this allows any key to be added as directory, what if we add

$config_directories['hack_dir'] = 'staging2'; and we add as directory create any security issue?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

re #6: In order to add a directory you would need to either affect $GLOBALS somehow or have access to settings.php - so I'm pretty sure we are not adding any security issues here.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented

@alexpott explained on IRC, it is more about global variable and if one gained $global access can do much more than hacking this file. So this is good to go.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 2384853.3.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

That test fail looks unrelated and it passes locally.

Test name	Pass	Fail	Exception
CollapsedDrupal\system\Tests\Form\FormTest	651	2	0
Message	Group	Filename	Line	Function	Status
Default value for disabled_container_datetime: expected array ( 'date' => '1978-11-01 10:30:00', 'timezone_type' => 3, 'timezone' => 'Europe/Berlin', ), returned array ( 'date' => '1978-11-01 10:30:00.000000', 'timezone_type' => 3, 'timezone' => 'Europe/Berlin', ).	Other	FormTest.php	534	Drupal\system\Tests\Form\FormTest->testDisabledElements()	
Default value for disabled_container_datetime: expected array ( 'date' => '1978-11-01 10:30:00', 'timezone_type' => 3, 'timezone' => 'Europe/Berlin', ), returned array ( 'date' => '1978-11-01 10:30:00.000000', 'timezone_type' => 3, 'timezone' => 'Europe/Berlin', ).

Status: Needs work » Needs review

alexpott queued 5: 2384853.3.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 5: 2384853.3.patch, failed testing.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Version: 8.1.x-dev » 8.0.x-dev

Status: Needs work » Needs review

alexpott queued 5: 2384853.3.patch for re-testing.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs review » Reviewed & tested by the community

as per #6

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Issue tags: +CMI, +Configuration system
vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Issue summary: View changes
vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Issue summary: View changes
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

This looks straightforward. Committed/pushed to 8.0.x, thanks!

  • catch committed 66fa3fa on 8.0.x 
    Issue #2384853 by alexpott: Both configuration directories have to be...

Status: Fixed » Closed (fixed)

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