8000 [OptionResolver] (Default) null values are not allowed since 2.6 · Issue #12809 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[OptionResolver] (Default) null values are not allowed since 2.6 #12809

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
4rthem opened this issue Dec 2, 2014 · 10 comments
Closed

[OptionResolver] (Default) null values are not allowed since 2.6 #12809

4rthem opened this issue Dec 2, 2014 · 10 comments
Labels
BC Break Bug OptionsResolver

Comments

@4rthem
Copy link
Contributor
4rthem commented Dec 2, 2014 
$resolver->setDefaults([
    'my_option' => null,
]);
$resolver->setAllowedValues([
    'my_option' => ['one', 'two', 'three'],
]);

With this configuration, the option my_option must be defined because it can't be null.

Is this wanted? I can't find any note about this on the upgrade guide.

This causes an InvalidOptionsException with HWIOAuthBundle. We now have to define all options or wait for a bundle update: see options configuration where access_type was optional.
@weaverryan
Copy link
Member

I can confirm this BC break that's new in 2.6. I can reproduce it exactly with this code:

use Symfony\Component\OptionsResolver\OptionsResolver;
$resolver = new OptionsResolver();

$resolver->setDefaults([
    'my_option' => null,
]);
$resolver->setAllowedValues([
    'my_option' => ['one', 'two', 'three'],
]);

$options = $resolver->resolve(array());

With the 2.5 branch, I receive no error. With 2.6.1, $resolver->resolve() gives the following error:

PHP Fatal error: Uncaught exception 'Symfony\Component\OptionsResolver\Exception\InvalidOptionsException' with message 'The option "my_option" with value null is invalid. Accepted values are: "one", "two", "three".'

I confirmed that the BC break was introduced at #11716, which I believe was accidental. I think we should fix this to the old behavior.

Thanks @4rthem for the report!

@weaverryan weaverryan added the Bug label Dec 4, 2014
@fabpot
Copy link
Member
fabpot commented Dec 5, 2014

@webmozart Can you have a look at this one?

@jasonhanley
Copy link

I can confirm this problem with Symfony 2.6.1.

It breaks HWIOAuthBundle when using Google login and gives the error: "The option "access_type" with value null is invalid. Accepted values are: "online", "offline"."

@stof
Copy link
Member
stof commented Jan 4, 2015

Well, this is indeed a BC break in one case: when your default value is not allowed by your validation of allowed values.

However, this means that your option resolution forbids people to pass you the option with its default value explicitly. This looks like a broken setup IMO.
Should we really revert the other bugfixes we made to support this weird use case ? I think people should rather fix their value validation to allow their default value.

@Tobion
Copy link
Contributor
Tobion commented Jan 4, 2015

I also think the old behavior was not intended to work and just worked accidentally. The current behavior is the better one. To support the use-case you only have to add null to the allowed values. The other solution would be that HWIOAuthBundle makes these opt 8000 ions optional with setOptional/setDefined and does not set a random null value as default.

@fabpot fabpot removed the Critical label Jan 5, 2015
@jasonhanley
Copy link

Although I agree that throwing the exception is certainly more "correct", and HWIOAuthBundle is clearly doing the wrong thing -- From an end user perspective, the new check prevents anyone from upgrading to 2.6 until every dependency they have that relies on the old behavior is updated. I've had to require "symfony/symfony": "2.5.*" in all my projects pending a resolution. I've submitted hwi/HWIOAuthBundle#710 since I don't think it's logged there.

@jasonhanley jasonhanley mentioned this issue Jan 8, 2015
Closed
@weaverryan
Copy link
Member

@jasonhanley brings up a good point. Is there any reason we shouldn't re-allow null now and deprecate this "feature" for removal in 3.0?

@Tobion
Copy link
Contributor
Tobion commented Jan 8, 2015

I'd rather consider this a bug that was fixed. And reintroducing a bug seems bad.
It's quite obvious that it's a bug considering that allowed values and allowed types behave differently. So null values are ignored for allowed values but if you set allowed types, then null is not ignored. The difference are these two lines array_key_exists vs isset

Tobion added a commit to Tobion/symfony that referenced this issue Jan 8, 2015
@Tobion
[OptionsResolver] added test for allowed values and types that defaul…
0b42cad 
…t to null

ref symfony#12809
@Tobion Tobion mentioned this issue Jan 8, 2015
Merged
@Tobion
Copy link
Contributor
Tobion commented Jan 8, 2015

Closing because it was a bug fix in @stof and my opinion. I added a test to keep this logic in place.

@Tobion Tobion closed this as completed Jan 8, 2015
@stof
Copy link
Member
stof commented Jan 18, 2015

@weaverryan The BC break is not about null specifically. It is about any mismatch between the default value and the allowed values. Symfony 2.5 was also allowing you to defined the allowed values as being array(1, 2, 3) and then setting the default value to 4.
The only way to avoid the BC break is to revert the whole bugfix. Adding a special case for null would not keep BC.

fabpot added a commit that referenced this issue Jan 26, 2015
@fabpot
minor #13331 [OptionsResolver] added test for allowed values and type…
1901c84 
…s that default to null (Tobion)

This PR was merged into the 2.6 branch.

Discussion
----------

[OptionsResolver] added test for allowed values and types that default to null

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | no
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Added test to keep logic as experienced in #12809

Commits
-------

0b42cad [OptionsResolver] added test for allowed values and types that default to null
@BigZ BigZ mentioned this issue Jan 11, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
BC Break Bug OptionsResolver
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@fabpot @weaverryan @stof @jasonhanley @Tobion @4rthem
0