8000 Added i18n support to ConfirmationQuestion by wouterj · Pull Request #11129 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 1 commit into from
Jan 25, 2015
Merged

Conversation

wouterj
Copy link
Member
@wouterj wouterj commented Jun 16, 2014
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.

@stof
Copy link
Member
stof commented Jun 16, 2014

there should be some validation that it is a 1-char string (otherwise it will never be true)

@wouterj
Copy link
Member Author
wouterj commented Jun 16, 2014

@stof done.

{
parent::__construct($question, (bool) $default);

$this->setNormalizer($this->getDefaultNormalizer());

if (1 === strlen($trueAnswer)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be !==

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mb_strlen instead?

Copy link
Member

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

@stof
Copy link
Member
stof commented Jun 16, 2014

This needs to be covered by tests

@nkt
Copy link
nkt commented Jun 16, 2014

@wouterj why you creates limitation on answer length? Some languages could have more than one character for "yes" word.

@stof
Copy link
Member
stof commented Jun 16, 2014

@wouterj the ConfirmationQuestion currently handles things the same way than most Unix commands do: anything starting with a y is considered as yes. If we have several chars being allowed, it means changing the way the logic is implemented, not only allowing longer strings in the current logic (which was the case in the initial version of the PR, thus matching nothing as true answer in these cases)

@wouterj
Copy link
Member Author
wouterj commented Jun 16, 2014

@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

@wouterj
Copy link
Member Author
wouterj commented Aug 30, 2014

I've updated it to allow multiple characters to compare

@jderusse
Copy link
Member

Could be interested to have a RegExp pattern with default value $trueAnsmer = '/^y/i'
And let the developer use special patterns to enforce some dangerous responses /^yes$/i
Or use multiple answers /^(yes|oui)$/i...

@@ -34,11 +38,12 @@ private function getDefaultNormalizer()
return $answer;
}

$answerIsTrue = $this->trueAnswer === strtolower(substr($answer, 0, strlen($this->trueAnswer)));
Copy link
Member

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)

@wouterj
Copy link
Member Author
wouterj commented Jan 7, 2015

Updated to use a regex, great suggestion @jeremy-derusse !

I've also added a test.

@stof
Copy link
Member
stof commented Jan 18, 2015

👍

@jderusse
Copy link
Member

@wouterj
Copy link
Member Author
wouterj commented Jan 18, 2015

See symfony/symfony-docs#4879

@fabpot
Copy link
Member
fabpot commented Jan 25, 2015

Thank you @wouterj.

@fabpot fabpot merged commit 3cb280d into symfony:master Jan 25, 2015
fabpot added a commit that referenced this pull request Jan 25, 2015
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
fabpot added a commit that referenced this pull request Jan 25, 2015
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
@wouterj wouterj deleted the patch-6 branch January 25, 2015 09:49
weaverryan added a commit to symfony/symfony-docs that referenced this pull request Jan 30, 2015
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0