Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement config object #13

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion bin/slim
Original file line number Diff line number Diff line change
Expand Up @@ -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__));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


$app = new Application($configDir);

//$app->add(new \Slim\Console\Command\InitCommand());

$app->run();
3 changes: 2 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
"require": {
"php": "^7.2",
"symfony/console": "^5.0",
"symfony/config": "^5.0"
"symfony/config": "^5.0",
"ext-json": "*"
},
"require-dev": {
"adriansuter/php-autoload-override": "^1.0",
Expand Down
1 change: 1 addition & 0 deletions phpstan.neon.dist
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
parameters:
level: max
inferPrivatePropertyTypeFromConstructor: true
checkGenericClassInNonGenericObjectType: false
19 changes: 18 additions & 1 deletion src/Application.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@

namespace Slim\Console;

use Slim\Console\Config\Config;
use Slim\Console\Config\ConfigResolver;
use Symfony\Component\Console\Application as SymfonyApplication;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;
Expand All @@ -19,9 +21,14 @@ class Application extends SymfonyApplication
{
private const VERSION = '0.1';

public function __construct()
/** @var Config|null */
protected $config;

public function __construct(string $configDir)
{
parent::__construct('Slim Console', self::VERSION);

$this->setConfig($configDir);
}

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment block.

public function getConfig(): ?Config
{
return $this->config;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment block.

public function setConfig(string $configDir): void
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @adriansuter

{
$this->config = (new ConfigResolver($configDir))->loadConfig();
}
}
42 changes: 42 additions & 0 deletions src/Command/AbstractCommand.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
<?php

/**
* Slim Framework (https://slimframework.com)
*
* @license https://github.com/slimphp/Slim/blob/4.x/LICENSE.md (MIT License)
*/

declare(strict_types=1);

namespace Slim\Console\Command;

use Slim\Console\Application;
use Slim\Console\Config\Config;
use Symfony\Component\Console\Command\Command as SymfonyCommand;

abstract class AbstractCommand extends SymfonyCommand
{
/**
* @return Config|null
*/
public function getConfig(): ?Config
{
$app = $this->getApplication();
if ($app instanceof Application === false) {
return null;
}

return $app->getConfig();
}

/**
* @param string $configDir
*/
public function setConfig(string $configDir): void
{
$app = $this->getApplication();
if ($app instanceof Application) {
$app->setConfig($configDir);
}
}
Comment on lines +35 to +41
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
173 changes: 173 additions & 0 deletions src/Config/Config.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
<?php

/**
* Slim Framework (https://slimframework.com)
*
* @license https://github.com/slimphp/Slim-Console/blob/0.x/LICENSE.md (MIT License)
*/

declare(strict_types=1);

namespace Slim\Console\Config;

use ArrayAccess;

use function array_key_exists;
use function array_merge;
use function ctype_lower;
use function is_null;
use function preg_replace;
use function strtoupper;

class Config implements ArrayAccess
{
private const ENV_PREFIX = 'SLIM_CONSOLE_';

/**
* @var array<mixed>
*/
private $default = [
'bootstrapDir' => DIRECTORY_SEPARATOR . 'app',
'commandsDir' => DIRECTORY_SEPARATOR . 'Application/Commands',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a DIRECTORY_SEPARATOR between Application and Commands too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'indexDir' => DIRECTORY_SEPARATOR . 'public',
'indexFile' => DIRECTORY_SEPARATOR . 'index.php',
'sourceDir' => DIRECTORY_SEPARATOR . 'src',
];

/**
* @var array<mixed>
*/
private $params = [];

/**
* Config constructor.
* @param array<mixed> $params
* @param string $configDir
*/
public function __construct(array $params = [], string $configDir = '')
{
if ($configDir !== '') {
$this->set('rootDir', $configDir);
}
$this->setAll($params);
}

/**
* @return array<mixed>
*/
public function all(): array
{
return $this->params;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make it fluent and return self.

{
$params = array_merge($this->default, $params);

foreach ($params as $key => $value) {
//Env variables take precedence
$this->params[$key] = $this->getEnvironmentVariableValue((string) $key) ?: $this->get('rootDir') . $value;
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(...);
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.

{
return $this->params[$key] ?? null;
}

/**
* @param string $key
* @param mixed $value
*/
public function set(string $key, $value): void
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make it fluent and return self.

{
$this->params[$key] = $value;
}

/**
* @param string $key
* @return bool
*/
public function has(string $key): bool
{
return isset($this->params[$key]);
}

/**
* @param string $key
*/
public function delete(string $key): void
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make it fluent and return self.

{
unset($this->params[$key]);
}

/**
* @param mixed $key
* @return bool
*/
public function offsetExists($key): bool
{
return $this->has($key);
}

/**
* @param mixed $key
* @return mixed|null
*/
public function offsetGet($key)
{
return $this->get($key);
}

/**
* @param mixed $key
* @param mixed $value
*/
public function offsetSet($key, $value): void
{
$this->set($key, $value);
}

/**
* @param mixed $key
*/
public function offsetUnset($key): void
{
$this->delete($key);
}

/**
* @param string $key
* @return string|null
*/
private function getEnvironmentVariableValue(string $key): ?string
{
//Return nothing for unknown keys
if (array_key_exists($key, $this->default) === false) {
return null;
}

if (ctype_lower($key) === true) {
return strtoupper($key);
}

//Convert CamelCase to Snake_Case
$key = preg_replace('/(.)(?=[A-Z])/u', '$1' . '_', $key);
if (is_null($key)) {
return null;
}

$key = self::ENV_PREFIX . strtoupper($key);
$value = getenv($key);

return $value ?: null;
}
}
101 changes: 101 additions & 0 deletions src/Config/ConfigResolver.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
<?php

l0gicgate marked this conversation as resolved.
Show resolved Hide resolved
namespace Slim\Console\Config;

use InvalidArgumentException;
use ReflectionClass;
use Slim\Console\Exception\ConfigNotFoundException;

use function array_filter;
use function file_exists;
use function file_get_contents;
use function implode;
use function is_array;
use function is_null;
use function json_decode;
use function strpos;

class ConfigResolver
{
public const FORMAT_JSON = 'json';
public const FORMAT_PHP = 'php';
//public const FORMAT_YAML = 'yaml';
private const CONFIG_FILENAME = 'slim-console.config';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's make this variable protected


/** @var string|null */
private $file = null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a more descriptive name, for example configFile.


/** @var string */
private $dir;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a more descriptive name, for example configDirectory.


public function __construct(string $dir)
{
$this->dir = $dir;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment block.

public function loadConfig(): ?Config
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@flangofas flangofas May 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
$this->locateConfig();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we should do:

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we handle the exception?


return $this->loadConfigByFormat();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
}

}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment block.

public function getFile(): string
{
return $this->file ?? "";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use single quotes.

}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment block.

protected function locateConfig(): void
{
$reflection = new ReflectionClass($this);
$filterFormats = function ($value, $key) {
return strpos($key, 'FORMAT') === 0;
};
$formats = array_filter($reflection->getConstants(), $filterFormats, ARRAY_FILTER_USE_BOTH);
Comment on lines +56 to +62
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this is clever, we should instead make use of a regular array that contains the supported formats instead. It'll avoid having to use costly ReflectionClass and unnecessary parsing logic.

Let's do this instead, this way the Config object can be extended and overriden:

protected $supportedFormats = [
    self::FORMAT_PHP,
    self::FORMAT_JSON,
];


foreach ($formats as $possibleFormat) {
$fileName = $this->dir . DIRECTORY_SEPARATOR . self::CONFIG_FILENAME . '.' . $possibleFormat;
if (file_exists($fileName)) {
$this->file = $fileName;

break;
}
}

if (is_null($this->file)) {
throw new ConfigNotFoundException(
'Please create config file, supported formats: ' . implode(',', $formats)
);
}
Comment on lines +73 to +77
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment block.

protected function loadConfigByFormat(): ?Config
{
if (is_null($this->file)) {
return null;
}

$format = pathinfo($this->file, PATHINFO_EXTENSION);

if ($format === self::FORMAT_PHP) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's use a switch statement

$array = require($this->file);
if (is_array($array) === false) {
throw new InvalidArgumentException("The file $this->file should return an array");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

return new Config($array, $this->dir);
}

if ($format === self::FORMAT_JSON) {
$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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

return new Config($array, $this->dir);
}

return null;
}
}
Loading