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

Jump to bottom

[Console] Default ansi option to null #44176

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
+12 −11

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 Nov 21, 2021
@carsonbot carsonbot added this to the 5.3 milestone Nov 21, 2021
kbond
kbond approved these changes Nov 21, 2021
View changes
@carsonbot carsonbot changed the title Default ansi option to null [Console] Default ansi option to null Nov 21, 2021
@ogizanagi
Copy link
Member

@ogizanagi ogizanagi commented Nov 21, 2021

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

Loading

@chalasr
Copy link
Member

@chalasr 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.

Loading

@jderusse
Copy link
Member Author

@jderusse jderusse commented Nov 22, 2021

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.

Loading

@ogizanagi
Copy link
Member

@ogizanagi ogizanagi commented Nov 22, 2021

Thanks @jderusse.

Loading

@ogizanagi ogizanagi merged commit a4f132e into symfony:5.3 Nov 22, 2021
8 of 11 checks passed
Loading
@fabpot fabpot mentioned this pull request Nov 22, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbond kbond

@nicolas-grekas nicolas-grekas

@GromNaN GromNaN

@derrabus derrabus

@ogizanagi ogizanagi

@chalasr chalasr

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

Successfully merging this pull request may close these issues.

None yet

8 participants
@jderusse @ogizanagi @chalasr @kbond @nicolas-grekas @GromNaN @derrabus @carsonbot