OPENEUROPA-1149: Provide alternative method to override settings in settings.php #82

sergepavle · 2018-09-26T14:33:29Z

No description provided.

ademarco

Good start and nice alternative approach @sergepavle ! I've left some comments and we need to fix / add tests.

ademarco · 2018-09-26T17:25:49Z

src/Commands/DrupalCommands.php

@@ -282,8 +282,20 @@ public function settingsSetup(array $options = [
      'root' => InputOption::VALUE_REQUIRED,
    ])
    {
+        $settings_file = $options['root'].'/sites/default/settings.php';
+        $settings_local_file = $options['root'] . '/sites/default/' . $this->getConfig()->get('drupal.settings_local_file');


The site path /sites/default/ is actually configurable, we should get it from the runner configuration.

ademarco · 2018-09-26T17:27:12Z

config/runner.yml

@@ -22,6 +22,7 @@ drupal:
  root: "build"
  root_absolute: ~
  base_url: "http://127.0.0.1:8888"
+  settings_local_file: 'settings.local.php'


Drupal core already uses settings.local.php shall we maybe rename this as below?

settings_override_file: 'settings.override.php'

ademarco · 2018-09-26T17:28:31Z

src/Commands/DrupalCommands.php

        return $this->collectionBuilder()->addTaskList([
-            $this->taskAppendConfiguration($options['root'].'/sites/default/default.settings.php', $this->getConfig())->setConfigKey('drupal.settings'),
+            $this->taskFilesystemStack()->copy($options['root'] . '/sites/default/default.settings.php', $settings_file),


We need to set the third parameter $force to TRUE otherwise the runner will not override an already existing settings.php.

ademarco · 2018-09-27T11:02:04Z

config/runner.yml

@@ -22,6 +22,7 @@ drupal:
  root: "build"
  root_absolute: ~
  base_url: "http://127.0.0.1:8888"
+  settings_local_file: 'settings.override.php'


Let's also pass this one as an overridable parameter to the command. We should also rename it in settings_override_file

Done: https://github.com/openeuropa/task-runner/pull/82/files#diff-9a1d65d4e37665bfc2f924f6cc5c8c4e

ademarco · 2018-09-28T08:17:50Z

src/Commands/DrupalCommands.php

        return $this->collectionBuilder()->addTaskList([
-            $this->taskAppendConfiguration($options['root'].'/sites/default/default.settings.php', $this->getConfig())->setConfigKey('drupal.settings'),
+            $this->taskFilesystemStack()->copy($options['root'] . '/sites/default/default.settings.php', $settings_file, true),


$options['sites-subdir'] should replace /default/ on these two lines above too.

ademarco · 2018-09-28T08:22:14Z

src/Commands/DrupalCommands.php

+                '<?php',
+                '',
+            ]),
+            $this->taskAppendConfiguration($settings_override_path, $this->getConfig())->setConfigKey('drupal.settings'),


Use taskWriteConfiguration instead of taskAppendConfiguration and you can remove the lines initializing the file above:

$this->taskWriteToFile($settings_override_path)->lines([ '<?php', '', ]),

The taskWriteConfiguration takes care of inserting a <?php\n already.

ademarco · 2018-09-28T08:27:22Z

src/Commands/DrupalCommands.php

+                'if (file_exists($app_root . \'/\' . $site_path . \'/' . $settings_override_filename . '\')) {',
+                '  include $app_root . \'/\' . $site_path . \'/' . $settings_override_filename . '\';',
+                '}'
+            ]),


Can we wrap these strings in double quotes, so we don't have to escape and concatenate things? Something like the following is more readable:

$this->taskWriteToFile($settings_file)->append()->lines([ "if (file_exists(\$app_root . '/' . \$site_path . '/$settings_override_filename')) {", " include \$app_root . '/' . \$site_path . '/$settings_override_filename';", "}", ]),

ademarco · 2018-09-28T10:05:33Z

@voidtek we also need to update the command comment:

    /**
     * Setup default Drupal settings file.
     *
     * This command will append settings specified at "drupal.settings" to the
     * current site's "default.settings.php" which, in turn, will be used
     * to generate the actual "settings.php" at installation time.
     *

ademarco · 2018-09-28T10:42:11Z

src/Commands/DrupalCommands.php

        return $this->collectionBuilder()->addTaskList([
-            $this->taskAppendConfiguration($options['root'].'/sites/default/default.settings.php', $this->getConfig())->setConfigKey('drupal.settings'),
+            $this->taskFilesystemStack()->copy($settings_default_path, $settings_path, true),


Let's make the force configurable (last parameter) by passing a parameter to the command (so no configuration value in this case), and let's default it to false, since if we force it here we wipe out the whole settings.php file which will force a full site reinstall in order to have it regenerated by Drupal.

ademarco · 2018-09-28T14:53:53Z

config/commands/drupal.yml

@@ -31,4 +31,4 @@ command:
        options:
          sites-subdir: ${drupal.site.sites_subdir}
          settings-override-file: ${drupal.site.settings_override_file}
-          force-override-file: ${drupal.site.force_override_file}
+          force: ${drupal.site.force}


We don't need this here since the default value is set directly in the method function signature to false.

ademarco · 2018-09-28T14:55:10Z

src/Commands/DrupalCommands.php


        return $this->collectionBuilder()->addTaskList([
-            $this->taskFilesystemStack()->copy($settings_default_path, $settings_path, true),
+            $this->taskFilesystemStack()->copy($settings_default_path, $settings_path, ($options['force'] === true) ? $options['force'] : false),


Do we need this?

($options['force'] === true) ? $options['force'] : false

Isn't enough to use $options['force']?

I've changed to (bool) $options['force'] .

voidtek

👍

…s in default.settings.php file.

…additional option for settings-setup command.

…ride file is properly included.

voidtek

👍

ademarco requested changes Sep 26, 2018

View reviewed changes

sergepavle force-pushed the OPENEUROPA-1149 branch 2 times, most recently from 4345413 to b24f321 Compare September 27, 2018 08:27

ademarco reviewed Sep 27, 2018

View reviewed changes

sergepavle force-pushed the OPENEUROPA-1149 branch from 60ed986 to 05e26e2 Compare September 28, 2018 07:46

ademarco requested changes Sep 28, 2018

View reviewed changes

ademarco changed the title ~~OPENEUROPA-1149 implementing workaround for avoiding writing settings…~~ OPENEUROPA-1149: Provide alternative method to override settings in settings.php Sep 28, 2018

ademarco requested changes Sep 28, 2018

View reviewed changes

voidtek force-pushed the OPENEUROPA-1149 branch from 8012f34 to d371202 Compare September 28, 2018 12:49

ademarco requested changes Sep 28, 2018

View reviewed changes

voidtek force-pushed the OPENEUROPA-1149 branch 2 times, most recently from 63f4048 to 478abc7 Compare October 1, 2018 09:48

voidtek previously approved these changes Oct 1, 2018

View reviewed changes

drupol dismissed voidtek’s stale review via 3a9d24e October 1, 2018 12:30

drupol force-pushed the OPENEUROPA-1149 branch 3 times, most recently from 0ff3482 to 8fab957 Compare October 1, 2018 12:36

OPENEUROPA-1149: Implementing workaround for avoiding writing setting…

56fb495

…s in default.settings.php file.

drupol force-pushed the OPENEUROPA-1149 branch from 8fab957 to fc56405 Compare October 1, 2018 12:37

sergepavle and others added 10 commits October 1, 2018 14:37

OPENEUROPA-1149: Make included additional settings file configurable.

a98ce50

OPENEUROPA-1149: Correct notes according to notes in PR.

9455a21

OPENEUROPA-1149: Change in task runner command class.

d447b3d

OPENEUROPA-1149: Changes in task runner configs.

c78584b

OPENEUROPA-1149: Minor code amend for DrupalCommand.

d29e8e2

OPENEUROPA-1149: Change structure of task runner config file and add …

c2e4dd5

…additional option for settings-setup command.

OPENEUROPA-1149: Apply additional options in settingsSetup method.

2832d19

OPENEUROPA-1149: Prepare files for phpunit.

549a5cd

OPENEUROPA-1149: Change configurations for unittests.

c6e51f7

OPENEUROPA-1149: Fix code issue in DrupalCommands class.

8bd6151

sergepavle and others added 4 commits October 1, 2018 14:37

OPENEUROPA-1149: Fix code sniffer issues.

a04cb6a

OPENEUROPA-1149: Fix Composer issue with --prefer-lowest on Drone.

04172b7

OPENEUROPA-1149: Implement all remarks and fix issues.

e008ddf

OPENEUROPA-1149: Update command description.

63cfe78

drupol force-pushed the OPENEUROPA-1149 branch from fc56405 to b9ddd27 Compare October 1, 2018 12:37

voidtek and others added 8 commits October 1, 2018 14:39

OPENEUROPA-1149: Add the option force to override the setting.php.

7798fb3

OPENEUROPA-1149: Change logic for the force and add test.

c08a52f

OPENEUROPA-1149: Resolve file permissions issues.

5c0715c

OPENEUROPA-1149: Fix configuration issues.

e9659ee

OPENEUROPA-1149: Fix check for sites_subdir definition.

72295b6

OPENEUROPA-1149: Improve force option processing.

f663bcb

OPENEUROPA-1149: Minor code improvements.

bbddc87

OPENEUROPA-1149: Update the logic of the force option.

2a9c672

drupol force-pushed the OPENEUROPA-1149 branch 8 times, most recently from e81cf3d to f0f9f5a Compare October 1, 2018 14:19

drupol added 2 commits October 1, 2018 16:29

OPENEUROPA-1149: Update the syntax and add documentation block.

6822357

OPENEUROPA-1149: Update the tests to make sure that the settings over…

e636f9e

…ride file is properly included.

drupol force-pushed the OPENEUROPA-1149 branch from f0f9f5a to e636f9e Compare October 1, 2018 14:29

OPENEUROPA-1149: Update generated comment.

4fe0890

ademarco approved these changes Oct 1, 2018

View reviewed changes

voidtek approved these changes Oct 1, 2018

View reviewed changes

voidtek merged commit 77372d2 into master Oct 1, 2018

voidtek deleted the OPENEUROPA-1149 branch October 1, 2018 15:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OPENEUROPA-1149: Provide alternative method to override settings in settings.php #82

OPENEUROPA-1149: Provide alternative method to override settings in settings.php #82

sergepavle commented Sep 26, 2018 •

edited by ademarco

Loading

ademarco left a comment

ademarco Sep 26, 2018

ademarco Sep 26, 2018

ademarco Sep 26, 2018

ademarco Sep 27, 2018 •

edited

Loading

sergepavle Sep 27, 2018

ademarco Sep 28, 2018

ademarco Sep 28, 2018

ademarco Sep 28, 2018

ademarco commented Sep 28, 2018

ademarco Sep 28, 2018

ademarco Sep 28, 2018

ademarco Sep 28, 2018

dxvargas Sep 28, 2018

voidtek left a comment

voidtek left a comment

OPENEUROPA-1149: Provide alternative method to override settings in settings.php #82

OPENEUROPA-1149: Provide alternative method to override settings in settings.php #82

Conversation

sergepavle commented Sep 26, 2018 • edited by ademarco Loading

ademarco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ademarco Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ademarco commented Sep 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voidtek left a comment

Choose a reason for hiding this comment

voidtek left a comment

Choose a reason for hiding this comment

sergepavle commented Sep 26, 2018 •

edited by ademarco

Loading

ademarco Sep 27, 2018 •

edited

Loading