8000 [Console] QuestionHelper: add possibility to ask for a value starting or ending with whitespace · Issue #23210 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Console] QuestionHelper: add possibility to ask for a value starting or ending with whitespace #23210

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.

Sign up for GitHub

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

Jump to bottom
Closed
sustmi opened this issue Jun 16, 2017 · 8 comments
Closed

[Console] QuestionHelper: add possibility to ask for a value starting or ending with whitespace #23210

sustmi opened this issue Jun 16, 2017 · 8 comments
Labels
Console Feature

Comments

@sustmi
Copy link
Contributor
sustmi commented Jun 16, 2017 •
edited
Loading
Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? no
Symfony version all since v2.6 (336bba2)

Currently, QuestionHelper trims all answers (see eg. https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Console/Helper/QuestionHelper.php#L108 ). Can we add a possibility to ask for a value that starts or ends with whitespace?

The actual usecase is for example asking for a password. We do not want to restrict users to use only passwords that do not start or end with whitespace.
PasswordType has this feature by default - see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Extension/Core/Type/PasswordType.php#L38 .

The QuestionHelper::doAsk() method is already quite complex but I guess we could add some logic there to check whether Question has flag trimAnswer set or not.

What do you think?
@sustmi
Copy link
Contributor Author
sustmi commented Jun 16, 2017

Note: I can make a pull-request if it is desirable.

@javiereguiluz
Copy link
Member

This feature request is really tricky. I'm not sure if I'm 👍 or 👎 .

However, for the mentioned use case, I'm strongly 👎 . Using passwords with leading or trailing white spaces is so fragile, that it will "never" work. If that's your use case, keep using the current QuestionHelper and remember to trim white spaces if those passwords are set via web too.

@xabbuh xabbuh added the Feature label Jun 18, 2017
@Nek-
Copy link
Contributor
Nek- commented Jun 19, 2017 •
edited
Loading

@javiereguiluz I disagree with your point on the use case. Just let people do what they want with password and the world will be better 🙃. (especially when it's "weird")

I don't see the problem with having a configurable option on the Symfony\Component\Console\Question class.

@javiereguiluz
Copy link
Member

@Nek- I disagree with you. Look around and you'll see that no serious web site let users do what they want with their passwords. They require you to use a min/max length, they require you to use certain characters, the password cannot include the login, etc. Yes, some of those rules are stupid ... but most of them are great.

In my opinion, not allowing (or silently ignoring) the leading and trailing white spaces is a good thing to avoid issues.

@sustmi
Copy link
Contributor Author
sustmi commented Jul 22, 2017 •
edited
Loading

Ok, so I crawled the internet a little bit and I found there already are many discussions on similar topic:

I came to a conclusion that there is no clear answer to this. As a framework Symfony should provide a way to handle both the trimming and the non-trimming use-case but I feel that this could make the QuestionHelper class too complicated.

We should at least be transparent about the trimming. So I propose mentioning it in the ask() comment: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Console/Helper/QuestionHelper.php#L45
For example: @return mixed The user answer trimmed of whitespace characters.

What do you think?

@chalasr
Copy link
Member
chalasr commented Nov 3, 2017 •
edited
Loading

We know that spaces in passwords can lead to weird behaviors, but as said the Form PasswordType itself is flexible on that point.
Pushing to a good practice and mentioning the risks when not following it is a good thing, even make it default, but forcing to follow is not in my opinion. Nobody forbids you to store passwords in clear for instance, but we strongly recommend to not do so.
Making this opt-in as a feature looks sensible to me, it should be up to the users to trim user inputs or not, providing good defaults of course (required for BC anyway).

@Simperfit
Copy link
Contributor

I think we should not implement this too.

@chalasr
Copy link
Member
chalasr commented May 26, 2019

PR welcome to add a flag on Question that indicates if the answer should be trimmed or no with default to true. Closing due to the lack of activity here.

@chalasr chalasr closed this as completed May 26, 2019
@Simperfit Simperfit mentioned this issue May 26, 2019
Merged
fabpot added a commit that referenced this issue Jul 8, 2019
@fabpot
feature #31626 [Console] allow answer to be trimmed by adding a flag …
088270e 
…(Simperfit)

This PR was merged into the 4.4 branch.

Discussion
----------

[Console] allow answer to be trimmed by adding a flag

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | none   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#11603 <!-- required for new features -->

<!--
Replace this notice by a short README for your feature/bugfix. This will help people
understand your PR and can be used as a start for the documentation.

Additionally (see https://symfony.com/roadmap):
 - Bug fixes must be submitted against the lowest maintained 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.
-->

According to #23210 (comment) we add a new flag in the `Question` class to be able to not trim the answer.

Commits
-------

8f182d8 [Console] allow answer to be trimmed by adding a flag
symfony-splitter pushed a commit to symfony/console that referenced this issue Jul 8, 2019
@fabpot
feature #31626 [Console] allow answer to be trimmed by adding a flag …
257fc20 
…(Simperfit)

This PR was merged into the 4.4 branch.

Discussion
----------

[Console] allow answer to be trimmed by adding a flag

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | none   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#11603 <!-- required for new features -->

<!--
Replace this notice by a short README for your feature/bugfix. This will help people
understand your PR and can be used as a start for the documentation.

Additionally (see https://symfony.com/roadmap):
 - Bug fixes must be submitted against the lowest maintained 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.
-->

According to symfony/symfony#23210 (comment) we add a new flag in the `Question` class to be able to not trim the answer.

Commits
-------

8f182d811e [Console] allow answer to be trimmed by adding a flag
This was referenced Jul 24, 2019
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Console Feature
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@javiereguiluz @sustmi @Nek- @xabbuh @Simperfit @chalasr
0