Skip to content
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

ConfirmationQuestion normalizer may behave unexpectedly for Y/N question #29835

Closed
mrthehud opened this issue Jan 10, 2019 · 5 comments
Closed

Comments

@mrthehud
Copy link
Contributor

Symfony version(s) affected: 4.2

Description
When using the ConfirmationQuestion class to ask a yes / no question, if the default is true, and the answer regex is '/^y/i', then any value not starting with [yY] is considered false - except "0"

How to reproduce
Create a console application, and in the execute method:

    $question = new ConfirmationQuestion("Yes or No?", true);
    $ans = $this->io->askQuestion($question); 
    echo $ans ? 'true' : 'false';

Then run it, and observe the output:
Input: "yes", "y", "YES", "Y", "" (empty string) etc
Output: "true"
Input "no", "No", "foo", "Bar", "1", "true" etc
Output: "false"
Input: "0"
Output: "true" <- I would expect false, as the string does not match the regex

Possible Solution
https://github.com/symfony/console/blob/4a5d48d2ca2422c5c02ed37e4146fb65591369d5/Question/ConfirmationQuestion.php

    private function getDefaultNormalizer()
    {
        $default = $this->getDefault();
        $regex = $this->trueAnswerRegex;
        return function ($answer) use ($default, $regex) {
            if (\is_bool($answer)) {
                return $answer;
            }
            $answerIsTrue = (bool) preg_match($regex, $answer);
            if (false === $default) {
                return $answer && $answerIsTrue;
            }
            return !$answer || $answerIsTrue;
        };
    }

Change line 56 (last return) to:

return strlen($answer) === 0 ? $default : $answerIsTrue;
@mrthehud
Copy link
Contributor Author

I'm happy to PR this and add some tests if It's the right solution.

@ro0NL
Copy link
Contributor

ro0NL commented Jan 10, 2019

looks reasonable :) return $answerIsTrue || $default would also work isnt it, as a replacement for both returns currently.

mrthehud added a commit to mrthehud/symfony that referenced this issue Jan 11, 2019
… for answer '0'

When using the ConfirmationQuestion class to ask a yes / no question,
if the default is true, and the answer regex is '/^y/i', then any
value not starting with [yY] is considered false.
This must include "0"
mrthehud added a commit to mrthehud/symfony that referenced this issue Jan 11, 2019
… for answer '0'

When using the ConfirmationQuestion class to ask a yes / no question,
if the default is true, and the answer regex is '/^y/i', then any
value not starting with [yY] is considered false.
This must include "0"
@mrthehud
Copy link
Contributor Author

looks reasonable :) return $answerIsTrue || $default would also work isnt it, as a replacement for both returns currently.

That didn't quite work out, it broke one of the other test cases I added, but did remind me that return '' == $answer | $answerIsTrue was cleaner.

@ro0NL
Copy link
Contributor

ro0NL commented Jan 11, 2019

i find the first branch confusing :)

            if (false === $default) {
                return $answer && $answerIsTrue;
            }

$answerIsTrue implies $answer is truthy already

but im unaware of failing tests.. so perhaps my suggestion is not right :)

@mrthehud
Copy link
Contributor Author

Hmm, looks like the Appveyor tests failed, so I'll have to take a look at that when I get some time.

mrthehud added a commit to mrthehud/symfony that referenced this issue Jan 11, 2019
… for answer '0'

When using the ConfirmationQuestion class to ask a yes / no question,
if the default is true, and the answer regex is '/^y/i', then any
value not starting with [yY] is considered false.
This must include "0"
nicolas-grekas pushed a commit to mrthehud/symfony that referenced this issue Jan 25, 2019
nicolas-grekas added a commit that referenced this issue Jan 25, 2019
…true for answer '0' (mrthehud)

This PR was squashed before being merged into the 3.4 branch (closes #29844).

Discussion
----------

[Console] Fixed #29835: ConfirmationQuestion with default true for answer '0'

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | Almost all, one failure on appveyor?
| Fixed tickets | #29835
| License       | MIT
| Doc PR        | n/a

<!--
Write a short README entry for your feature/bugfix here (replace this comment block.)
This will help people understand your PR and can be used as a start of the Doc PR.
Additionally:
 - Bug fixes must be submitted against the lowest branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the master branch.
-->

When using the ConfirmationQuestion class to ask a yes / no question,
if the default is true, and the answer regex is '/^y/i', then any
value not starting with [yY] is considered false.

This must include "0", which previously would return true, producing results such as:
```
$ php bin/console do:stuff
$ Do you want to continue? 0 <enter>
$ Ok, continuing!
```

Commits
-------

a0a7400 [Console] Fixed #29835: ConfirmationQuestion with default true for answer '0'
nicolas-grekas added a commit that referenced this issue Jan 25, 2019
* 3.4:
  [HttpFoundation] Check file exists before unlink
  [Console] Fixed #29835: ConfirmationQuestion with default true for answer '0'
  [Translation] Concatenated translation messages
  [Form] ensure compatibility with older PHPUnit mocks
  [Serializer] Docblock about throwing exceptions on serializer
  [Debug][ErrorHandler] Preserve our error handler when a logger set another one
  [Form] Changed UrlType input type to text when default_protocol is not null
  [Bugfix] MemcachedSessionHandler::close() must close connection
nicolas-grekas added a commit that referenced this issue Jan 25, 2019
* 4.1:
  [HttpFoundation] Check file exists before unlink
  [Console] Fixed #29835: ConfirmationQuestion with default true for answer '0'
  [Translation] Concatenated translation messages
  [Form] ensure compatibility with older PHPUnit mocks
  [Serializer] Docblock about throwing exceptions on serializer
  [Debug][ErrorHandler] Preserve our error handler when a logger set another one
  [Form] Changed UrlType input type to text when default_protocol is not null
  [Bugfix] MemcachedSessionHandler::close() must close connection
nicolas-grekas added a commit that referenced this issue Jan 25, 2019
* 4.2:
  [HttpFoundation] Check file exists before unlink
  [Console] Fixed #29835: ConfirmationQuestion with default true for answer '0'
  [Cache] PDO-based cache pool table autocreation does not work
  [Translation] Concatenated translation messages
  [Form] ensure compatibility with older PHPUnit mocks
  [Serializer] Docblock about throwing exceptions on serializer
  [Cache] fix used variable name
  [Debug][ErrorHandler] Preserve our error handler when a logger set another one
  [Form] Changed UrlType input type to text when default_protocol is not null
  [Bugfix] MemcachedSessionHandler::close() must close connection
  Always pass $key to NullAdapter->createCacheItem
This was referenced Jan 29, 2019
sandergo90 pushed a commit to sandergo90/symfony that referenced this issue Jul 4, 2019
* 3.4:
  [HttpFoundation] Check file exists before unlink
  [Console] Fixed symfony#29835: ConfirmationQuestion with default true for answer '0'
  [Translation] Concatenated translation messages
  [Form] ensure compatibility with older PHPUnit mocks
  [Serializer] Docblock about throwing exceptions on serializer
  [Debug][ErrorHandler] Preserve our error handler when a logger set another one
  [Form] Changed UrlType input type to text when default_protocol is not null
  [Bugfix] MemcachedSessionHandler::close() must close connection
sandergo90 pushed a commit to sandergo90/symfony that referenced this issue Jul 4, 2019
* 4.1:
  [HttpFoundation] Check file exists before unlink
  [Console] Fixed symfony#29835: ConfirmationQuestion with default true for answer '0'
  [Translation] Concatenated translation messages
  [Form] ensure compatibility with older PHPUnit mocks
  [Serializer] Docblock about throwing exceptions on serializer
  [Debug][ErrorHandler] Preserve our error handler when a logger set another one
  [Form] Changed UrlType input type to text when default_protocol is not null
  [Bugfix] MemcachedSessionHandler::close() must close connection
sandergo90 pushed a commit to sandergo90/symfony that referenced this issue Jul 4, 2019
* 4.2:
  [HttpFoundation] Check file exists before unlink
  [Console] Fixed symfony#29835: ConfirmationQuestion with default true for answer '0'
  [Cache] PDO-based cache pool table autocreation does not work
  [Translation] Concatenated translation messages
  [Form] ensure compatibility with older PHPUnit mocks
  [Serializer] Docblock about throwing exceptions on serializer
  [Cache] fix used variable name
  [Debug][ErrorHandler] Preserve our error handler when a logger set another one
  [Form] Changed UrlType input type to text when default_protocol is not null
  [Bugfix] MemcachedSessionHandler::close() must close connection
  Always pass $key to NullAdapter->createCacheItem
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants