8000 [RFC][OptionsResolver] Add ability to deprecate options · Issue #27216 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[RFC][OptionsResolver] Add ability to deprecate options #27216

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
yceruto opened this issue May 9, 2018 · 8 comments
Closed

[RFC][OptionsResolver] Add ability to deprecate options #27216

yceruto opened this issue May 9, 2018 · 8 comments
Labels
OptionsResolver RFC RFC = Request For Comments (proposals about features that you want to be discussed)

Comments

@yceruto
Copy link
Member
yceruto commented May 9, 2018 •
edited
Loading

Introduction

Currently the consumers of the OptionsResolver component (mainly the Form component, later LDAP started to use it and possibly many bundles and libraries too) need sometime to delete or rename an already defined option or change its default value keeping BC, seeing itself in the need to create notes like this:

symfony/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php

Line 302 in 07d2570

'property' => null, // deprecated, use "choice_label"

and manage themselves where and how to trigger the corresponding depreciation message:

symfony/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php

Lines 250 to 257 in 07d2570

// deprecation note
$propertyNormalizer = function (Options $options, $propertyName) {
if ($propertyName) {
@trigger_error('The "property" option is deprecated since Symfony 2.7 and will be removed in 3.0. Use "choice_label" instead.', E_USER_DEPRECATED);
}
return $propertyName;
};

Moreover, it isn't always effective if the option has a default value. For instance, according to the previous code doing this:

$builder->add('foo', EntityType::class, [
    // ...
    'property' => null,
]);

it wouldn't show the deprecation message because when the configured default value is equal to the given value -> we don't know that this option is being used. Also, the normalizer function (as just one is allowed) can be overridden by any current child type or type extension in userland, in that case, the message wouldn't show either.

Proposal to improve the current scenario

Introduce a simpler and safer way to deprecate an option using a new method in OptionsResolver:

public function setDeprecated(string $option, string $deprecationMessage = 'The option "%name%" is deprecated.', callable $callback = null): self

Then, if this option is used or has default value, the deprecation message always will be triggered. Conditionally you could deprecate some values of many feasible by using the 3rd argument.

It is a small but powerful feature as this information can be displayed anywhere, in the form profiler, debug:form command, etc.

Other method that could help to know whether a given option is deprecated or not is:

public function isDeprecated(string $option): bool

Example

Deprecating a whole option:

public function configureOptions(OptionsResolver $resolver)
{
    $resolver->setDefaults([
        'property' => null,
        'choice_label' => // ...
    ]);

    $resolver->setDeprecated('property', 'The option "property" is deprecated since Symfony X.Y. Use "choice_label" instead.');
}

Deprecating an option value:

public function configureOptions(OptionsResolver $resolver)
{
    $resolver
        ->setDefault('foo', false)
        ->setAllowedTypes('foo', ['null', 'bool'])
        ->setDeprecated('foo', 'Passing "null" to option "foo" is deprecated since ...', function ($value) {
            return null === $value; // value before normalization
        })
    ;
}

Let me know first if it's something that core & community need. I've prepared a tiny PR for that.

Cheers!
@yceruto yceruto changed the title [OptionsResolver] Add ability to deprecate options [RFC][OptionsResolver] Add ability to deprecate options May 9, 2018
@linaori
Copy link
Contributor
linaori commented May 10, 2018

What if only certain usage scenario are deprecated? Could add a 3rd argument that receives a callable and acts based on the value passed to the option resolver (not the resolved value)

@yceruto
Copy link
Member Author
yceruto commented May 10, 2018 •
edited
Loading

@iltar That could be a good Idea! I think that's possible too and cover other use case like deprecating just one value/type from many feasible. I updated the description to include that, thanks.

@phansys
Copy link
Contributor
phansys commented May 10, 2018

It's an interesting feature, it would be nice to make possible to cover all the aspects supported by the component, like argument values and its types.

@xabbuh xabbuh added OptionsResolver RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels May 10, 2018
@yceruto
Copy link
Member Author
yceruto commented May 10, 2018 •
edited
Loading

@phansys yes, argument values and their types are already covered by @iltar's idea: into callback func you could do that, e.g:

->setDeprecated('foo', 'Passing "null", an integer or instance of "\Bar" to option "foo" is deprecated since ...', function ($value) {
    return null === $value || is_int($value) || $value instanceof \Bar;
})

@yceruto
Copy link
Member Author
yceruto commented May 12, 2018

Although to be more accurate, probably we should accept a string or a callable as 2nd argument, if a callable is given it must return the deprecation message or anything else to skip, so:

->setDeprecated('foo', function ($value) {
    if (null === $value) {
        $str = '"null"';
    } elseif (is_int($value)) {
        $str = sprintf('an integer "%d"', $value);
    } elseif ($value instanceof \Bar) {
        $str = sprintf('an instance of \Bar "%s"', get_class($value));
    } else {
        return;
    }
    
    return sprintf('Passing %s to option "foo" is deprecated.', $str);
});

@Aerendir
Copy link
Contributor

I like more to have three arguments each one with its purpose than have two.

@yceruto
Copy link
Member Author
yceruto commented May 14, 2018 •
edited
Loading

I think the purpose of the 2nd argument can be documented as well, or better:

/** 
 * @param string|callable $deprecationMessage The option deprecation message or function
 *                                            that accept the value and must return a string
 *                                            or anything else to skip
 */

In fact, that'll simplify a bit the implementation as we only have to save two data: option name (string) and deprecation message (string or callable), so with one internal variable private $deprecated = array() will be enough.

I can send the PR this week if there is no one against, it feels like enough 👍 to me :)

@yceruto yceruto mentioned this issue May 15, 2018
Merged
@yceruto
Copy link
Member Author
yceruto commented May 15, 2018 •
edited
Loading

PR created #27277 reviews welcomed

nicolas-grekas added a commit that referenced this issue Jun 19, 2018
@nicolas-grekas
feature #27277 [OptionsResolver] Introduce ability to deprecate optio…
76d3589 
…ns, allowed types and values (yceruto)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[OptionsResolver] Introduce ability to deprecate options, allowed types and values

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

**Deprecating an option**
```php
$resolver = (new OptionsResolver())
    ->setDefined(['foo', 'bar'])
    ->setDeprecated('foo')
;
$resolver->resolve(['foo' => 'baz']); // PHP Deprecated: The option "foo" is deprecated.
```
With custom message:
```php
$resolver = (new OptionsResolver())
    ->setDefined('foo')
    ->setDefault('bar', function (Options $options) {
        return $options['foo'];
    })
    ->setDeprecated('foo', 'The option "foo" is deprecated, use "bar" option instead.')
;
$resolver->resolve(['foo' => 'baz']); // PHP Deprecated: The option "foo" is deprecated, use "bar" option instead.
$resolver->resolve(['bar' => 'baz']); // OK.
```
**Deprecating allowed types**
```php
$resolver = (new OptionsResolver())
    ->setDefault('type', null)
    ->setAllowedTypes('type', ['null', 'string', FormTypeInterface::class])
    ->setDeprecated('type', function ($value) {
        if ($value instanceof FormTypeInterface) {
            return sprintf('Passing an instance of "%s" to option "type" is deprecated, pass its FQCN instead.', FormTypeInterface::class);
        }
    })
;
$resolver->resolve(['type' => new ChoiceType()]); // PHP Deprecated: Passing an instance of "Symfony\Component\Form\FormTypeInterface" to option "type" is deprecated, pass its FQCN instead.
$resolver->resolve(['type' => ChoiceType::class]); // OK.
```
The closure is invoked when `resolve()` is called. The closure must return a string (the deprecation message) or an empty string to ignore the option deprecation.

Multiple types and normalizer:
```php
$resolver = (new OptionsResolver())
    ->setDefault('percent', 0.0)
    ->setAllowedTypes('percent', ['null', 'int', 'float'])
    ->setDeprecated('percent', function ($value) {
        if (null === $value) {
            return 'Passing "null" to option "percent" is deprecated, pass a float number instead.';
        }

        if (is_int($value)) {
            return sprintf('Passing an integer "%d" to option "percent" is deprecated, pass a float number instead.', $value);
        }
    })
    ->setNormalizer('percent', function (Options $options, $value) {
        return (float) $value;
    })
;
$resolver->resolve(['percent' => null]); // PHP Deprecated: Passing "null" to option "percent" is deprecated, pass a float number instead.
$resolver->resolve(['percent' => 20]); // PHP Deprecated: Passing an integer "20" to option "percent" is deprecated, pass a float number instead.
$resolver->resolve(['percent' => 20.0]); // OK.
```
The parameter passed to the closure is the value of the option after validating it and before normalizing it.

**Deprecating allowed values**
```php
$resolver = (new OptionsResolver())
    ->setDefault('percent', 0.0)
    ->setAllowedTypes('percent', 'float')
    ->setDeprecated('percent', function ($value) {
        if ($value < 0) {
            return 'Passing a number less than 0 to option "percent" is deprecated.';
        }
    })
;
$resolver->resolve(['percent' => -50.0]); // PHP Deprecated: Passing a number less than 0 to option "percent" is deprecated.
```

Commits
-------

f8746ce Add ability to deprecate options
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
OptionsResolver RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@nicolas-grekas @phansys @linaori @xabbuh @Aerendir @yceruto
0