Skip to content

[RFC][DX][OptionsResolver] Allow setting info message per option #35400

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.

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

Merged
merged 1 commit into from
Feb 10, 2020

Conversation

yceruto
Copy link
Member

@yceruto yceruto commented Jan 20, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR TODO

This is a DX proposal that will help in debugging/errors to better understand the meaning of one defined option.

This is how you'd use it:

$resolver = new OptionsResolver();
$resolver->setDefined('date');
$resolver->setAllowedTypes('date', \DateTime::class);
$resolver->setInfo('date', 'A future date'); // <-- NEW
// ...

This information may be useful for those options where their name cannot be intuitive enough, or their purpose is too complex. Here is an example (based on the example above):

// ...
$resolver->setAllowedValues('date', static function ($value): bool {
    return $value >= new \DateTime('now');
});

So, if you introduce a date value that does not match the criteria, you will get this error message:

Before:

The option "date" with value DateTime is invalid.

Note that the allowed value is not printable in this case, hence the error message cannot be clear at all.

After:

The option "date" with value DateTime is invalid. Info: A future date.

Although a more accurate error message can be triggered within the \Closure if desired.

Also you'll see this info message on debug:form command (see tests), then you have in advance the informative description of any option.

What do you think?

@yceruto yceruto requested a review from xabbuh as a code owner January 20, 2020 14:02
@carsonbot carsonbot added Status: Needs Review RFC RFC = Request For Comments (proposals about features that you want to be discussed) DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature labels Jan 20, 2020
@javiereguiluz
Copy link
Member

javiereguiluz commented Jan 20, 2020

Here's a crazy proposal 🙈 to solve this same problem but using the Symfony Validator component:

// Before:
$resolver = new OptionsResolver();
$resolver->setAllowedValues('date', static function ($value): bool {
    return $value >= new \DateTime('now');
});
$resolver->setInfo('date', 'This option must be a date in the future.');



// After:
use Symfony\Component\Validator\Constraints\GreaterThan;

$resolver = new OptionsResolver();
$resolver->setValidator('date', new GreaterThan([
    'value' => new DateTime('today'),
    'message' => 'This option must be a date in the future.'
]));

@yceruto
Copy link
Member Author

yceruto commented Jan 20, 2020

Hi @javiereguiluz, thanks for your input :) your proposal would require the integration of the Validator component, isn't it?

Note that my proposal isn't only about improving the validation error message, but also about debugging information on debug:form cmd. It's just like the setInfo method in the BaseNode of the Config component.

@javiereguiluz
Copy link
Member

Thanks for your comments ... I was thinking about an optional dependency with the Validator component (the usual "You can't use XXXX because YYYY. Try running composer require ..." message we use in other parts of Symfony). But yes, my comment is completely different from your original proposal. We can ignore it! Thanks.

@yceruto yceruto force-pushed the options_resolver_info branch 2 times, most recently from 9f968b7 to 2748e6f Compare January 20, 2020 15:03
@nicolas-grekas nicolas-grekas added this to the next milestone Jan 20, 2020
@xabbuh xabbuh removed their request for review January 24, 2020 15:07
@yceruto yceruto force-pushed the options_resolver_info branch 2 times, most recently from b76529c to cd75ad6 Compare February 7, 2020 02:27
@yceruto
Copy link
Member Author

yceruto commented Feb 10, 2020

Comments addressed, thanks!

@fabpot
Copy link
Member

fabpot commented Feb 10, 2020

Thank you @yceruto.

fabpot added a commit that referenced this pull request Feb 10, 2020
…per option (yceruto)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[RFC][DX][OptionsResolver] Allow setting info message per option

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | TODO

This is a DX proposal that will help in debugging/errors to better understand the meaning of one defined option.

This is how you'd use it:
```php
$resolver = new OptionsResolver();
$resolver->setDefined('date');
$resolver->setAllowedTypes('date', \DateTime::class);
$resolver->setInfo('date', 'A future date'); // <-- NEW
// ...
```
This information may be useful for those options where their name cannot be intuitive enough, or their purpose is too complex. Here is an example (based on the example above):
```php
// ...
$resolver->setAllowedValues('date', static function ($value): bool {
    return $value >= new \DateTime('now');
});
```
So, if you introduce a date value that does not match the criteria, you will get this error message:

**Before:**
```
The option "date" with value DateTime is invalid.
```
Note that the allowed value is not printable in this case, hence the error message cannot be clear at all.

**After:**
```
The option "date" with value DateTime is invalid. Info: A future date.
```
Although a more accurate error message can be triggered within the `\Closure` if desired.

Also you'll see this info message on `debug:form` command (see tests), then you have in advance the informative description of any option.

What do you think?

Commits
-------

0477a06 Allow setting one info message per option
@fabpot fabpot merged commit 0477a06 into symfony:master Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature OptionsResolver RFC RFC = Request For Comments (proposals about features that you want to be discussed) Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants