-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Added i18n support to ConfirmationQuestion #11129
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
Conversation
there should be some validation that it is a 1-char string (otherwise it will never be true) |
@stof done. |
{ | ||
parent::__construct($question, (bool) $default); | ||
|
||
$this->setNormalizer($this->getDefaultNormalizer()); | ||
|
||
if (1 === strlen($trueAnswer)) { |
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 !==
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.
mb_strlen
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.
no, because the code compares it to the first byte of the input, not to the first unicode char
This needs to be covered by tests |
@wouterj why you creates limitation on answer length? Some languages could have more than one character for "yes" word. |
@wouterj the ConfirmationQuestion currently handles things the same way than most Unix commands do: anything starting with a |
@stof however, we might want to change to logic to check for the length for the given string. This way, languages which have the same start letter for yes and no can be validated nicely too |
I've updated it to allow multiple characters to compare |
Could be interested to have a RegExp pattern with default value |
@@ -34,11 +38,12 @@ private function getDefaultNormalizer() | |||
return $answer; | |||
} | |||
|
|||
$answerIsTrue = $this->trueAnswer === strtolower(substr($answer, 0, strlen($this->trueAnswer))); |
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.
$answerIsTrue = 0 === substr_compare($answer, $this->trueAnswer, 0, strlen($this->trueAnswer), true)
Updated to use a regex, great suggestion @jeremy-derusse ! I've also added a test. |
👍 |
A small doc PR (http://symfony.com/doc/current/components/console/helpers/questionhelper.html#asking-the-user-for-confirmation) will be great :) |
Thank you @wouterj. |
This PR was merged into the 3.0-dev branch. Discussion ---------- Added i18n support to ConfirmationQuestion | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | symfony/symfony-docs#4879 For instance, when creating a dutch cli app, you want this to be `j` (from "Ja") instead of the english `y`. Commits ------- 3cb280d Added i18n support to ConfirmationQuestion
This PR was submitted for the master branch but it was merged into the 2.7 branch instead (closes #11129). Discussion ---------- Added i18n support to ConfirmationQuestion | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | symfony/symfony-docs#4879 For instance, when creating a dutch cli app, you want this to be `j` (from "Ja") instead of the english `y`. Commits ------- c2f3f89 Added i18n support to ConfirmationQuestion
This PR was merged into the 2.7 branch. Discussion ---------- Documented true regex | Q | A | --- | --- | Doc fix? | no | New docs? | yes (symfony/symfony#11129) | Applies to | 2.7+ | Fixed tickets | - Commits ------- 6e6bae8 Small grammar-ish fix 050f7ce Documented true regex
For instance, when creating a dutch cli app, you want this to be
j
(from "Ja") instead of the englishy
.