10000 merged branch Tobion/patch-1 (PR #5104) · symfony/symfony@acec659 · GitHub
[go: up one dir, main page]
Skip to content

Commit acec659

Browse files
fabpotfabpot
committed
merged branch Tobion/patch-1 (PR #5104)
This PR was merged into the master branch. Commits ------- e2a50ef [OptionsResolver] fix normalizer without corresponding option 5a53821 [OptionsResolver] fix removing normalizers Discussion ---------- OptionsResolver: normalizer fix setNormalizer() -> replace() -> all() would generate an error. --------------------------------------------------------------------------- by bschussek at 2012-07-29T16:09:20Z Thank you for the fix! Could you please add a test case? --------------------------------------------------------------------------- by Tobion at 2012-07-30T15:42:26Z There is another problem: setNormalizer() (without setting an option) -> all() I suggest to simply ignore normalizers that have no corresponding option. Do you agree? --------------------------------------------------------------------------- by Tobion at 2012-07-30T16:19:24Z On the other hand, one could argue that a normalizer without option should also work like this: ``` $this->options->setNormalizer('foo', function (Options $options) { return ''; }); $this->assertEquals(array('foo' => ''), $this->options->all()); ``` But when having a normalizer that wants a previous value as param, it does not work (because there is none). --------------------------------------------------------------------------- by stof at 2012-07-30T16:30:34Z @Tobion according to github, this need to be rebased --------------------------------------------------------------------------- by bschussek at 2012-07-30T19:16:48Z I guess setNormalizer() should check whether the option is set and fail otherwise. The second possibility, as you say, is to ignore them in all(). I'd prefer whatever is more efficient. --------------------------------------------------------------------------- by bschussek at 2012-07-30T19:17:27Z But setting a normalizer without setting an option, and having that option appear in the final options, does not make sense if you ask me. --------------------------------------------------------------------------- by Tobion at 2012-07-30T21:23:46Z Well it could make sense. If you want to override/normalize an option to a given value however it has been overloaded by others or just not overloaded at all. This is what normalizers do. I think its more consistent than the other solutions. Raising exception in setNormalizer would make the Class dependent on the order you call the methods, e.g. `setNormalizer(); set()` would not work. But the other way round would be ok. Ignoring some normalizers in `all` would be strange because they are there but not applied under some circumstances. --------------------------------------------------------------------------- by Tobion at 2012-07-30T21:42:40Z Added the fix. If you disagree tell me. --------------------------------------------------------------------------- by bschussek at 2012-08-04T09:30:18Z > Raising exception in setNormalizer would make the Class dependent on the order you call the methods, e.g. `setNormalizer(); set()` would not work. But the other way round would be ok. I think this would be a better solution. I dislike if the normalizer magically adds an option that does not exist. This could hide implementation error, e.g. when a refactoring removes an option, but the normalizer is forgotten. Can you throw an exception in this case? Should we find use cases that rely on this to work, we can soften the behavior and remove the exception. --------------------------------------------------------------------------- by Tobion at 2012-08-04T15:02:51Z Well, that would also make it impossible to set a normalizer for on optional option in OptionsResolver. So `setOptional` + `setNormalizers` would throw an exception which sounds counter-intuitive. Are you sure about that? --------------------------------------------------------------------------- by Tobion at 2012-08-17T11:47:58Z ping @bschussek --------------------------------------------------------------------------- by Tobion at 2012-10-07T22:31:44Z @bschussek ping --------------------------------------------------------------------------- by stof at 2012-10-13T18:04:30Z @bschussek ping --------------------------------------------------------------------------- by Tobion at 2012-11-08T09:55:15Z @bschussek please let's get this finished.
2 parents 64b6980 + e2a50ef commit acec659

File tree

2 files changed

+58
-1
lines changed

2 files changed

+58
-1
lines changed

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,7 @@ public function replace(array $options)
143143

144144
$this->options = array();
145145
$this->lazy = array();
146+
$this->normalizers = array();
146147

147148
foreach ($options as $option => $value) {
148149
$this->overload($option, $value);
@@ -503,7 +504,7 @@ private function normalize($option)
503504
$normalizer = $this->normalizers[$option];
504505

505506
$this->lock[$option] = true;
506-
$this->options[$option] = $normalizer($this, $this->options[$option]);
507+
$this->options[$option] = $normalizer($this, array_key_exists($option, $this->options) ? $this->options[$option] : null);
507508
unset($this->lock[$option]);
508509

509510
// The option is now normalized

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

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -469,4 +469,60 @@ public function testHasWithNullValue()
469469

470470
$this->assertTrue($this->options->has('foo'));
471471
}
472+
473+
public function testRemoveOptionAndNormalizer()
474+
{
475+
$this->options->set('foo1', 'bar');
476+
$this->options->setNormalizer('foo1', function (Options $options) {
477+
return '';
478+
});
479+
$this->options->set('foo2', 'bar');
480+
$this->options->setNormalizer('foo2', function (Options $options) {
481+
return '';
482+
});
483+
484+
$this->options->remove('foo2');
485+
$this->assertEquals(array('foo1' => ''), $this->options->all());
486+
}
487+
488+
public function testReplaceOptionAndNormalizer()
489+
{
490+
$this->options->set('foo1', 'bar');
491+
$this->options->setNormalizer('foo1', function (Options $options) {
492+
return '';
493+
});
494+
$this->options->set('foo2', 'bar');
495+
$this->options->setNormalizer('foo2', function (Options $options) {
496+
return '';
497+
});
498+
499+
$this->options->replace(array('foo1' => 'new'));
500+
$this->assertEquals(array('foo1' => 'new'), $this->options->all());
501+
}
502+
503+
public function testClearOptionAndNormalizer()
504+
{
505+
$this->options->set('foo1', 'bar');
506+
$this->options->setNormalizer('foo1', function (Options $options) {
507+
return '';
508+
});
509+
$this->options->set('foo2', 'bar');
510+
$this->options->setNormalizer('foo2', function (Options $options) {
511+
return '';
512+
});
513+
514+
$this->options->clear();
515+
$this->assertEmpty($this->options->all());
516+
}
517+
518+
public function testNormalizerWithoutCorrespondingOption()
519+
{
520+
$test = $this;
521+
522+
$this->options->setNormalizer('foo', function (Options $options, $previousValue) use ($test) {
523+
$test->assertNull($previousValue);
524+
return '';
525+
});
526+
$this->assertEquals(array('foo' => ''), $this->options->all());
527+
}
472528
}

0 commit comments

Comments
 (0)
0