[OptionsResolver] Fixed unexpected behavior when using setOptional + setNormalizers #6233
Conversation
| @@ -500,12 +500,14 @@ private function normalize($option) | |||
| throw new OptionDefinitionException('The options "' . implode('", "', $conflicts) . '" have a cyclic dependency.'); | |||
| } | |||
|
|
|||
| /** @var \Closure $normalizer */ | |||
| $normalizer = $this->normalizers[$option]; | |||
| if (array_key_exists($option, $this->options)) { | |||
There was a problem hiding this comment.
It's probably better to not call $this->normalize at all (in all()) in this case.
There was a problem hiding this comment.
@Tobion since a call to unset($this->normalizers[$option]); is contained in this function I feel that moving this check to all() could lead to confusion and possibly unexpected behavior later.
There was a problem hiding this comment.
Can there be a lock on an option that does not exist yet when normalizing? I don't think so. So you could move the array_key_exists check above if (isset($this->lock[$option])) { which saves a method call. Or as I suggested above put this logic in all so even save the method call to normalize. Somy tiny code duplication is prefered compared to slowing the code that is executed thousands of times.
|
We cannot fix this without breaking BC (see #9174). Postponed to 3.0. |
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #6215
License of the code: MIT