8000 [OptionsResolver] Options not provided by user triggers deprecation · Issue #28848 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[OptionsResolver] Options not provided by user triggers deprecation #28848

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
ro0NL opened this issue Oct 13, 2018 · 6 comments
Closed

[OptionsResolver] Options not provided by user triggers deprecation #28848

ro0NL opened this issue Oct 13, 2018 · 6 comments
Labels
OptionsResolver

Comments

@ro0NL
Copy link
Contributor
ro0NL commented Oct 13, 2018

Description
Since 4.2 OptionsResolver can deprecate options, however the deprecation is triggered from offsetGet()

symfony/src/Symfony/Component/OptionsResolver/OptionsResolver.php

Line 943 in 9bec1fc

@trigger_error(strtr($deprecationMessage, array('%name%' => $option)), E_USER_DEPRECATED);

Im not sure about this, it causes the deprecation to trigger without even being provided by the user (i.e. when accessing the default value). Shouldnt it be moved to resolve() instead?

ref #27277 cc @yceruto
@ro0NL
Copy link
Contributor Author
ro0NL commented Oct 14, 2018 •
edited
Loading

i confused array $options and Options $options. Of course only in case of the latter the deprecation triggers when accessing the option, but this can still happen e.g. in SF core (e.g. when referenced from a default value closure in another option)

So no real issue so far, but maybe it needs special API like offsetGet($option, $triggerDeprecation = false) sooner or later.

@ro0NL ro0NL mentioned this issue Oct 14, 2018
Merged
@yceruto
Copy link
Member
yceruto commented Oct 15, 2018

[OptionsResolver] Options not provided by user triggers deprecation

I agree, but this behavior is expected also for ppls who use a deprecated option during a lazy evaluation or normalization of another option. In those cases, they should always notice the deprecation message (even, if no value is provided).

Tomorrow I'll take a look,
Cheers!

@ro0NL
Copy link
Contributor Author
ro0NL commented Oct 15, 2018

See #28860 that includes a patch

IMHO setDefaults belongs to the definition side (along with setDeprecated), and only consuming deprecated options should trigger

@yceruto yceruto mentioned this issue Oct 15, 2018
Merged
@yceruto
Copy link
Member
yceruto commented Oct 15, 2018

See the proposal to solve it #28878

@stof
Copy link
Member
stof commented Oct 15, 2018

@ro0NL if you have a lazy default value using the value of another deprecated option, you need to be warned, to know you need to migrate your callable (but there should be a way to opt-out of the deprecation to be able to write a BC layer)

@fabpot fabpot closed this as completed Oct 24, 2018
fabpot added a commit that referenced this issue Oct 24, 2018
@fabpot
bug #28878 [OptionsResolver] Trigger deprecation only if the option i…
9d3621e 
…s used (yceruto)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[OptionsResolver] Trigger deprecation only if the option is used

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #28848
| License       | MIT
| Doc PR        | symfony/symfony-docs#10496

It's true that showing a deprecation message when the option is not used is a bit annoying and it would be heavy to get rid of it.

Now, a deprecated option is triggered only when it's provided by the user or each time is being called from a lazy evaluation (except for deprecations based on the value, they're triggered only when provided by the user).

Commits
-------

1af23c9 [OptionsResolver] Trigger deprecation only if the option is used
fabpot added a commit that referenced this issue Oct 25, 2018
@fabpot
feature #28860 [Form] Deprecate TimezoneType regions option (ro0NL)
b01ea27 
This PR was merged into the 4.2-dev branch.

Discussion
----------

[Form] Deprecate TimezoneType regions option

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | yes
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #28848
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

I know i've added this option myself in 4.1, but given my recent development for #28624 i realized it's an opinionated feaure, which can/should be solved on user-side (`choice_filter/choice_loader` and/or `group_by`).

- blocks translations as we dont have them (see #28831)
- blocks possibility of switching to Intl zones which doesnt really have this filter feature (see #28836)

~While at it, i solved a few issues with `OptionsResolver` that is able to deprecate options as of 4.2 also.~ Fixed in #28878

- when resolved trigger the deprecation
- allow to opt-out from triggering the deprecation
- dont trigger deprecation for default values (only given ones)

Commits
-------

5cb532d [Form] Deprecate TimezoneType regions option
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
OptionsResolver
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@fabpot @javiereguiluz @stof @ro0NL @yceruto
0