Implement config object #13

flangofas · 2020-04-30T17:07:11Z

This is the implementation of #6

It can be accessed by the Application or Command Object.

Use cases:

$config = $app->getConfig();
$config = $command->getConfig();

//Default params:
$params = [
    'bootstrapDir'   => 'app',
    'commandsDir'    => 'Application/Commands',
    'indexDir'	     => 'public',
    'indexFile'	     => 'index.php',
    'rootDir'	     => 'Absolute path of the project\'s root directory',
    'sourceDir'	     => 'src',
];

//Retrieve value
$config['commandsDir'];
//OR
$config->get('commandsDir');

Additional params can be stored as follows:

$config['newParam'] = 'value';
//OR
$config->set('newParam') = 'value';

Supported formats

JSON
PHP
Yaml

l0gicgate

This is just a preliminary review, before writing all the tests let's nail down the initial mechanisms.

Thanks for this, great work!

l0gicgate · 2020-04-30T20:52:10Z

src/Command/AbstractCommand.php

+    public function setConfig(string $configDir): void
+    {
+        $app = $this->getApplication();
+        if ($app instanceof Application) {
+            $app->setConfig($configDir);
+        }
+    }


I wonder if we should throw here to indicate to the user that the parent Application does not support this in case this command is being used out of context. Same for the getConfig() method

l0gicgate · 2020-04-30T21:08:02Z

src/Config/Config.php

+     * @param string $key
+     * @return mixed|null
+     */
+    public function get(string $key)


I think that we should omit from using ArrayAccess and use named getters/setters:

getBootstrapDir(): string

getCommandsDir(): ?string

getIndexDir(): string

getIndexFile(): string

getSourceDir(): string

Same for setters, here we return the Config object so we can chain set calls:

setBootstrapDir(string $dir): Config
...

I think that this object should be immutable but extensible. So maybe an interface is necessary. Another point I would like to make is that in no instance should getters return null, as this will create downstream errors. These should all be validated, required parameters. The only parameter that could be null is commandsDir.

So instead of instantiating the configuration file with an array I would do something like:

public function __construct( string $sourceDir, string $bootstrapDir, string $indexDir, string $indexFile, ?string $commandsDir = null, )

Then you could have a static instantiator:

