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

Parameter config directive #4

Open
rflavien opened this issue Feb 21, 2018 · 5 comments
Open

Parameter config directive #4

rflavien opened this issue Feb 21, 2018 · 5 comments

Comments

@rflavien
Copy link
Member

rflavien commented Feb 21, 2018

ObjectivePHP RFC: Parameter config directive

  • Version: 1.1.0
  • Date: 2018-02-22
  • Author: Flavien Rodrigues, [email protected]
  • Status: UNDER DISCUSSION

Summary

An app’s config is everything that is likely to vary between deploys (staging, production, developer environments, etc). I suggest, according to 12 factors app, a strict separation of config from code. Config varies substantially across deploys, code does not.

Currently we are able to override the config with a .local.php file. That's a way we can keep.

I suggest to add a feature to the config component, to reach the environment variables.

Proposal

The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD",
"SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be
interpreted as described in RFC 2119.

Using a Environment Directive with default value :

<?php

return [
    new EnvironmentDirective('APP_MODE', 'development');
];

Using an optional Environment Directive :

<?php

return [
    (new EnvironmentDirective('database.host'))->setOptional();
];

$config->get('database.host') instanceof UndefinedDirective; // true if no key `database.host` is not found in the environment

We also need to add another loader ObjectivePHP\Config\Loader\DotenvLoader. It will use vlucas/phpdotenv.

Backward Incompatible Changes

It will not include Incompatible Changes, it's a MINOR enhancement

Proposed ObjectivePHP Version(s)

objective-php/config 1.1.0

RFC Impact

To Components

NO

To Packages

NO

To Workflow

NO

To Configuration

It add this new way to manage changes between environments.

Future Scope

Future scope can be the extension of this directive with SecretParameterDirective that store encrypted value of the parameter.

Proposed Voting Choices

50%+1 majority

Links to external references, discussions, issues or RFCs

@bcerati
Copy link

bcerati commented Feb 21, 2018

That'd be nice to be able to configure easily an Objective application with environment variables.

I suggest to rename ParameterDirective to EnvParam or something like that.

I have a question though, how in your application do you retrieve this kind of config ?

Currently we get them like that: $config->subset(Param::class)->get('APP_MODE')

So for the environments it'll be $config->subset(ParameterDirective::class)->get('APP_MODE')

It will be confusing for the developers. Should i get "Param" or ParameterDirective ?

@rflavien
Copy link
Member Author

rflavien commented Feb 21, 2018

I think it would be better to get things working like :

$config->subset(ApplicationConfiguration::class)->get('APP_MODE');

(Param is not an objective-php/config class and so is not ApplicationConfiguration)

@fanshan
Copy link
Member

fanshan commented Feb 21, 2018

I think parameters must be mandatory by default.

If no default value are provided, it could be possible to make the directive optional :

(new ParameterDirective('database.host'))->optional();

new ParameterDirective('database.host', null) would not be an equivalent.

This is mean if database.host env variable is not defined then the directive database.host will not exist within the application.

Right ?

@rflavien
Copy link
Member Author

@fanshan I agree that it's better to make it mandatory by default with the ->optional() to make it not mandatory.

If database.host is not defined in the env and the directive is not set as optional then it must throw the MissingMandatoryParameter

If database.host is not defined in the env and the directive is set as optional then we must provide a way to be aware of it. It can be something like :

$config->subset(DatabaseConfiguration::class)->get('host') instanceof UndefinedParameter; // this is true in this second case

@rflavien
Copy link
Member Author

I added the ability to load variables from dotenv files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants