Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Console] Make SymfonyQuestionHelper::ask optional by default#22317
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
chalasr commentedApr 6, 2017
👍 Already pointed out in#20141 when making it more flexible, I don't know why this default validator has been added, console's Symfony* stuff should only be about output styling, not input filtering. |
| /** | ||
| * {@inheritdoc} | ||
| * | ||
| * @deprecated since version 3.3, to be removed in 4.0 |
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.
Not sure about this.. it creates some nasty strikethroughs in IDE's, and the same method call will work in 4.0
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.
Indeed, I think I would talk only about the removed validation. Btw this needs to be documented :)
nicolas-grekas commentedApr 28, 2017
Moving to milestone 3.4 as this is not a blocker for 3.3. If you don't agree, please tell :) |
ro0NL commentedApr 28, 2017
Just annoying :) 👍 for 3.4, im used doing this already. |
fabpot 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.
Target should also be changed to 3.4.
UPGRADE-3.3.md Outdated
| have been deprecated in favor of the`console.error` event and the`ConsoleErrorEvent` | ||
| class. The deprecated event and class will be removed in 4.0. | ||
| * The`SymfonyQuestionHelper::ask` default validation has been deprecated and will be removed in 4.0. Apply validation using`Question::setValidator` instead. |
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.
should be moved to 3.4.
| }else { | ||
| // make required | ||
| if (!is_array($value) && !is_bool($value) &&0 ===strlen($value)) { | ||
| @trigger_error('The default question validator is deprecated since Symfony 3.3 and will not be used anymore in version 4.0. Set a custom question validator if needed.',E_USER_DEPRECATED); |
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.
3.4 instead of 3.3
…default (ro0NL)This PR was merged into the 4.0-dev branch.Discussion----------[Console] Make SymfonyQuestionHelper::ask optional by default| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | yes| Tests pass? | yes (nothing in core depends on it)| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!--highly recommended for new features-->i noticed when writing commands i always keep doing```php$io = new SymfonyStyle($input, $output);$answer = $io->ask('...', null, function ($value) { return $value; });// instead of just$answer = $io->ask('...');```only to bypass a built-in validation, of which im not sure why it's there. Note the base question helper doesnt make this assumption...Commits-------2da429c [Console] Make SymfonyQuestionHelper::ask optional by default…nal by default (ro0NL)This PR was merged into the 4.0-dev branch.Discussion----------[Console] Make SymfonyQuestionHelper::ask optional by default| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | yes| Tests pass? | yes (nothing in core depends on it)| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!--highly recommended for new features-->i noticed when writing commands i always keep doing```php$io = new SymfonyStyle($input, $output);$answer = $io->ask('...', null, function ($value) { return $value; });// instead of just$answer = $io->ask('...');```only to bypass a built-in validation, of which im not sure why it's there. Note the base question helper doesnt make this assumption...Commits-------2da429c [Console] Make SymfonyQuestionHelper::ask optional by defaultchalasr commentedJul 22, 2017
Thank you@ro0NL |
| }else { | ||
| // make required | ||
| if (!is_array($value) && !is_bool($value) &&0 ===strlen($value)) { | ||
| @trigger_error('The default question validator is deprecated since Symfony 3.4 and will not be used anymore in version 4.0. Set a custom question validator instead.',E_USER_DEPRECATED); |
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.
@ro0NL is this really in the correct code path? Throwing both a deprecation and an exception is suspicious to me.
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.
indeed
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.
We're deprecating the fact this exception is thrown when the value is blank. Meaning in 4.0 ask() is optional by default, whereas now it's required by default.
I actually considered putting the trigger aboveif (!is_array($value) && !is_bool($value) && 0 === strlen($value)) {, but its weird to get the deprecation for a case that is still valid in 4.0.
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.
Indeed, I think it's the right place for this deprecation.
* 3.4: [DI] use assertStringEqualsFile when possible [VarDumper] Adapt to php 7.2 changes [DI] Fix using private services in expressions [Form][TwigBridge] Don't render _method in form_rest() for a child form [Form] Static call TimezoneType::getTimezones Removed references for non existent validator constraints Suggest using quotes instead of Yaml::PARSE_KEYS_AS_STRINGS [DI] Fix test [Cache] Handle unserialization failures for Memcached Remove unused prop + added@deprecated Remove unused mocks/vars feature#22317 [Console] Make SymfonyQuestionHelper::ask optional by default (ro0NL) [DoctrineBridge][PropertyInfo] Added support for Doctrine Embeddables [Validator] Fix IbanValidator for ukrainian IBANs Router: allow HEAD method to be defined first [WebProfilerBundle] Display trace and context in the logger profiler Fixing a bug where if a core class was autowired, autowiring tried to autowire optional args as if they were required
* 3.4: [DI] use assertStringEqualsFile when possible [VarDumper] Adapt to php 7.2 changes [DI] Fix using private services in expressions [Form][TwigBridge] Don't render _method in form_rest() for a child form [Form] Static call TimezoneType::getTimezones Removed references for non existent validator constraints Suggest using quotes instead of Yaml::PARSE_KEYS_AS_STRINGS [DI] Fix test [Cache] Handle unserialization failures for Memcached Remove unused prop + added@deprecated Remove unused mocks/vars featuresymfony#22317 [Console] Make SymfonyQuestionHelper::ask optional by default (ro0NL) [DoctrineBridge][PropertyInfo] Added support for Doctrine Embeddables [Validator] Fix IbanValidator for ukrainian IBANs Router: allow HEAD method to be defined first [WebProfilerBundle] Display trace and context in the logger profiler Fixing a bug where if a core class was autowired, autowiring tried to autowire optional args as if they were required
i noticed when writing commands i always keep doing
only to bypass a built-in validation, of which im not sure why it's there. Note the base question helper doesnt make this assumption...