Skip to content

[Console] Default ansi option to null #44176

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
Merged
merged 1 commit into from
Nov 22, 2021
Merged

[Console] Default ansi option to null #44176

merged 1 commit into from
Nov 22, 2021

Conversation

jderusse
Copy link
Member

@jderusse jderusse commented Nov 21, 2021 •
edited
Loading

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
no-ansi = false
--no-ansi ansi = false
no-ansi = true
nothing ansi = null
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

@jderusse jderusse requested a review from chalasr as a code owner November 21, 2021 00:11
@carsonbot carsonbot added this to the 5.3 milestone Nov 21, 2021
@carsonbot carsonbot changed the title Default ansi option to null [Console] Default ansi option to null Nov 21, 2021
@ogizanagi
Copy link
Contributor

We should probably mention this BC break in the CHANGELOG file though

@chalasr
Copy link
Member

chalasr commented Nov 21, 2021 •
edited
Loading

Either it's a BC break or it's a bugfix, but not both.
If we consider this a BC break, we should merge it as a feature and document in the CHANGELOG. Otherwise, it should just be merged as-is with no CHANGELOG update.

Do libs impacted by the previous behavior change that this PR aims to fix (psysh, tinker) have changed their code meanwhile? That may help taking the right decision here.

@jderusse
Copy link
Member Author

Do libs impacted by the previous behavior change that this PR aims to fix (psysh, tinker) have changed their code meanwhile? That may help taking the right decision here.

psysh didn't change the way it parse options
https://github.com/bobthecow/psysh/blob/29ad19f99ece710577ec9f9022276254f4c8f03e/src/Configuration.php#L239-L246

Switching false to null fixes their issue because they don't use strict comparison.

@ogizanagi
Copy link
Contributor

Thanks @jderusse.

@ogizanagi ogizanagi merged commit a4f132e into symfony:5.3 Nov 22, 2021
This was referenced Nov 22, 2021
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbond kbond kbond approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@GromNaN GromNaN GromNaN approved these changes

@derrabus derrabus derrabus approved these changes

@ogizanagi ogizanagi ogizanagi approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Bug Console Status: Reviewed
Projects
None yet
Milestone
5.3
Development

Successfully merging this pull request may close these issues.
8 participants
@jderusse @ogizanagi @chalasr @kbond @nicolas-grekas @GromNaN @derrabus @carsonbot