Skip to content

Commit

Permalink
bug #28878 [OptionsResolver] Trigger deprecation only if the option i…
Browse files Browse the repository at this point in the history 
…s used (yceruto)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[OptionsResolver] Trigger deprecation only if the option is used

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #28848
| License       | MIT
| Doc PR        | symfony/symfony-docs#10496

It's true that showing a deprecation message when the option is not used is a bit annoying and it would be heavy to get rid of it.

Now, a deprecated option is triggered only when it's provided by the user or each time is being called from a lazy evaluation (except for deprecations based on the value, they're triggered only when provided by the user).

Commits
-------

1af23c9 [OptionsResolver] Trigger deprecation only if the option is used
  • Loading branch information
@fabpot
fabpot committed Oct 24, 2018
2 parents 01dfca1 + 1af23c9 commit 9d3621e
Show file tree
Hide file tree
Showing 3 changed files with 131 additions and 19 deletions.
2 changes: 2 additions & 0 deletions src/Symfony/Component/OptionsResolver/Options.php
View file
Expand Up @@ -16,6 +16,8 @@
*
* @author Bernhard Schussek <bschussek@gmail.com>
* @author Tobias Schultze <http://tobion.de>
*
* @method mixed offsetGet(string $option, bool $triggerDeprecation = true)
*/
interface Options extends \ArrayAccess, \Countable
{
Expand Down
20 changes: 17 additions & 3 deletions src/Symfony/Component/OptionsResolver/OptionsResolver.php
View file
Expand Up @@ -88,6 +88,11 @@ class OptionsResolver implements Options
*/
private $deprecated = array();

/**
* The list of options provided by the user.
*/
private $given = array();

/**
* Whether the instance is locked for reading.
*
Expand Down Expand Up @@ -744,6 +749,7 @@ public function resolve(array $options = array())

// Override options set by the user
foreach ($options as $option => $value) {
$clone->given[$option] = true;
$clone->defaults[$option] = $value;
unset($clone->resolved[$option], $clone->lazy[$option]);
}
Expand Down Expand Up @@ -772,7 +778,8 @@ public function resolve(array $options = array())
/**
* Returns the resolved value of an option.
*
* @param string $option The option name
* @param string $option The option name
* @param bool $triggerDeprecation Whether to trigger the deprecation or not (true by default)
*
* @return mixed The option value
*
Expand All @@ -784,14 +791,20 @@ public function resolve(array $options = array())
* @throws OptionDefinitionException If there is a cyclic dependency between
* lazy options and/or normalizers
*/
public function offsetGet($option)
public function offsetGet($option/*, bool $triggerDeprecation = true*/)
{
if (!$this->locked) {
throw new AccessException('Array access is only supported within closures of lazy options and normalizers.');
}

$triggerDeprecation = 1 === \func_num_args() || \func_get_arg(1);

// Shortcut for resolved options
if (array_key_exists($option, $this->resolved)) {
if ($triggerDeprecation && isset($this->deprecated[$option]) && \is_string($this->deprecated[$option])) {
@trigger_error(strtr($this->deprecated[$option], array('%name%' => $option)), E_USER_DEPRECATED);
}

return $this->resolved[$option];
}

Expand Down Expand Up @@ -920,7 +933,8 @@ public function offsetGet($option)
}

// Check whether the option is deprecated
if (isset($this->deprecated[$option])) {
// and it is provided by the user or is being called from a lazy evaluation
if ($triggerDeprecation && isset($this->deprecated[$option]) && (isset($this->given[$option]) || ($this->calling && \is_string($this->deprecated[$option])))) {
$deprecationMessage = $this->deprecated[$option];

if ($deprecationMessage instanceof \Closure) {
Expand Down
128 changes: 112 additions & 16 deletions src/Symfony/Component/OptionsResolver/Tests/OptionsResolverTest.php
View file
Expand Up @@ -491,12 +491,12 @@ public function testSetDeprecatedFailsIfInvalidDeprecationMessageType()
public function testLazyDeprecationFailsIfInvalidDeprecationMessageType()
{
$this->resolver
->setDefault('foo', true)
->setDefined('foo')
->setDeprecated('foo', function (Options $options, $value) {
return false;
})
;
$this->resolver->resolve();
$this->resolver->resolve(array('foo' => null));
}

/**
Expand All @@ -506,16 +506,15 @@ public function testLazyDeprecationFailsIfInvalidDeprecationMessageType()
public function testFailsIfCyclicDependencyBetweenDeprecation()
{
$this->resolver
->setDefault('foo', null)
->setDefault('bar', null)
->setDefined(array('foo', 'bar'))
->setDeprecated('foo', function (Options $options, $value) {
$options['bar'];
})
->setDeprecated('bar', function (Options $options, $value) {
$options['foo'];
})
;
$this->resolver->resolve();
$this->resolver->resolve(array('foo' => null, 'bar' => null));
}

public function testIsDeprecated()
Expand All @@ -539,10 +538,15 @@ public function testIsNotDeprecatedIfEmptyString()
/**
* @dataProvider provideDeprecationData
*/
public function testDeprecationMessages(\Closure $configureOptions, array $options, ?array $expectedError)
public function testDeprecationMessages(\Closure $configureOptions, array $options, ?array $expectedError, int $expectedCount)
{
$count = 0;
error_clear_last();
set_error_handler(function () { return false; });
set_error_handler(function () use (&$count) {
++$count;

return false;
});
$e = error_reporting(0);

$configureOptions($this->resolver);
Expand All @@ -555,6 +559,7 @@ public function testDeprecationMessages(\Closure $configureOptions, array $optio
unset($lastError['file'], $lastError['line']);

$this->assertSame($expectedError, $lastError);
$this->assertSame($expectedCount, $count);
}

public function provideDeprecationData()
Expand All @@ -571,6 +576,7 @@ function (OptionsResolver $resolver) {
'type' => E_USER_DEPRECATED,
'message' => 'The option "foo" is deprecated.',
),
1,
);

yield 'It deprecates an option with custom message' => array(
Expand All @@ -588,20 +594,27 @@ function (OptionsResolver $resolver) {
'type' => E_USER_DEPRECATED,
'message' => 'The option "foo" is deprecated, use "bar" option instead.',
),
2,
);

yield 'It deprecates a missing option with default value' => array(
yield 'It deprecates an option evaluated in another definition' => array(
function (OptionsResolver $resolver) {
// defined by superclass
$resolver
->setDefaults(array('foo' => null, 'bar' => null))
->setDefault('foo', null)
->setDeprecated('foo')
;
// defined by subclass
$resolver->setDefault('bar', function (Options $options) {
return $options['foo']; // It triggers a deprecation
});
},
array('bar' => 'baz'),
array(),
array(
'type' => E_USER_DEPRECATED,
'message' => 'The option "foo" is deprecated.',
),
1,
);

yield 'It deprecates allowed type and value' => array(
Expand All @@ -623,20 +636,46 @@ function (OptionsResolver $resolver) {
'type' => E_USER_DEPRECATED,
'message' => 'Passing an instance of "stdClass" to option "foo" is deprecated, pass its FQCN instead.',
),
1,
);

yield 'It ignores deprecation for missing option without default value' => array(
yield 'It triggers a deprecation based on the value only if option is provided by the user' => array(
function (OptionsResolver $resolver) {
$resolver
->setDefined(array('foo', 'bar'))
->setDeprecated('foo')
->setDefined('foo')
->setAllowedTypes('foo', array('null', 'bool'))
->setDeprecated('foo', function (Options $options, $value) {
if (!\is_bool($value)) {
return 'Passing a value different than true or false is deprecated.';
}

return '';
})
->setDefault('baz', null)
->setAllowedTypes('baz', array('null', 'int'))
->setDeprecated('baz', function (Options $options, $value) {
if (!\is_int($value)) {
return 'Not passing an integer is deprecated.';
}

return '';
})
->setDefault('bar', function (Options $options) {
$options['baz']; // It does not triggers a deprecation

return $options['foo']; // It does not triggers a deprecation
})
;
},
array('bar' => 'baz'),
null,
array('foo' => null), // It triggers a deprecation
array(
'type' => E_USER_DEPRECATED,
'message' => 'Passing a value different than true or false is deprecated.',
),
1,
);

yield 'It ignores deprecation if closure returns an empty string' => array(
yield 'It ignores a deprecation if closure returns an empty string' => array(
function (OptionsResolver $resolver) {
$resolver
->setDefault('foo', null)
Expand All @@ -647,6 +686,7 @@ function (OptionsResolver $resolver) {
},
array('foo' => Bar::class),
null,
0,
);

yield 'It deprecates value depending on other option value' => array(
Expand All @@ -668,6 +708,62 @@ function (OptionsResolver $resolver) {
'type' => E_USER_DEPRECATED,
'message' => 'Using the "date_format" option when the "widget" option is set to "single_text" is deprecated.',
),
1,
);

yield 'It triggers a deprecation for each evaluation' => array(
function (OptionsResolver $resolver) {
$resolver
// defined by superclass
->setDefined('foo')
->setDeprecated('foo')
// defined by subclass
->setDefault('bar', function (Options $options) {
return $options['foo']; // It triggers a deprecation
})
->setNormalizer('bar', function (Options $options, $value) {
$options['foo']; // It triggers a deprecation
$options['foo']; // It triggers a deprecation

return $value;
})
;
},
array('foo' => 'baz'), // It triggers a deprecation
array(
'type' => E_USER_DEPRECATED,
'message' => 'The option "foo" is deprecated.',
),
4,
);

yield 'It ignores a deprecation if no option is provided by the user' => array(
function (OptionsResolver $resolver) {
$resolver
->setDefined('foo')
->setDefault('bar', null)
->setDeprecated('foo')
->setDeprecated('bar')
;
},
array(),
null,
0,
);

yield 'It explicitly ignores a depreciation' => array(
function (OptionsResolver $resolver) {
$resolver
->setDefault('foo', null)
->setDeprecated('foo')
->setDefault('bar', function (Options $options) {
return $options->offsetGet('foo', false);
})
;
},
array(),
null,
0,
);
}

Expand Down

0 comments on commit 9d3621e

Please sign in to comment.