Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

• Always add tests
• Keep backward compatibility (see https://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
• Features and deprecations must be submitted against the 5.4 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

I understand your issue but I think this is not the best solution to revert changes to the ansi option. In my opinion, you should apply the same check as here with the negatable options.
Please provide also a test for this issue.

I'd also prefer to not revert the change.

I understand that on Symfony 5.2, adding a --no-ansi option was kind-of a valid thing to do because the component did not specify it. Can we find a way to gracefully allow commands to do that and maybe trigger a deprecation in that case?

edit: I was wrong here, --no-ansi did exist on 5.2.

@YaFou I had also checked to add the check (https://github.com/symfony/console/blob/5.3/Input/InputDefinition.php#L233), beacuse I add an option with the name no-ansi it is comparing with the name ansi and then the check will return false, That is not working,

Reproduce the bug. Create a command class, in the configure method set $this->addOption('no-ansi', null, InputOption::VALUE_NONE, 'Disable ANSI output'), you can also try it with the negatable, will be the same result.

@YaFou and @derrabus another solution which comes to my mind is removing the check if it exists on negatable. What do you think?

Well, creating the same option in a command and in the application is something we should forbid, as things will break in subtle ways if the 2 places don't apply the same config on the option.

@stof true point, that is why I would propose a revert, becasue then I can add someting twice with the same settings. which aint possible for a negetable

I agree with the revert in 5.3, but I think 5.4 should deprecated overriding application-level options (and 6.0 should forbid it), to avoid such subtle breakages.

Why not deprecate it now @stof? Maybe because it is on a maintenance branch?

Maybe because it is on a maintenance branch?

yes. Deprecations cannot be introduced in patch releases

I agree with the revert in 5.3, but I think 5.4 should deprecated overriding application-level options (and 6.0 should forbid it), to avoid such subtle breakages.

If we do this, I'd also like to switch back to negatable on 6.0.

Before merging this PR in 5.3, could you please open a PR in 5.4 that deprecate overriding an option, and allow us to revert this PR in 6.0?

👍 for merging this. This issue is causing a lot of issues for Laravel users, which make up a large number of monthly downloads of this component.

Better to break code that's only been here for a few weeks, than to break code that's been here for years.

Hmm.. I dug into the issue and started working on a fix (#41792), but I realized that this does not make sense.

1. In Symfony <5.3, You were not allowed to redefine an option UNLESS the options are exactly identical. The single change that is allowed is the description.
2. The ansi and no-ansi are defined in the Command's definition, which means these are applied AFTER the user's command option (see

   symfony/src/Symfony/Component/Console/Command/Command.php

   Lines 360 to 361 in f12a419

   	$this->fullDefinition->setOptions($this->definition->getOptions());
   	$this->fullDefinition->addOptions($this->application->getDefinition()->getOptions());

   ), which means they override the user's definition

Both previous points leads to: in symfony < 5.3, adding an option ansi or --no-ansi has no effect.

This issue is causing a lot of issues for Laravel users, which make up a large number of monthly downloads of this component.

Do you have such issue @GrahamCampbell ? because laravel/tinker#127 is not related.
in laravel/tinker#127 the issue is: previously both ansi and --no-ansi return false. While now no-ansi returns true.

This issue should be addressed by #41794