-
Notifications
You must be signed in to change notification settings - Fork 12
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
parameters: | ||
level: max | ||
inferPrivatePropertyTypeFromConstructor: true | ||
checkGenericClassInNonGenericObjectType: false |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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); | ||
} | ||
|
||
/** | ||
|
@@ -48,4 +55,14 @@ public function doRun(InputInterface $input, OutputInterface $output): int | |
|
||
return parent::doRun($input, $output); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a comment block. |
||
public function getConfig(): ?Config | ||
{ | ||
return $this->config; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a comment block. |
||
public function setConfig(string $configDir): void | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't it weird, that If so, the same holds for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @adriansuter |
||
{ | ||
$this->config = (new ConfigResolver($configDir))->loadConfig(); | ||
} | ||
} |
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-Console/blob/0.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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} |
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', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Could even go one step further and do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that we should omit from using
Same for setters, here we return the
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 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(...);
} There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,109 @@ | ||
<?php | ||
|
||
l0gicgate marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/** | ||
* 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 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'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's make this variable |
||
|
||
/** @var string|null */ | ||
private $file = null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use a more descriptive name, for example |
||
|
||
/** @var string */ | ||
private $dir; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use a more descriptive name, for example |
||
|
||
public function __construct(string $dir) | ||
{ | ||
$this->dir = $dir; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a comment block. |
||
public function loadConfig(): ?Config | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So we can basically instantiate a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This resolver should only have one public method called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @adriansuter @l0gicgate |
||
{ | ||
$this->locateConfig(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we should do: $this->file = $this->locateConfig(); In the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we handle the exception? |
||
|
||
return $this->loadConfigByFormat(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Here we should do: try {
return $this->attemptCreatingConfigFromEnv();
catch (CannotCreateConfigFromEnvironmentException $e) {
return $this->attemptCreatingConfigFromSupportedFormats();
} |
||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a comment block. |
||
public function getFile(): string | ||
{ | ||
return $this->file ?? ""; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use single quotes. |
||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Let's do this instead, this way the 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this |
||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's use a |
||
$array = require($this->file); | ||
if (is_array($array) === false) { | ||
throw new InvalidArgumentException("The file $this->file should return an array"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's wrap |
||
} | ||
|
||
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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's wrap There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we provide more information to the user, for example, using |
||
} | ||
|
||
return new Config($array, $this->dir); | ||
} | ||
|
||
return null; | ||
} | ||
} |
There was a problem hiding this comment.
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?