-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Console] Both ansi and no-ansi return false by default #41794
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
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
In summary: before 5.2, both Having 3 states for a negatable option make sense: for instance
How can we fix the
😐 We believe that
👍 the cleanest way to say
This is the behavior of this PR, and I'm not sure to like it. 👍 Restore the behavior of 5.2 and fix the BC
👍 Fix issue in 5.2 |
|
My personal opinion: 6 months have past since this BC break, so is it really worth to still complicate the API to fix it? (imho, everything different than the current behavior is less expected) This is yet another example of how everyone can improve Symfony (and indirectly many projects not related to Symfony but using it's components) by testing out pre-releases. |
this is the option At the moment, they can not know if user choose |
So, to make it clear, this is what I would expect (and IIRC, this is the current behavior?)
But apps should rely on |
The current 5.3 behaviour is as follows:
And in 5.2 and lower:
With this PR, the behaviour is the same as 5.2. Difference is |
Yes, @kbond is right. the current behavior of negatable option (when user did not enter a flag)
BUT Switching to with my PR, the behavior will be
That will fix the issue for
|
So option 2, right? My preference goes to this, since the BC break looks pretty mitigated to me and the fact it seems both the cleanest and simplest way to have an "auto" mode where the app is in charge of choosing the solution. |
Using the standard negatable behaviour (option 2) is my preference as well. Generally, I think apps should use |
This PR was merged into the 5.3 branch. Discussion ---------- [Console] Default ansi option to null | Q | A | ------------- | --- | Branch? | 5.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - This PR is an alternative to #41794 (see comment for the reasoning of this PR) to fixes laravel/tinker#127 It defaults the negatable option `--ansi` to null For the recall, having default null is interpreted like this: | flag passed | getOption(??) return | | --- | --- | | `--ansi` | `ansi = true`<br>`no-ansi = false` | | `--no-ansi` | `ansi = false`<br>`no-ansi = true` | | nothing | `ansi = null`<br>`no-ansi = null` | This could be a BC break for people that use strict comparison `getOption('ansi') === false` But: - Provide a compatible code for people that cast to bool or use weak comparison - It fixes the issue by letting know if the option has been defined by the user or is the default value (null) Here is a summary of the changes | call | 5.2 | 5.3 | this PR | | -- | --- | --- | --- | getOption('ansi') | false | false | null | getOption('no-ansi') | false | true | null | Commits ------- 8b45280 Default ansi option to null
In Symfony 5.2, both
getOption('ansi')
andgetOption('no-ansi')
returnedfalse
.Since the option is now negatable, we can not keep the same behavior, the value returned should be either
null
,true/false
orfalse/true
.This PR returns
false
when a negatable option has a default valuenull
.I'm not 100% convinced by this, as we lost
null
behavior.... Maybe we should allow 5 differents valuesnull
,true/false
,false/true
,true/true
,false/false