Merge pull request #85 from openeuropa/OPENEUROPA-1271

OPENEUROPA-1271: Valueless options are ignored when put into configuration
openeuropa · Oct 12, 2018 · 6f84f95 · 6f84f95
2 parents f36a890 + 65b3b2c
commit 6f84f95
Show file tree

Hide file tree

Showing 6 changed files with 140 additions and 27 deletions.
diff --git a/config/commands/drupal.yml b/config/commands/drupal.yml
@@ -24,7 +24,6 @@ command:
         database-password: ${drupal.database.password}
         sites-subdir: ${drupal.site.sites_subdir}
         config-dir: ${drupal.site.config_dir}
-        skip-permissions-setup: ${drupal.site.skip_permissions_setup}
     drush-setup:
       options:
         config-dir: ${drupal.root}/drush
@@ -33,7 +32,6 @@ command:
         sites-subdir: ${drupal.site.sites_subdir}
         settings-override-file: ${drupal.site.settings_override_file}
         force: ${drupal.site.force}
-        skip-permissions-setup: ${drupal.site.skip_permissions_setup}
     permissions-setup:
       options:
         sites-subdir: ${drupal.site.sites_subdir}
diff --git a/src/Commands/AbstractCommands.php b/src/Commands/AbstractCommands.php
@@ -2,15 +2,17 @@
 
 namespace OpenEuropa\TaskRunner\Commands;
 
+use Consolidation\AnnotatedCommand\AnnotationData;
 use Robo\Common\ConfigAwareTrait;
 use Robo\Common\IO;
+use Robo\Contract\BuilderAwareInterface;
 use Robo\Contract\ConfigAwareInterface;
 use Robo\Contract\IOAwareInterface;
-use Robo\Contract\BuilderAwareInterface;
 use Robo\Exception\TaskException;
 use Robo\LoadAllTasks;
 use Robo\Robo;
 use Symfony\Component\Console\Event\ConsoleCommandEvent;
+use Symfony\Component\Console\Input\InputInterface;
 
 /**
  * Class AbstractCommands
@@ -47,6 +49,64 @@ public function initializeRuntimeConfiguration(ConsoleCommandEvent $event)
         Robo::loadConfiguration([$this->getConfigurationFile()], $this->getConfig());
     }
 
+    /**
+     * Returns an array of valueless options and their corresponding config key.
+     *
+     * This provides the data for ::initializeValuelessOptions().
+     *
+     * @see \OpenEuropa\TaskRunner\Commands\AbstractCommands::initializeValuelessOptions()
+     *
+     * @return array
+     *   An associative array, keyed by command name, with each value consisting
+     *   of an associative array where the key is the valueless option name, and
+     *   the value is the location where the option should be placed in the
+     *   configuration array. Example:
+     *   'drupal:my-command' => ['my-option' => 'drupal.site.my_option']
+     */
+    public function getValuelessConfigurationKeys()
+    {
+        return [];
+    }
+
+    /**
+     * Initializes valueless options.
+     *
+     * Valueless options (i.e. options of type `InputOption::VALUE_NONE`) cannot
+     * be defined in the usual way in a YAML configuration file such as
+     * `drupal.yml` since the library which is responsible for value
+     * substitution in config files only does straight string replacement. It
+     * does not support the notion that the absence of a valueless option means
+     * that it should be set to `FALSE`.
+     *
+     * This method uses the data from `::getValuelessConfigurationKeys()` to
+     * look up whether the option is present in the specified location in the
+     * configuration files, and will set the value to FALSE if the option is
+     * missing. If the option is present it will use its actual value.
+     *
+     * If the valueless option is passed on the command line this will take
+     * precedence over the value in the configuration files.
+     *
+     * @hook init *
+     */
+    public function initializeValuelessOptions(InputInterface $input, AnnotationData $annotationData)
+    {
+        $command_name = $annotationData->get('command');
+        $keys = $this->getValuelessConfigurationKeys();
+        if (!array_key_exists($command_name, $keys)) {
+            return;
+        }
+        foreach ($keys[$command_name] as $option => $key) {
+            // Check if the option was passed on the command line. This takes
+            // precedence over the presence of the value in the configuration
+            // file.
+            if (!$input->getOption($option)) {
+                // If not, take the corresponding value from the configuration.
+                $value = (bool) $this->getConfig()->get($key);
+                $input->setOption($option, $value);
+            }
+        }
+    }
+
     /**
      * @param  string $name
      * @return string

diff --git a/src/Commands/DrupalCommands.php b/src/Commands/DrupalCommands.php
@@ -32,6 +32,18 @@ public function getConfigurationFile()
         return __DIR__.'/../../config/commands/drupal.yml';
     }
 
+    /**
+     * {@inheritdoc}
+     */
+    public function getValuelessConfigurationKeys()
+    {
+        return [
+            'drupal:site-install' => [
+                'skip-permissions-setup' => 'drupal.site.skip_permissions_setup',
+            ],
+        ];
+    }
+
     /**
      * Set runtime configuration values.
      *
@@ -87,24 +99,25 @@ public function validateSiteInstall(CommandData $commandData)
      *
      * @command drupal:site-install
      *
-     * @option root              Drupal root.
-     * @option site-name         Site name.
-     * @option site-mail         Site mail.
-     * @option site-profile      Installation profile
-     * @option site-update       Whereas to enable the update module or not.
-     * @option site-locale       Default site locale.
-     * @option account-name      Admin account name.
-     * @option account-password  Admin account password.
-     * @option account-mail      Admin email.
-     * @option database-type     Deprecated, use "database-scheme"
-     * @option database-scheme   Database scheme.
-     * @option database-host     Database host.
-     * @option database-port     Database port.
-     * @option database-name     Database name.
-     * @option database-user     Database username.
-     * @option database-password Database password.
-     * @option sites-subdir      Sites sub-directory.
-     * @option config-dir        Config export directory.
+     * @option root                   Drupal root.
+     * @option site-name              Site name.
+     * @option site-mail              Site mail.
+     * @option site-profile           Installation profile
+     * @option site-update            Whereas to enable the update module or not.
+     * @option site-locale            Default site locale.
+     * @option account-name           Admin account name.
+     * @option account-password       Admin account password.
+     * @option account-mail           Admin email.
+     * @option database-type          Deprecated, use "database-scheme"
+     * @option database-scheme        Database scheme.
+     * @option database-host          Database host.
+     * @option database-port          Database port.
+     * @option database-name          Database name.
+     * @option database-user          Database username.
+     * @option database-password      Database password.
+     * @option sites-subdir           Sites sub-directory.
+     * @option config-dir             Config export directory.
+     * @option skip-permissions-setup Whether to skip making the settings file and folder writable during installation.
      *
      * @aliases drupal:si,dsi
      *

diff --git a/tests/AbstractTest.php b/tests/AbstractTest.php
@@ -20,6 +20,7 @@ abstract class AbstractTest extends TestCase
     protected function setUp()
     {
         $filesystem = new Filesystem();
+        $filesystem->chmod($this->getSandboxRoot(), 0777, umask(), true);
         $filesystem->remove(glob($this->getSandboxRoot()."/*"));
         date_default_timezone_set('Europe/London');
     }

diff --git a/tests/Commands/DrupalCommandsTest.php b/tests/Commands/DrupalCommandsTest.php
@@ -17,10 +17,22 @@ class DrupalCommandsTest extends AbstractTest
 {
     /**
      * @param array $config
+     *   The configuration array to pass to the command.
+     * @param string $command
+     *   The command to execute.
+     * @param bool $expected_error
+     *   Whether or not it is expected that the command will return an error
+     *   code.
+     * @param string $expected_settings_dir_permission
+     *   A string representing the octal permission number that is expected to
+     *   be applied on the settings folder.
+     * @param string $expected_settings_file_permission
+     *   A string representing the octal permission number that is expected to
+     *   be applied on the settings file.
      *
      * @dataProvider drupalSettingsDataProvider
      */
