Skip to content

Commit

Permalink
feature #33295 [OptionsResolver] Display full nested option hierarchy…
Browse files Browse the repository at this point in the history 
… in exceptions (fancyweb)

This PR was merged into the 4.4 branch.

Discussion
----------

[OptionsResolver] Display full nested option hierarchy in exceptions

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

It kind of improve the DX, especially when you define a lot of nested form options since the file and line cannot be displayed.

```php
$resolver->setDefaults([
    'array' => function (OptionsResolver $arrayResolver): void {
        $arrayResolver->setRequired('foo');
    },
]);

```

Before:
`The required option "foo" is missing.`

After:
`The required option "array[foo]" is missing.`

That can go to 4.3 I guess.

Commits
-------

a981fc3 [OptionsResolver] Display full nested options hierarchy in exceptions
  • Loading branch information
@fabpot
fabpot committed Sep 8, 2019
2 parents 8aa708e + a981fc3 commit e776419
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 21 deletions.
56 changes: 38 additions & 18 deletions src/Symfony/Component/OptionsResolver/OptionsResolver.php
View file
Expand Up @@ -103,6 +103,8 @@ class OptionsResolver implements Options
*/
private $locked = false;

private $parentsOptions = [];

private static $typeAliases = [
'boolean' => 'bool',
'integer' => 'int',
Expand Down Expand Up @@ -423,7 +425,7 @@ public function setDeprecated(string $option, $deprecationMessage = 'The option
}

if (!isset($this->defined[$option])) {
throw new UndefinedOptionsException(sprintf('The option "%s" does not exist, defined options are: "%s".', $option, implode('", "', array_keys($this->defined))));
throw new UndefinedOptionsException(sprintf('The option "%s" does not exist, defined options are: "%s".', $this->formatOptions([$option]), implode('", "', array_keys($this->defined))));
}

if (!\is_string($deprecationMessage) && !$deprecationMessage instanceof \Closure) {
Expand Down Expand Up @@ -481,7 +483,7 @@ public function setNormalizer($option, \Closure $normalizer)
}

if (!isset($this->defined[$option])) {
throw new UndefinedOptionsException(sprintf('The option "%s" does not exist. Defined options are: "%s".', $option, implode('", "', array_keys($this->defined))));
throw new UndefinedOptionsException(sprintf('The option "%s" does not exist. Defined options are: "%s".', $this->formatOptions([$option]), implode('", "', array_keys($this->defined))));
}

$this->normalizers[$option] = [$normalizer];
Expand Down Expand Up @@ -526,7 +528,7 @@ public function addNormalizer(string $option, \Closure $normalizer, bool $forceP
}

if (!isset($this->defined[$option])) {
throw new UndefinedOptionsException(sprintf('The option "%s" does not exist. Defined options are: "%s".', $option, implode('", "', array_keys($this->defined))));
throw new UndefinedOptionsException(sprintf('The option "%s" does not exist. Defined options are: "%s".', $this->formatOptions([$option]), implode('", "', array_keys($this->defined))));
}

if ($forcePrepend) {
Expand Down Expand Up @@ -569,7 +571,7 @@ public function setAllowedValues($option, $allowedValues)
}

if (!isset($this->defined[$option])) {
throw new UndefinedOptionsException(sprintf('The option "%s" does not exist. Defined options are: "%s".', $option, implode('", "', array_keys($this->defined))));
throw new UndefinedOptionsException(sprintf('The option "%s" does not exist. Defined options are: "%s".', $this->formatOptions([$option]), implode('", "', array_keys($this->defined))));
}

$this->allowedValues[$option] = \is_array($allowedValues) ? $allowedValues : [$allowedValues];
Expand Down Expand Up @@ -610,7 +612,7 @@ public function addAllowedValues($option, $allowedValues)
}

if (!isset($this->defined[$option])) {
throw new UndefinedOptionsException(sprintf('The option "%s" does not exist. Defined options are: "%s".', $option, implode('", "', array_keys($this->defined))));
throw new UndefinedOptionsException(sprintf('The option "%s" does not exist. Defined options are: "%s".', $this->formatOptions([$option]), implode('", "', array_keys($this->defined))));
}

if (!\is_array($allowedValues)) {
Expand Down Expand Up @@ -651,7 +653,7 @@ public function setAllowedTypes($option, $allowedTypes)
}

if (!isset($this->defined[$option])) {
throw new UndefinedOptionsException(sprintf('The option "%s" does not exist. Defined options are: "%s".', $option, implode('", "', array_keys($this->defined))));
throw new UndefinedOptionsException(sprintf('The option "%s" does not exist. Defined options are: "%s".', $this->formatOptions([$option]), implode('", "', array_keys($this->defined))));
}

$this->allowedTypes[$option] = (array) $allowedTypes;
Expand Down Expand Up @@ -686,7 +688,7 @@ public function addAllowedTypes($option, $allowedTypes)
}

if (!isset($this->defined[$option])) {
throw new UndefinedOptionsException(sprintf('The option "%s" does not exist. Defined options are: "%s".', $option, implode('", "', array_keys($this->defined))));
throw new UndefinedOptionsException(sprintf('The option "%s" does not exist. Defined options are: "%s".', $this->formatOptions([$option]), implode('", "', array_keys($this->defined))));
}

if (!isset($this->allowedTypes[$option])) {
Expand Down Expand Up @@ -793,7 +795,7 @@ public function resolve(array $options = [])
ksort($clone->defined);
ksort($diff);

throw new UndefinedOptionsException(sprintf((\count($diff) > 1 ? 'The options "%s" do not exist.' : 'The option "%s" does not exist.').' Defined options are: "%s".', implode('", "', array_keys($diff)), implode('", "', array_keys($clone->defined))));
throw new UndefinedOptionsException(sprintf((\count($diff) > 1 ? 'The options "%s" do not exist.' : 'The option "%s" does not exist.').' Defined options are: "%s".', $this->formatOptions(array_keys($diff)), implode('", "', array_keys($clone->defined))));
}

// Override options set by the user
Expand All @@ -809,7 +811,7 @@ public function resolve(array $options = [])
if (\count($diff) > 0) {
ksort($diff);

throw new MissingOptionsException(sprintf(\count($diff) > 1 ? 'The required options "%s" are missing.' : 'The required option "%s" is missing.', implode('", "', array_keys($diff))));
throw new MissingOptionsException(sprintf(\count($diff) > 1 ? 'The required options "%s" are missing.' : 'The required option "%s" is missing.', $this->formatOptions(array_keys($diff))));
}

// Lock the container
Expand Down Expand Up @@ -860,10 +862,10 @@ public function offsetGet($option/*, bool $triggerDeprecation = true*/)
// Check whether the option is set at all
if (!isset($this->defaults[$option]) && !\array_key_exists($option, $this->defaults)) {
if (!isset($this->defined[$option])) {
throw new NoSuchOptionException(sprintf('The option "%s" does not exist. Defined options are: "%s".', $option, implode('", "', array_keys($this->defined))));
throw new NoSuchOptionException(sprintf('The option "%s" does not exist. Defined options are: "%s".', $this->formatOptions([$option]), implode('", "', array_keys($this->defined))));
}

throw new NoSuchOptionException(sprintf('The optional option "%s" has no value set. You should make sure it is set with "isset" before reading it.', $option));
throw new NoSuchOptionException(sprintf('The optional option "%s" has no value set. You should make sure it is set with "isset" before reading it.', $this->formatOptions([$option])));
}

$value = $this->defaults[$option];
Expand All @@ -872,17 +874,19 @@ public function offsetGet($option/*, bool $triggerDeprecation = true*/)
if (isset($this->nested[$option])) {
// If the closure is already being called, we have a cyclic dependency
if (isset($this->calling[$option])) {
throw new OptionDefinitionException(sprintf('The options "%s" have a cyclic dependency.', implode('", "', array_keys($this->calling))));
throw new OptionDefinitionException(sprintf('The options "%s" have a cyclic dependency.', $this->formatOptions(array_keys($this->calling))));
}

if (!\is_array($value)) {
throw new InvalidOptionsException(sprintf('The nested option "%s" with value %s is expected to be of type array, but is of type "%s".', $option, $this->formatValue($value), $this->formatTypeOf($value)));
throw new InvalidOptionsException(sprintf('The nested option "%s" with value %s is expected to be of type array, but is of type "%s".', $this->formatOptions([$option]), $this->formatValue($value), $this->formatTypeOf($value)));
}

// The following section must be protected from cyclic calls.
$this->calling[$option] = true;
try {
$resolver = new self();
$resolver->parentsOptions = $this->parentsOptions;
$resolver->parentsOptions[] = $option;
foreach ($this->nested[$option] as $closure) {
$closure($resolver, $this);
}
Expand All @@ -897,7 +901,7 @@ public function offsetGet($option/*, bool $triggerDeprecation = true*/)
// If the closure is already being called, we have a cyclic
// dependency
if (isset($this->calling[$option])) {
throw new OptionDefinitionException(sprintf('The options "%s" have a cyclic dependency.', implode('", "', array_keys($this->calling))));
throw new OptionDefinitionException(sprintf('The options "%s" have a cyclic dependency.', $this->formatOptions(array_keys($this->calling))));
}

// The following section must be protected from cyclic
Expand Down Expand Up @@ -932,10 +936,10 @@ public function offsetGet($option/*, bool $triggerDeprecation = true*/)
$keys = array_keys($invalidTypes);

if (1 === \count($keys) && '[]' === substr($keys[0], -2)) {
throw new InvalidOptionsException(sprintf('The option "%s" with value %s is expected to be of type "%s", but one of the elements is of type "%s".', $option, $this->formatValue($value), implode('" or "', $this->allowedTypes[$option]), $keys[0]));
throw new InvalidOptionsException(sprintf('The option "%s" with value %s is expected to be of type "%s", but one of the elements is of type "%s".', $this->formatOptions([$option]), $this->formatValue($value), implode('" or "', $this->allowedTypes[$option]), $keys[0]));
}

throw new InvalidOptionsException(sprintf('The option "%s" with value %s is expected to be of type "%s", but is of type "%s".', $option, $this->formatValue($value), implode('" or "', $this->allowedTypes[$option]), implode('|', array_keys($invalidTypes))));
throw new InvalidOptionsException(sprintf('The option "%s" with value %s is expected to be of type "%s", but is of type "%s".', $this->formatOptions([$option]), $this->formatValue($value), implode('" or "', $this->allowedTypes[$option]), implode('|', array_keys($invalidTypes))));
}
}

