[OptionsResolver] remove dead code and useless else

[ronfroy]

Q	A
Branch?	master
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
License	MIT

[javiereguiluz]

If we make this change in master, which uses modern PHP versions, we could even simplify it more:

-    if (!isset($this->allowedValues[$option])) {
-        $this->allowedValues[$option] = $allowedValues;
-    } else {
-        $this->allowedValues[$option] = array_merge($this->allowedValues[$option], $allowedValues);
-    }

+    $this->allowedValues[$option] = array_merge($this->allowedValues[$option] ?? array(), $allowedValues);

[skalpa]

Those if/else constructs aren't useless: they prevent a unnecessary call to array_merge, which is a costly function, even though the damage should be limited when it is called with an empty array as argument.
As such, these changes don't do anything valuable, they just reduce the number of lines of code at the expense of performance, so they should be left untouched IMHO.

[ronfroy]

@skalpa it is a really minor cost.

[stof]

a minor cost repeated hundreds or thousands times when building complex forms with many fields becomes major

[linaori]

I agree with stof on this one. Perhaps it would be a good idea to add a comment in the code as of why this was done, instead of always calling array_merge?

[ronfroy]

removed

[yceruto]

Also, we can remove the array check above and use (array) $allowedValues instead, just like addAllowedTypes() method.

[skalpa]

@yceruto I understand that the performance cost I discussed is minor. However your changes don't add any functionality and some would argue that you are just changing code for the sake of changing code.
Removing the dead code is useful, but I believe that there is no point in fixing code that isn't broken to replace it with a version that is, even marginally, worse.

What do you think was wrong with the if/else statements ?

[nicolas-grekas]

Thank you @ronfroy.