Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[OptionsResolver] Passing Options argument to deprecation closure#28738
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
Conversation
ogizanagi left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
xabbuh commentedOct 5, 2018
Yes, let's do that. Otherwise, it will become hard to remember when arguments are passed in which order. |
db8d2b4 to2d531b3Comparec903fdb to21b9aa3Compare
xabbuh left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
just two minor suggestions
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
yceruto commentedOct 5, 2018
@xabbuh comments addressed. I'm preparing the doc PR right now. |
javiereguiluz left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Useful feature, nicely developed and fully documented.@yceruto thanks a lot for doing things so nice!
532b6bd to2936051Comparefabpot commentedOct 10, 2018
Thank you@yceruto. |
…ion closure (yceruto)This PR was squashed before being merged into the 4.2-dev branch (closes#28738).Discussion----------[OptionsResolver] Passing Options argument to deprecation closure| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#28721 (comment)| License | MIT| Doc PR |symfony/symfony-docs#10439As spotted here#28721, we sometimes need more advanced cases, where the deprecation of the value depends on another option:```php$resolver->setDeprecated('date_format', function (Options $options, $dateFormat) { if (null !== $options['date_format'] && 'single_text' === $options['widget']) { return sprintf('Using the "date_format" option of the %s when the "widget" option is set to "single_text" is deprecated since Symfony 4.2.', self::class); } return '';});```There is still a decision to make:> We're in time to change the arguments position (Options $options, $value) to be consistent with other closure signatures.WDYT?Commits-------2936051 [OptionsResolver] Passing Options argument to deprecation closure
…re deprecation func (yceruto)This PR was merged into the master branch.Discussion----------[OptionsResolver] Documenting Options argument for closure deprecation funcSeesymfony/symfony#28738Commits-------abb5189 Documenting Options argument for closure deprecation func
Uh oh!
There was an error while loading.Please reload this page.
As spotted here#28721, we sometimes need more advanced cases, where the deprecation of the value depends on another option:
There is still a decision to make:
WDYT?