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.

Sign up for GitHub

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

Support for nested parameters #54

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Support for nested parameters #54

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c36c8a
Add possibility to use nested parameters
lucascourot May 2, 2014
21b0ff1
Update Readme to mention multi-level parameters nesting.
lucascourot May 2, 2014
1a4a9af
Remove alignment of the equal signs.
lucascourot May 2, 2014
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
67 changes: 52 additions & 15 deletions Processor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,19 @@
class Processor
{
private $io;
private $isStarted;

public function __construct(IOInterface $io)
{
$this->io = $io;
$this->io = $io;
$this->isStarted = false;
}

public function processFile(array $config)
{
$config = $this->processConfig($config);

$realFile = $config['file'];
$realFile = $config['file'];
Copy link
Member

Choose a reason for hiding this comment

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

please don't align the = signs. I don't follow this rule in my projects as I find it is a bad one. It forces to introduces useless git conflicts risks each time we add a longer variable name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

$parameterKey = $config['parameter-key'];

$exists = is_file($realFile);
Expand Down Expand Up @@ -70,7 +72,7 @@ private function processConfig(array $config)
}

if (empty($config['dist-file'])) {
$config['dist-file'] = $config['file'].'.dist';
$config['dist-file'] = $config['file'] . '.dist';
}

if (!is_file($config['dist-file'])) {
Expand All @@ -87,7 +89,7 @@ private function processConfig(array $config)
private function processParams(array $config, array $expectedParams, array $actualParams)
{
// Grab values for parameters that were renamed
$renameMap = empty($config['rename-map']) ? array() : (array) $config['rename-map'];
$renameMap = empty($config['rename-map']) ? array() : (array) $config['rename-map'];
$actualParams = array_replace($actualParams, $this->processRenamedValues($renameMap, $actualParams));

$keepOutdatedParams = false;
Expand Down Expand Up @@ -141,27 +143,62 @@ private function getParams(array $expectedParams, array $actualParams)
{
// Simply use the expectedParams value as default for the missing params.
if (!$this->io->isInteractive()) {
return array_replace($expectedParams, $actualParams);
return array_replace_recursive($expectedParams, $actualParams);
Copy link
Member

Choose a reason for hiding this comment

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

this will break if you have a parameter containing an index array and you expect to replace it (a list of trusted proxy IPs for instance)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tried with

            test:
                - one
                - two
                - three

and this actually works for me using composer install --no-interaction.
Or maybe I misunderstood your comment?

Copy link
Member

Choose a reason for hiding this comment

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

It works when you add more elements than previously. It fails if your new array has 2 elements only while there were 3 before (the third one will be kept)

}

$isStarted = false;
$actualParams = $this->provideParams($expectedParams, $actualParams);

foreach ($expectedParams as $key => $message) {
if (array_key_exists($key, $actualParams)) {
return $actualParams;
}

/**
* @param array $expectedParams
* @param array $actualParams
* @param string $parentKeys
*
* @return array
*/
private function provideParams(array $expectedParams, array $actualParams, $parentKeys = '')
{
foreach ($expectedParams as $paramKey => $paramValue) {
if (array_key_exists($paramKey, $actualParams) && !is_array($paramValue)) {
continue;
}

if (!$isStarted) {
$isStarted = true;
$this->io->write('<comment>Some parameters are missing. Please provide them.</comment>');
if (is_array($paramValue)) {
if (!array_key_exists($paramKey, $actualParams)) {
$actualParams[$paramKey] = array();
}

$actualParams[$paramKey] = $this->provideParams($paramValue, $actualParams[$paramKey], $this->getParametersPath($parentKeys, $paramKey));
} else {
if (!$this->isStarted) {
$this->isStarted = true;
$this->io->write('<comment>Some parameters are missing. Please provide them.</comment>');
}

$default = Inline::dump($paramValue);
$parametersPath = $this->getParametersPath($parentKeys, $paramKey);
$value = $this->io->ask(
sprintf('<question>%s</question> (<comment>%s</comment>): ', $parametersPath, $default),
$default
);
$actualParams[$paramKey] = Inline::parse($value);
}

$default = Inline::dump($message);
$value = $this->io->ask(sprintf('<question>%s</question> (<comment>%s</comment>): ', $key, $default), $default);

$actualParams[$key] = Inline::parse($value);
}

return $actualParams;
}

/**
* @param $parentKeys
* @param $key
*
* @return string
*/
private function getParametersPath($parentKeys, $key)
{
return $parentKeys ? $parentKeys . '.' . $key : $key;
Copy link
Member

Choose a reason for hiding this comment

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

this seems wrong to me, because dots are valid in keys (they are even the common convention in Symfony for parameter names). So it is ambiguous

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only for the view in the terminal, I can change it if you have a better suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

the issue is that you can have the following file:

parameters:
    github.test: true
    github:
        something: foobar

This is valid. Using a dot is confusing.

The primary use case for this library was setting the Symfony parameters in the parameters.yml file. In such context, the difference is very important, as the behavior will be

$container->setParameter('github.test', true);
$container->setParameter('github', array('something' => 'foobar'));

Having a confusing UI for the configuration is a very bad idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand. But it's not possible to have multi-level parameters in Symfony parameters.yml file anyway.

Copy link
Member

Choose a reason for hiding this comment

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

this is not about having multi-level parameters (which is not a concept which exists in Symfony anyway, paramaetrs are always a flat structure). It is about having an array value in a parameter, which is totally valid

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I think it is not possible to introduce it without breaking some things (array values for parameters are currently supported by entering them using the inline Yaml syntax when being asked for it)

If you start changing some deep nested array, it probably means you are setting the semantic configuration of bundles rather than setting parameters. And for this, a dedicated tool being aware of the config you are building will probably be a better job (a dist file will not be enough to describe a deep config in a way allowing to provide a good interactive builder)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, you're right. Thanks for your explanations.
However, I work on a small project built on top of the Symfony console component. I thought I could put all my configuration parameters in one yml file, exactly like Behat does http://docs.behat.org/guides/7.config.html. Behat is using nested parameters and it's common to create a behat.yml.dist file to share within the project. For me this is both configuration and parameters. Am I wrong?

Copy link
Member

Choose a reason for hiding this comment

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

the Behat configuration files are not letting you put parameters directly. They are using the semantic configuration system of Symfony

Copy link
Member

Choose a reason for hiding this comment

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

and btw, given that Behat allows importing other configuration files, copying the .dist file to create the local config is not smart. there is a much better way:

# behat.yml
imports:
  - behat.yml.dist # TADA, the default config gets merged with your local one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I thought about the import but I liked the interactive mode of the ParameterHandler. I'll switch to the import then for my personal project. Thanks.

}
}
3 changes: 2 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
This tool allows you to manage your ignored parameters when running a composer
install or update. It works when storing the parameters in a Yaml file under
a single top-level key (named ``parameters`` by default). Other keys are
copied without change.
copied without change. It's obviously possible to nest multi-level parameters
in your Yaml file.

[![Build Status](https://travis-ci.org/Incenteev/ParameterHandler.png)](https://travis-ci.org/Incenteev/ParameterHandler)
[![Code Coverage](https://scrutinizer-ci.com/g/Incenteev/ParameterHandler/badges/coverage.png?s=ea5de28d9764fdcb6a576a41e244c0ac537b3c81)](https://scrutinizer-ci.com/g/Incenteev/ParameterHandler/)
Expand Down
10 changes: 10 additions & 0 deletions Tests/fixtures/testcases/extra_array_keys/dist.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
parameters:
foo: bar
bare:
foo2: existing_foo2
foo3: existing_foo3
foo4: existing_foo4
foo5:
bare2:
- bloup
- glop
6 changes: 6 additions & 0 deletions Tests/fixtures/testcases/extra_array_keys/existing.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# This file is auto-generated during the composer install
parameters:
foo: existing_foo
bare:
foo1: existing_foo1
foo3: existing_foo3
12 changes: 12 additions & 0 deletions Tests/fixtures/testcases/extra_array_keys/expected.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# This file is auto-generated during the composer install
parameters:
foo: existing_foo
bare:
foo2: existing_foo2
foo3: existing_foo3
foo4: existing_foo4
foo5:
bare2:
- bloup
- glop
foo1: existing_foo1
1 change: 1 addition & 0 deletions Tests/fixtures/testcases/extra_array_keys/setup.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
title: Extra deep array keys and preserve existing values