-    public function testPermissions(array $config)
+    public function testPermissions(array $config, $command, $expected_error, $expected_settings_dir_permission, $expected_settings_file_permission)
     {
         $configFile = $this->getSandboxFilepath('runner.yml');
         file_put_contents($configFile, Yaml::dump($config));
@@ -32,20 +44,28 @@ public function testPermissions(array $config)
         // Prepare site settings file.
         $siteSettings = $sitesSubdir . 'settings.php';
         touch($siteSettings);
-        chmod($siteSettings, 0777);
+
+        // Make the settings folder and file unwritable so we can detect whether
+        // the exception is thrown in `DrupalCommands::validateSiteInstall()` as
+        // well as whether the permissions are correctly set.
+        chmod($siteSettings, 0444);
+        chmod($sitesSubdir, 0555);
 
         // Run command.
-        $input = new StringInput("drupal:permissions-setup --working-dir=" . $this->getSandboxRoot());
+        $input = new StringInput("$command --working-dir=" . $this->getSandboxRoot());
         $runner = new TaskRunner($input, new BufferedOutput(), $this->getClassLoader());
-        $runner->run();
+        $exit_code = $runner->run();
+
+        // Check if an error is returned when this is expected.
+        $this->assertEquals($expected_error, $exit_code != 0);
 
         // Check site directory.
         $sitesSubdirPermissions = substr(sprintf('%o', fileperms($sitesSubdir)), -4);
-        $this->assertEquals('0775', $sitesSubdirPermissions);
+        $this->assertEquals($expected_settings_dir_permission, $sitesSubdirPermissions);
 
         // Check site settings file.
         $siteSettingsPermissions = substr(sprintf('%o', fileperms($siteSettings)), -4);
-        $this->assertEquals('0664', $siteSettingsPermissions);
+        $this->assertEquals($expected_settings_file_permission, $siteSettingsPermissions);
     }
 
     /**

diff --git a/tests/fixtures/commands/drupal-site-install.yml b/tests/fixtures/commands/drupal-site-install.yml
@@ -1,5 +1,26 @@
+# When the drupal:permissions-setup command is run it should make the settings
+# folder and file writable, and there should not be any error.
 - configuration:
     drupal:
       settings:
         sites-subdir: "build/sites/default/"
         settings_file: "settings.php"
+  command: "drupal:permissions-setup"
+  expected_error: false
+  expected_settings_dir_permission: "0775"
+  expected_settings_file_permission: "0664"
+
+# When the drupal:site-install command is run with the 'skip-permission-setup'
+# option set to 'true' then the settings folder and file should not be made
+# writable, and an error should be returned.
+- configuration:
+    drupal:
+      settings:
+        sites-subdir: "build/sites/default/"
+        settings_file: "settings.php"
+      site:
+        skip_permissions_setup: true
+  command: "drupal:site-install"
+  expected_error: true
+  expected_settings_dir_permission: "0555"
+  expected_settings_file_permission: "0444"