8000 merged branch Tobion/optionsresolver-optim (PR #4388) · symfony/symfony@45849ce · GitHub
[go: up one dir, main page]
Skip to content

Commit 45849ce

Browse files
fabpotfabpot
committed
merged branch Tobion/optionsresolver-optim (PR #4388)
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 a54ea1b into b07fb3c). --------------------------------------------------------------------------- by travisbot at 2012-05-24T05:22:33Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1419232) (merged bad4a1f into b07fb3c). --------------------------------------------------------------------------- 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
2 parents 48ccdaa + bad4a1f commit 45849ce

File tree

4 files changed

+105
-52
lines changed

4 files changed

+105
-52
lines changed

‎src/Symfony/Component/OptionsResolver/LazyOption.php

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,6 @@
1111

1212
namespace Symfony\Component\OptionsResolver;
1313

14-
use Closure;
15-
1614
/**
1715
* An option that is evaluated lazily using a closure.
1816
*
@@ -22,7 +20,7 @@ class LazyOption
2220
{
2321
/**
2422
* The underlying closure.
25-
* @var Closure
23+
* @var \Closure
2624
*/
2725
private $closure;
2826

@@ -43,7 +41,7 @@ class LazyOption
4341
*
4442
* @see evaluate()
4543
*/
46-
public function __construct(Closure $closure, $previousValue)
44+
public function __construct(\Closure $closure, $previousValue)
4745
{
4846
$this->closure = $closure;
4947
$this->previousValue = $previousValue;

‎src/Symfony/Component/OptionsResolver/Options.php

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -139,17 +139,15 @@ public function overload($option, $value)
139139
throw new OptionDefinitionException('Options cannot be overloaded anymore once options have been read.');
140140
}
141141

142-
$newValue = $value;
143-
144142
// Reset lazy flag and locks by default
145143
unset($this->lock[$option]);
146144
unset($this->lazy[$option]);
147145

148146
// If an option is a closure that should be evaluated lazily, store it
149147
// inside a LazyOption instance.
150-
if ($this->isEvaluatedLazily($value)) {
148+
if (self::isEvaluatedLazily($value)) {
151149
$currentValue = isset($this->options[$option]) ? $this->options[$option] : null;
152-
$newValue = new LazyOption($value, $currentValue);
150+
$value = new LazyOption($value, $currentValue);
153151

154152
// Store locks for lazy options to detect cyclic dependencies
155153
$this->lock[$option] = false;
@@ -158,7 +156,7 @@ public function overload($option, $value)
158156
$this->lazy[$option] = true;
159157
}
160158

161-
$this->options[$option] = $newValue;
159+
$this->options[$option] = $value;
162160
}
163161

164162
/**

‎src/Symfony/Component/OptionsResolver/OptionsResolver.php

Lines changed: 55 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
* Helper for merging default and concrete option values.
2020
*
2121
* @author Bernhard Schussek <bschussek@gmail.com>
22+
* @author Tobias Schultze <http://tobion.de>
2223
*/
2324
class OptionsResolver
2425
{
@@ -35,7 +36,7 @@ class OptionsResolver
3536
private $knownOptions = array();
3637

3738
/**
38-
* The options required to be passed to resolve().
39+
* The options without defaults that are required to be passed to resolve().
3940
* @var array
4041
*/
4142
private $requiredOptions = array();
@@ -71,6 +72,7 @@ public function setDefaults(array $defaultValues)
7172
foreach ($defaultValues as $option => $value) {
7273
$this->defaultOptions->overload($option, $value);
7374
$this->knownOptions[$option] = true;
75+
unset($this->requiredOptions[$option]);
7476
}
7577

7678
return $this;
@@ -97,6 +99,7 @@ public function replaceDefaults(array $defaultValues)
9799
foreach ($defaultValues as $option => $value) {
98100
$this->defaultOptions->set($option, $value);
99101
$this->knownOptions[$option] = true;
102+
unset($this->requiredOptions[$option]);
100103
}
101104

102105
return $this;
@@ -105,10 +108,12 @@ public function replaceDefaults(array $defaultValues)
105108
/**
106109
* Sets optional options.
107110
*
108-
* This method is identical to `setDefaults`, only that no default values
109-
* are configured for the options. If these options are not passed to
110-
* resolve(), they will be missing in the final options array. This can be
111-
* helpful if you want to determine whether an option has been set or not.
111+
* This method declares a valid option names without setting default values for
112+
* them. If these options are not passed to {@link resolve()} and no default has
113+
* been set for them, they will be missing in the final options array. This can
114+
* be helpful if you want to determine whether an option has been set or not
115+
* because otherwise {@link resolve()} would trigger an exception for unknown
116+
* options.
112117
*
113118
* @param array $optionNames A list of option names.
114119
*
@@ -132,7 +137,8 @@ public function setOptional(array $optionNames)
132137
/**
133138
* Sets required options.
134139
*
135-
* If these options are not passed to resolve(), an exception will be thrown.
140+
* If these options are not passed to resolve() and no default has been set for
141+
* them, an exception will be thrown.
136142
*
137143
* @param array $optionNames A list of option names.
138144
*
@@ -148,7 +154,10 @@ public function setRequired(array $optionNames)
148154
}
149155

150156
$this->knownOptions[$option] = true;
151-
$this->requiredOptions[$option] = true;
157+
// set as required if no default has been set already
158+
if (!isset($this->defaultOptions[$option])) {
159+
$this->requiredOptions[$option] = true;
160+
}
152161
}
153162

154163
return $this;
@@ -163,12 +172,13 @@ public function setRequired(array $optionNames)
163172
*
164173
* @return OptionsResolver The resolver instance.
165174
*
166-
* @throws InvalidOptionsException If an option has not been defined for
167-
* which an allowed value is set.
175+ * @throws InvalidOptionsException If an option has not been defined
176+
* (see {@link isKnown()}) for which
177+
* an allowed value is set.
168178
*/
169179
public function setAllowedValues(array $allowedValues)
170180
{
171-
$this->validateOptionNames(array_keys($allowedValues));
181+
$this->validateOptionsExistence($allowedValues);
172182

173183
$this->allowedValues = array_replace($this->allowedValues, $allowedValues);
174184

@@ -191,7 +201,7 @@ public function setAllowedValues(array $allowedValues)
191201
*/
192202
public function addAllowedValues(array $allowedValues)
193203
{
194-
$this->validateOptionNames(array_keys($allowedValues));
204+
$this->validateOptionsExistence($allowedValues);
195205

196206
$this->allowedValues = array_merge_recursive($this->allowedValues, $allowedValues);
197207

@@ -224,7 +234,7 @@ public function isKnown($option)
224234
*/
225235
public function isRequired($option)
226236
{
227-
return isset($this->requiredOptions[$option]) && !isset($this->defaultOptions[$option]);
237+
return isset($this->requiredOptions[$option]);
228238
}
229239

230240
/**
@@ -243,7 +253,8 @@ public function isRequired($option)
243253
*/
244254
public function resolve(array $options)
245255
{
246-
$this->validateOptionNames(array_keys($options));
256+
$this->validateOptionsExistence($options);
257+
$this->validateOptionsCompleteness($options);
247258

248259
// Make sure this method can be called multiple times
249260
$combinedOptions = clone $this->defaultOptions;
@@ -266,44 +277,45 @@ public function resolve(array $options)
266277
* Validates that the given option names exist and throws an exception
267278
* otherwise.
268279
*
269-
* @param array $optionNames A list of option names.
280+
* @param array $options An list of option names as keys.
270281
*
271-
* @throws InvalidOptionsException If any of the options has not been
272-
* defined.
273-
* @throws MissingOptionsException If a required option is missing.
282+
* @throws InvalidOptionsException If any of the options has not been defined.
274283
*/
275-
private function validateOptionNames(array $optionNames)
284+
private function validateOptionsExistence(array $options)
276285
{
277-
ksort($this->knownOptions);
278-
279-
$knownOptions = array_keys($this->knownOptions);
280-
$diff = array_diff($optionNames, $knownOptions);
281-
282-
sort($diff);
286+
$diff = array_diff_key($options, $this->knownOptions);
283287

284288
if (count($diff) > 0) {
285-
if (count($diff) > 1) {
286-
throw new InvalidOptionsException(sprintf('The options "%s" do not exist. Known options are: "%s"', implode('", "', $diff), implode('", "', $knownOptions)));
287-
}
288-
289-
throw new InvalidOptionsException(sprintf('The option "%s" does not exist. Known options are: "%s"', current($diff), implode('", "', $knownOptions)));
289+
ksort($this->knownOptions);
290+
ksort($diff);
291+
292+
throw new InvalidOptionsException(sprintf(
293+
(count($diff) > 1 ? 'The options "%s" do not exist.' : 'The option "%s" does not exist.') . ' Known options are: "%s"',
294+
implode('", "', array_keys($diff)),
295+
implode('", "', array_keys($this->knownOptions))
296+
));
290297
}
298+
}
291299

292-
ksort($this->requiredOptions);
293-
294-
$requiredOptions = array_keys($this->requiredOptions);
295-
$diff = array_diff($requiredOptions, $optionNames);
296-
297-
sort($diff);
300+
/**
301+
* Validates that all required options are given and throws an exception
302+
* otherwise.
303+
*
304+
* @param array $options An list of option names as keys.
305+
*
306+
* @throws MissingOptionsException If a required option is missing.
307+
*/
308+
private function validateOptionsCompleteness(array $options)
309+
{
310+
$diff = array_diff_key($this->requiredOptions, $options);
298311

299312
if (count($diff) > 0) {
300-
if (count($diff) > 1) {
301-
throw new MissingOptionsException(sprintf('The required options "%s" are missing.',
302-
implode('",
303-
"', $diff)));
304-
}
313+
ksort($diff);
305314

306-
throw new MissingOptionsException(sprintf('The required option "%s" is missing.', current($diff)));
315+
throw new MissingOptionsException(sprintf(
316+
count($diff) > 1 ? 'The required options "%s" are missing.' : 'The required option "%s" is missing.',
317+
implode('", "', array_keys($diff))
318+
));
307319
}
308320
}
309321

@@ -313,8 +325,8 @@ private function validateOptionNames(array $optionNames)
313325
*
314326
* @param array $options A list of option values.
315327
*
316-
* @throws InvalidOptionsException If any of the values does not match the
317-
* allowed values of the option.
328+
* @throws InvalidOptionsException If any of the values does not match the
329+
* allowed values of the option.
318330
*/
319331
private function validateOptionValues(array $options)
320332
{

‎src/Symfony/Component/OptionsResolver/Tests/OptionResolverTest.php

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -372,4 +372,49 @@ public function testNotRequiredIfRequiredAndDefaultValue()
372372

373373
$this->assertFalse($this->resolver->isRequired('foo'));
374374
}
375+
376+
public function testResolveWithoutOptionSucceedsIfRequiredAndDefaultValue()
377+
{
378+
$this->resolver->setRequired(array(
379+
'foo',
380+
));
381+
$this->resolver->setDefaults(array(
382+
'foo' => 'bar',
383+
));
384+
385+
$this->assertEquals(array(
386+
'foo' => 'bar'
387+
), $this->resolver->resolve(array()));
388+
}
389+
390+
public function testResolveWithoutOptionSucceedsIfDefaultValueAndRequired()
391+
{
392+
$this->resolver->setDefaults(array(
393+
'foo' => 'bar',
394+
));
395+
$this->resolver->setRequired(array(
396+
'foo',
397+
));
398+
399+
$this->assertEquals(array(
400+
'foo' => 'bar'
401+
), $this->resolver->resolve(array()));
402+
}
403+
404+
public function testResolveSucceedsIfOptionRequiredAndValueAllowed()
405+
{
406+
$this->resolver->setRequired(array(
407+
'one', 'two',
408+
));
409+
$this->resolver->setAllowedValues(array(
410+
'two' => array('2'),
411+
));
412+
413+
$options = array(
414+
'one' => '1',
415+
'two' => '2'
416+
);
417+
418+
$this->assertEquals($options, $this->resolver->resolve($options));
419+
}
375420
}

0 commit comments

Comments
 (0)
0