Expand Down Expand Up @@ -989,7 +993,7 @@ public function offsetGet($option/*, bool $triggerDeprecation = true*/)
if ($deprecationMessage instanceof \Closure) {
// If the closure is already being called, we have a cyclic dependency
if (isset($this->calling[$option])) {
throw new OptionDefinitionException(sprintf('The options "%s" have a cyclic dependency.', implode('", "', array_keys($this->calling))));
throw new OptionDefinitionException(sprintf('The options "%s" have a cyclic dependency.', $this->formatOptions(array_keys($this->calling))));
}

$this->calling[$option] = true;
Expand All @@ -1012,7 +1016,7 @@ public function offsetGet($option/*, bool $triggerDeprecation = true*/)
// If the closure is already being called, we have a cyclic
// dependency
if (isset($this->calling[$option])) {
throw new OptionDefinitionException(sprintf('The options "%s" have a cyclic dependency.', implode('", "', array_keys($this->calling))));
throw new OptionDefinitionException(sprintf('The options "%s" have a cyclic dependency.', $this->formatOptions(array_keys($this->calling))));
}

// The following section must be protected from cyclic
Expand Down Expand Up @@ -1195,4 +1199,20 @@ private function formatValues(array $values): string

return implode(', ', $values);
}

private function formatOptions(array $options): string
{
if ($this->parentsOptions) {
$prefix = array_shift($this->parentsOptions);
if ($this->parentsOptions) {
$prefix .= sprintf('[%s]', implode('][', $this->parentsOptions));
}

$options = array_map(static function (string $option) use ($prefix): string {
return sprintf('%s[%s]', $prefix, $option);
}, $options);
}

return implode('", "', $options);
}
}
6 changes: 3 additions & 3 deletions src/Symfony/Component/OptionsResolver/Tests/OptionsResolverTest.php
View file
Expand Up @@ -1993,7 +1993,7 @@ public function testIsNestedOption()
public function testFailsIfUndefinedNestedOption()
{
$this->expectException('Symfony\Component\OptionsResolver\Exception\UndefinedOptionsException');
$this->expectExceptionMessage('The option "foo" does not exist. Defined options are: "host", "port".');
$this->expectExceptionMessage('The option "database[foo]" does not exist. Defined options are: "host", "port".');
$this->resolver->setDefaults([
'name' => 'default',
'database' => function (OptionsResolver $resolver) {
Expand All @@ -2008,7 +2008,7 @@ public function testFailsIfUndefinedNestedOption()
public function testFailsIfMissingRequiredNestedOption()
{
$this->expectException('Symfony\Component\OptionsResolver\Exception\MissingOptionsException');
$this->expectExceptionMessage('The required option "host" is missing.');
$this->expectExceptionMessage('The required option "database[host]" is missing.');
$this->resolver->setDefaults([
'name' => 'default',
'database' => function (OptionsResolver $resolver) {
Expand All @@ -2023,7 +2023,7 @@ public function testFailsIfMissingRequiredNestedOption()
public function testFailsIfInvalidTypeNestedOption()
{
$this->expectException('Symfony\Component\OptionsResolver\Exception\InvalidOptionsException');
$this->expectExceptionMessage('The option "logging" with value null is expected to be of type "bool", but is of type "NULL".');
$this->expectExceptionMessage('The option "database[logging]" with value null is expected to be of type "bool", but is of type "NULL".');
$this->resolver->setDefaults([
'name' => 'default',
'database' => function (OptionsResolver $resolver) {
Expand Down

0 comments on commit e776419

Please sign in to comment.