public static function fromArray(array $params): Config { // ... do array validation to ensure params contain required keys ... return new self(...); } public static function fromEnvironment(): Config { // ... do array validation to ensure params contain required keys ... return new self(...); }

src/Config/ConfigResolver.php

src/Exception/ConfigNotFoundException.php

tests/ApplicationTest.php

l0gicgate · 2020-04-30T21:18:45Z

src/Config/ConfigResolver.php

+        if ($format === self::FORMAT_PHP) {
+            $array = require($this->file);
+            if (is_array($array) === false) {
+                throw new InvalidArgumentException("The file $this->file should return an array");


let's wrap $this->file with {$this->file}

l0gicgate · 2020-04-30T21:18:50Z

src/Config/ConfigResolver.php

+            $string = (string) file_get_contents($this->file);
+            $array = json_decode($string, true);
+            if (json_last_error() !== JSON_ERROR_NONE) {
+                throw new InvalidArgumentException("The file $this->file should be a valid JSON");


let's wrap $this->file with {$this->file}

l0gicgate · 2020-04-30T21:25:48Z

src/Config/ConfigResolver.php

+
+    public function loadConfig(): ?Config
+    {
+        $this->locateConfig();


Here we should do:

$this->file = $this->locateConfig();

In the locateConfig(): string method we can instead return early from the proposed switch statement.

l0gicgate · 2020-04-30T21:26:36Z

src/Config/ConfigResolver.php

+        if (is_null($this->file)) {
+            throw new ConfigNotFoundException(
+                'Please create config file, supported formats: ' . implode(',', $formats)
+            );
+        }


this if statement can be removed since we would reach this from not returning early. The ConfigurationNotFoundException still stands though.

l0gicgate · 2020-04-30T21:30:05Z

src/Config/ConfigResolver.php

+    {
+        $this->locateConfig();
+
+        return $this->loadConfigByFormat();


Since ENV variables takes precedence over all other formats here what we should try first is $this->attemptCreatingConfigFromEnv(): Config which would call Config::fromEnvironment() (this method should throw something like CannotCreateConfigFromEnvironmentException. If we catch this, then we proceed to try and locate PHP config then JSON then YAML

Here we should do:

try { return $this->attemptCreatingConfigFromEnv(); catch (CannotCreateConfigFromEnvironmentException $e) { return $this->attemptCreatingConfigFromSupportedFormats(); }

adriansuter · 2020-05-01T13:31:40Z

src/Application.php

+        return $this->config;
+    }
+
+    public function setConfig(string $configDir): void


Isn't it weird, that getConfig returns a Config object while setConfig takes a string? I would suggest to name the method resolveAndLoadConfig which in turn calls setConfig(Config $config).

If so, the same holds for \Slim\Console\Command\AbstractCommand.

I agree with @adriansuter

adriansuter · 2020-05-01T13:43:03Z

src/Config/Config.php

+     */
+    private $default = [
+        'bootstrapDir'  => DIRECTORY_SEPARATOR . 'app',
+        'commandsDir'   => DIRECTORY_SEPARATOR . 'Application/Commands',


Do we need a DIRECTORY_SEPARATOR between Application and Commands too?

For the default we do. For user passed input that’ll be up to them!

adriansuter · 2020-05-01T13:48:11Z

src/Config/Config.php

+     * @param string $key
+     * @return mixed|null
+     */
+    public function get(string $key)


adriansuter · 2020-05-01T13:54:52Z

src/Config/ConfigResolver.php

+        $this->dir = $dir;
+    }
+
+    public function loadConfig(): ?Config


So we can basically instantiate a ConfigResolver and then load the config multiple times? Why should we?

This resolver should only have one public method called resolve which either returns a Config object or throws.

@adriansuter @l0gicgate
Should we disallow to load the config again when it is already loaded via ConfigResolver?

ABGEO · 2020-05-01T13:31:10Z

src/Config/Config.php

+     */
+    private $default = [
+        'bootstrapDir'  => DIRECTORY_SEPARATOR . 'app',
+        'commandsDir'   => DIRECTORY_SEPARATOR . 'Application/Commands',


Please use DIRECTORY_SEPARATOR between Application and Commands too.
And I think src/Commands will be a more semantic place for user commands.

Looking at how a DDD project is normally structured I would say that Application/Commands is better since you’ll typically only have only 3 root level folders being:

Application

Domain

Infrastructure

Could even go one step further and do Application/Console/Commands since in there you would technically want to separate your Console and Web layers.

ABGEO · 2020-05-01T15:55:10Z

src/Application.php

@@ -48,4 +55,14 @@ public function doRun(InputInterface $input, OutputInterface $output): int

        return parent::doRun($input, $output);
    }
+


Please add a comment block.

ABGEO · 2020-05-01T15:55:33Z

src/Application.php

+    {
+        return $this->config;
+    }
+


Please add a comment block.

ABGEO · 2020-05-01T16:02:24Z

bin/slim

@@ -17,5 +17,11 @@ if (file_exists(__DIR__ . '/../../../autoload.php')) {
    require __DIR__ . '/../vendor/autoload.php';
 }

-$app = new Application();
+//Config SHOULD be placed by init command in the project's root directory
+$configDir = dirname(dirname(__DIR__));


In the case where we use Slim Console as a dependency in project, dirname(dirname(__DIR__)) will return the correct project root, but what if we use Console as a global dependency? It will return ~/.composer. Can we make this part dynamic?

ABGEO · 2020-05-01T16:12:09Z

src/Config/Config.php

+     * @param array<mixed> $params
+     * @return void
+     */
+    public function setAll(array $params): void


Please make it fluent and return self.

ABGEO · 2020-05-01T16:18:51Z

src/Config/ConfigResolver.php

+            );
+        }
+    }
+


Please add a comment block.

ABGEO · 2020-05-01T16:24:18Z

src/Config/ConfigResolver.php

+    private $file = null;
+
+    /** @var string */
+    private $dir;


Please use a more descriptive name, for example configDirectory.

ABGEO · 2020-05-01T16:24:41Z

src/Config/ConfigResolver.php

+    private const CONFIG_FILENAME = 'slim-console.config';
+
+    /** @var string|null */
+    private $file = null;


Please use a more descriptive name, for example configFile.

ABGEO · 2020-05-01T16:27:05Z

src/Config/ConfigResolver.php

+
+    public function loadConfig(): ?Config
+    {
+        $this->locateConfig();


Should we handle the exception?

ABGEO · 2020-05-01T16:31:26Z

src/Config/ConfigResolver.php

+            $string = (string) file_get_contents($this->file);
+            $array = json_decode($string, true);
+            if (json_last_error() !== JSON_ERROR_NONE) {
+                throw new InvalidArgumentException("The file $this->file should be a valid JSON");


Should we provide more information to the user, for example, using json_last_error_msg()?

l0gicgate · 2020-05-01T21:15:35Z

@flangofas I created a new draft in #14. I think it's probably an easier starting point since you essentially would have to entirely rewrite this from the reviewers' directions.

Please don't take offense. Thank you for this initial contribution it gave us a good base to see where we wanted to start at least so it wasn't all done for nothing.

flangofas · 2020-05-01T22:27:12Z

@l0gicgate No offense taken, I agree. The decisions were not made and it wasn't quite clear how it is going to be implemented, I should've asked more questions in the first place.

flangofas requested review from l0gicgate and ABGEO April 30, 2020 17:07

Implement config object

999baf3

flangofas force-pushed the add-config-object branch from 7c14298 to 999baf3 Compare April 30, 2020 20:17

l0gicgate requested changes Apr 30, 2020

View reviewed changes

Fixed heading docblockr

b8e9af2

adriansuter requested changes May 1, 2020

View reviewed changes

ABGEO requested changes May 1, 2020

View reviewed changes

l0gicgate mentioned this pull request May 1, 2020

Config Object #14

Merged

3 tasks

l0gicgate closed this May 1, 2020

flangofas deleted the add-config-object branch May 2, 2020 09:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement config object #13

Implement config object #13

flangofas commented Apr 30, 2020 •

edited

Loading

l0gicgate left a comment

l0gicgate Apr 30, 2020

l0gicgate Apr 30, 2020

adriansuter May 1, 2020

l0gicgate Apr 30, 2020

l0gicgate Apr 30, 2020

l0gicgate Apr 30, 2020

l0gicgate Apr 30, 2020

l0gicgate Apr 30, 2020

adriansuter May 1, 2020

l0gicgate May 1, 2020

adriansuter May 1, 2020

l0gicgate May 1, 2020

adriansuter May 1, 2020

adriansuter May 1, 2020

l0gicgate May 1, 2020

flangofas May 1, 2020 •

edited

Loading

ABGEO May 1, 2020

l0gicgate May 1, 2020

ABGEO May 1, 2020

ABGEO May 1, 2020

ABGEO May 1, 2020

ABGEO May 1, 2020

ABGEO May 1, 2020

ABGEO May 1, 2020

ABGEO May 1, 2020

ABGEO May 1, 2020

ABGEO May 1, 2020

l0gicgate commented May 1, 2020

flangofas commented May 1, 2020 •

edited

Loading

		@@ -48,4 +55,14 @@ public function doRun(InputInterface $input, OutputInterface $output): int

		return parent::doRun($input, $output);
		}

Implement config object #13

Implement config object #13

Conversation

flangofas commented Apr 30, 2020 • edited Loading

l0gicgate 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flangofas May 1, 2020 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0gicgate commented May 1, 2020

flangofas commented May 1, 2020 • edited Loading

flangofas commented Apr 30, 2020 •

edited

Loading

flangofas May 1, 2020 •

edited

Loading

flangofas commented May 1, 2020 •

edited

Loading