Skip to content

[Console] Negatable option are null by default #40986

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

Merged
merged 1 commit into from
Apr 29, 2021

Conversation

jderusse
Copy link
Member

@jderusse jderusse commented Apr 29, 2021

Q A
Branch? 5.3
Bug fix? yes
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

given the command

class TestCommand extends Command
{
    protected static $defaultName = 'test';

    protected function configure()
    {
        $this->addOption('ansi', null, InputOption::VALUE_NEGATABLE, 'Force/disable ANSI output');
    }

    protected function execute(InputInterface $input, OutputInterface $output): int
    {
        dump($input->getOption('ansi'));

        return Command::SUCCESS;
    }
}
call result
bin/console test --no-ansi false
bin/console test --ansi true
bin/console test null

@jderusse jderusse requested a review from chalasr as a code owner April 29, 2021 10:40
@carsonbot carsonbot changed the title [console] Negatable option are null by default [Console] Negatable option are null by default Apr 29, 2021
Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this! It turns "negatable options" into tri-state options, which is something that was asked for by some developers. Thanks Jérémy!

Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change means that $this->getOption('ansi') can now return null, which is a BC break as it always returns a bool right now. Maybe we should have false as a default for this internal flag (no need for 3 states here anyway).

@javiereguiluz
Copy link
Member

The main difference is about "What's a negatable option?"

(1) A single option with two behaviors (yes/no)
(2) Two different options (yes, no) but defined as a single one for convenience

In case (1), we need a tri-state to differentiate these cases:

$useAnsi = $input->getOption('ansi');
if (true === $useAnsi) {
    // force ANSI usage
} elseif (false === $useAnsi) {
    // don't use ANSI
} elseif (null === $useAnsi) {
    // user didn't pass any ANSI preference;
    // we must decide what to do
}

In case (2), you only need two cases (the third one happens automatically):

$useAnsi = $input->getOption('ansi');
$dontUseAnsi = $input->getOption('no-ansi');
if ($useAnsi) {
    // force ANSI usage
} elseif ($dontUseAnsi) {
    // don't use ANSI
} else {
    // user didn't pass any ANSI preference;
    // we must decide what to do
}

@jderusse
Copy link
Member Author

The BC break has been solved by setting the default to false for OUR --ansi option.
In that way:

  • using getOption('ansi') will still return false
  • people can use InputOption::VALUE_NEGATABLE with a default null value and leverage the 3 states of the option.

@fabpot
Copy link
Member

fabpot commented Apr 29, 2021

Thank you @jderusse.

@fabpot fabpot merged commit 694c052 into symfony:5.x Apr 29, 2021
@fabpot fabpot mentioned this pull request May 1, 2021
@jderusse jderusse deleted the negatable-null branch May 12, 2021 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants