Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfromyceruto:options_resolver_info
Feb 10, 2020

Conversation

yceruto
Copy link
Member

@ycerutoyceruto commentedJan 20, 2020
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
Deprecations?no
Tickets-
LicenseMIT
Doc PRTODO

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 =newOptionsResolver();$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',staticfunction ($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 ondebug:form command (see tests), then you have in advance the informative description of any option.

What do you think?

OskarStark, bigfoot90, ismail1432, sstok, ogizanagi, rvanlaak, and yceruto reacted with heart emoji
@carsonbotcarsonbot added Status: Needs Review RFCRFC = Request For Comments (proposals about features that you want to be discussed) DXDX = Developer eXperience (anything that improves the experience of using Symfony) Feature labelsJan 20, 2020
@javiereguiluz
Copy link
Member

javiereguiluz commentedJan 20, 2020
edited
Loading

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

// Before:$resolver =newOptionsResolver();$resolver->setAllowedValues('date',staticfunction ($value):bool {return$value >=new \DateTime('now');});$resolver->setInfo('date','This option must be a date in the future.');// After:useSymfony\Component\Validator\Constraints\GreaterThan;$resolver =newOptionsResolver();$resolver->setValidator('date',newGreaterThan(['value' =>newDateTime('today'),'message' =>'This option must be a date in the future.']));
sstok reacted with laugh emojirvanlaak reacted with hooray emoji

@yceruto
Copy link
MemberAuthor

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 ondebug:form cmd. It's just like thesetInfo method in theBaseNode 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.

@ycerutoycerutoforce-pushed theoptions_resolver_info branch 2 times, most recently from9f968b7 to2748e6fCompareJanuary 20, 2020 15:03
@nicolas-grekasnicolas-grekas added this to thenext milestoneJan 20, 2020
@xabbuhxabbuh removed their request for reviewJanuary 24, 2020 15:07
@ycerutoycerutoforce-pushed theoptions_resolver_info branch 2 times, most recently fromb76529c tocd75ad6CompareFebruary 7, 2020 02:27
@yceruto
Copy link
MemberAuthor

Comments addressed, thanks!

@fabpot
Copy link
Member

Thank you@yceruto.

fabpot added a commit that referenced this pull requestFeb 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        | TODOThis 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
@fabpotfabpot merged commit0477a06 intosymfony:masterFeb 10, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@fabpotfabpotfabpot approved these changes

Assignees
No one assigned
Labels
DXDX = Developer eXperience (anything that improves the experience of using Symfony)FeatureOptionsResolverRFCRFC = Request For Comments (proposals about features that you want to be discussed)Status: Reviewed
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.

5 participants
@yceruto@javiereguiluz@fabpot@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp