diogo/symfony
Archived
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

merged branch Tobion/optionsresolver-optim (PR #4388)

Browse Source 
Commits
-------

bad4a1f [OptionsResolver] CS fix in LazyOption
a54ea1b [OptionsResolver] small optimization in Options class
104dcf2 [OptionsResolver] fixed bugs concerning required options
1bfcff4 [OptionsResolver] added failing test cases to demonstrate two bugs
37a3a29 [OptionsResolver] optimized validation

Discussion
----------

[OptionsResolver] fixed two bugs and applied optimization

The first commit optimizes the validation in OptionsResolver by removing several unneeded method calls (without changing anything semantically).

Then I recognized two bugs in the current code that I wrote failing test cases for in the second commit.
1. setAllowedValues wrongly validated missing options
2. required options with defaults were considered missing by `resolve` (contrary to the `isRequired` method)

The third commit fixes these bugs.

The forth commit applies a small optimization in Options and uses a static function call for a static function.

---------------------------------------------------------------------------

by travisbot at 2012-05-24T03:39:34Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1418785) (merged a54ea1b6 into b07fb3c4).

---------------------------------------------------------------------------

by travisbot at 2012-05-24T05:22:33Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1419232) (merged bad4a1f7 into b07fb3c4).

---------------------------------------------------------------------------

by bschussek at 2012-05-24T06:20:02Z

I just tested this on my machine, and static calls are a tiny bit faster here, although this is really irrelevant for practical use. Even though I dislike useless micro-optimizations like this, I'm ok with this PR in general.

---------------------------------------------------------------------------

by Tobion at 2012-05-24T13:23:11Z

I didn't say that's an optimization in the first place. (The optimization was the removal of a variable assignment)
I just changed it because in other PRs I've been told, static functions should be called in a static way.

---------------------------------------------------------------------------

by Tobion at 2012-05-24T23:36:13Z

Please merge before 4387
This commit is contained in:
Fabien Potencier 2012-05-25 09:54:05 +02:00
commit 45849ce306
4 changed files with 108 additions and 55 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
src/Symfony/Component/OptionsResolver/LazyOption.php
View File

@ -11,8 +11,6 @@
namespace Symfony\Component\OptionsResolver;
use Closure;
/**
* An option that is evaluated lazily using a closure.
*
@ -22,7 +20,7 @@ class LazyOption
{
/**
* The underlying closure.
* @var Closure
* @var \Closure
*/
private $closure;
@ -43,7 +41,7 @@ class LazyOption
*
* @see evaluate()
*/
public function __construct(Closure $closure, $previousValue)
public function __construct(\Closure $closure, $previousValue)
{
$this->closure = $closure;
$this->previousValue = $previousValue;

8
src/Symfony/Component/OptionsResolver/Options.php
View File

@ -139,17 +139,15 @@ class Options implements \ArrayAccess, \Iterator, \Countable
throw new OptionDefinitionException('Options cannot be overloaded anymore once options have been read.');
}
$newValue = $value;
// Reset lazy flag and locks by default
unset($this->lock[$option]);
unset($this->lazy[$option]);
// If an option is a closure that should be evaluated lazily, store it
// inside a LazyOption instance.
if ($this->isEvaluatedLazily($value)) {
if (self::isEvaluatedLazily($value)) {
$currentValue = isset($this->options[$option]) ? $this->options[$option] : null;
$newValue = new LazyOption($value, $currentValue);
$value = new LazyOption($value, $currentValue);
// Store locks for lazy options to detect cyclic dependencies
$this->lock[$option] = false;
@ -158,7 +156,7 @@ class Options implements \ArrayAccess, \Iterator, \Countable
$this->lazy[$option] = true;
}
$this->options[$option] = $newValue;
$this->options[$option] = $value;
}
/**

104
src/Symfony/Component/OptionsResolver/OptionsResolver.php
View File

@ -19,6 +19,7 @@ use Symfony\Component\OptionsResolver\Exception\MissingOptionsException;
* Helper for merging default and concrete option values.
*
* @author Bernhard Schussek <bschussek@gmail.com>
* @author Tobias Schultze <http://tobion.de>
*/
class OptionsResolver
{
@ -35,7 +36,7 @@ class OptionsResolver
private $knownOptions = array();
/**
* The options required to be passed to resolve().
* The options without defaults that are required to be passed to resolve().
* @var array
*/
private $requiredOptions = array();
@ -71,6 +72,7 @@ class OptionsResolver
foreach ($defaultValues as $option => $value) {
$this->defaultOptions->overload($option, $value);
$this->knownOptions[$option] = true;
unset($this->requiredOptions[$option]);
}
return $this;
@ -97,6 +99,7 @@ class OptionsResolver
foreach ($defaultValues as $option => $value) {
$this->defaultOptions->set($option, $value);
$this->knownOptions[$option] = true;
unset($this->requiredOptions[$option]);
}
return $this;
@ -105,10 +108,12 @@ class OptionsResolver
/**
* Sets optional options.
*
* This method is identical to `setDefaults`, only that no default values
* are configured for the options. If these options are not passed to
* resolve(), they will be missing in the final options array. This can be
* helpful if you want to determine whether an option has been set or not.
* This method declares a valid option names without setting default values for
* them. If these options are not passed to {@link resolve()} and no default has
* been set for them, they will be missing in the final options array. This can
* be helpful if you want to determine whether an option has been set or not
* because otherwise {@link resolve()} would trigger an exception for unknown
* options.
*
* @param array $optionNames A list of option names.
*
@ -132,7 +137,8 @@ class OptionsResolver
/**
* Sets required options.
*
* If these options are not passed to resolve(), an exception will be thrown.
* If these options are not passed to resolve() and no default has been set for
* them, an exception will be thrown.
*
* @param array $optionNames A list of option names.
*
@ -148,7 +154,10 @@ class OptionsResolver
}
$this->knownOptions[$option] = true;
$this->requiredOptions[$option] = true;
// set as required if no default has been set already
if (!isset($this->defaultOptions[$option])) {
$this->requiredOptions[$option] = true;
}
}
return $this;
@ -163,12 +172,13 @@ class OptionsResolver
*
* @return OptionsResolver The resolver instance.
*
* @throws InvalidOptionsException If an option has not been defined for
* which an allowed value is set.
* @throws InvalidOptionsException If an option has not been defined
* (see {@link isKnown()}) for which
* an allowed value is set.
*/
public function setAllowedValues(array $allowedValues)
{
$this->validateOptionNames(array_keys($allowedValues));
$this->validateOptionsExistence($allowedValues);
$this->allowedValues = array_replace($this->allowedValues, $allowedValues);
@ -191,7 +201,7 @@ class OptionsResolver
*/
public function addAllowedValues(array $allowedValues)
{
$this->validateOptionNames(array_keys($allowedValues));
$this->validateOptionsExistence($allowedValues);
$this->allowedValues = array_merge_recursive($this->allowedValues, $allowedValues);
@ -224,7 +234,7 @@ class OptionsResolver
*/
public function isRequired($option)
{
return isset($this->requiredOptions[$option]) && !isset($this->defaultOptions[$option]);
return isset($this->requiredOptions[$option]);
}
/**
@ -243,7 +253,8 @@ class OptionsResolver
*/
public function resolve(array $options)
{
$this->validateOptionNames(array_keys($options));
$this->validateOptionsExistence($options);
$this->validateOptionsCompleteness($options);
// Make sure this method can be called multiple times
$combinedOptions = clone $this->defaultOptions;
@ -266,44 +277,45 @@ class OptionsResolver
* Validates that the given option names exist and throws an exception
* otherwise.
*
* @param array $optionNames A list of option names.
* @param array $options An list of option names as keys.
*
* @throws InvalidOptionsException If any of the options has not been defined.
*/
private function validateOptionsExistence(array $options)
{
$diff = array_diff_key($options, $this->knownOptions);
if (count($diff) > 0) {
ksort($this->knownOptions);
ksort($diff);
throw new InvalidOptionsException(sprintf(
(count($diff) > 1 ? 'The options "%s" do not exist.' : 'The option "%s" does not exist.') . ' Known options are: "%s"',
implode('", "', array_keys($diff)),
implode('", "', array_keys($this->knownOptions))
));
}
}
/**
* Validates that all required options are given and throws an exception
* otherwise.
*
* @param array $options An list of option names as keys.
*
* @throws InvalidOptionsException If any of the options has not been
* defined.
* @throws MissingOptionsException If a required option is missing.
*/
private function validateOptionNames(array $optionNames)
private function validateOptionsCompleteness(array $options)
{
ksort($this->knownOptions);
$knownOptions = array_keys($this->knownOptions);
$diff = array_diff($optionNames, $knownOptions);
sort($diff);
$diff = array_diff_key($this->requiredOptions, $options);
if (count($diff) > 0) {
if (count($diff) > 1) {
throw new InvalidOptionsException(sprintf('The options "%s" do not exist. Known options are: "%s"', implode('", "', $diff), implode('", "', $knownOptions)));
}
ksort($diff);
throw new InvalidOptionsException(sprintf('The option "%s" does not exist. Known options are: "%s"', current($diff), implode('", "', $knownOptions)));
}
ksort($this->requiredOptions);
$requiredOptions = array_keys($this->requiredOptions);
$diff = array_diff($requiredOptions, $optionNames);
sort($diff);
if (count($diff) > 0) {
if (count($diff) > 1) {
throw new MissingOptionsException(sprintf('The required options "%s" are missing.',
implode('",
"', $diff)));
}
throw new MissingOptionsException(sprintf('The required option "%s" is missing.', current($diff)));
throw new MissingOptionsException(sprintf(
count($diff) > 1 ? 'The required options "%s" are missing.' : 'The required option "%s" is missing.',
implode('", "', array_keys($diff))
));
}
}
@ -313,8 +325,8 @@ class OptionsResolver
*
* @param array $options A list of option values.
*
* @throws InvalidOptionsException If any of the values does not match the
* allowed values of the option.
* @throws InvalidOptionsException If any of the values does not match the
* allowed values of the option.
*/
private function validateOptionValues(array $options)
{

45
src/Symfony/Component/OptionsResolver/Tests/OptionResolverTest.php
View File

@ -372,4 +372,49 @@ class OptionsResolverTest extends \PHPUnit_Framework_TestCase
$this->assertFalse($this->resolver->isRequired('foo'));
}
public function testResolveWithoutOptionSucceedsIfRequiredAndDefaultValue()
{
$this->resolver->setRequired(array(
'foo',
));
$this->resolver->setDefaults(array(
'foo' => 'bar',
));
$this->assertEquals(array(
'foo' => 'bar'
), $this->resolver->resolve(array()));
}
public function testResolveWithoutOptionSucceedsIfDefaultValueAndRequired()
{
$this->resolver->setDefaults(array(
'foo' => 'bar',
));
$this->resolver->setRequired(array(
'foo',
));
$this->assertEquals(array(
'foo' => 'bar'
), $this->resolver->resolve(array()));
}
public function testResolveSucceedsIfOptionRequiredAndValueAllowed()
{
$this->resolver->setRequired(array(
'one', 'two',
));
$this->resolver->setAllowedValues(array(
'two' => array('2'),
));
$options = array(
'one' => '1',
'two' => '2'
);
$this->assertEquals($options, $this->resolver->resolve($options));
}
}