[Console] Allow false as a $shortcut in InputOption

[jayminsilicon]

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	Fix #53702
License	MIT

This change is needed for laravel 8.x installation to allow running kernel and module based architecture. Right now our deployments are suddenly stopped since morning all saying "An option shortcut cannot be empty" and root cause for this is $shortcut is coming as false from laravel modules feature library.

[carsonbot]

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 7.1 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

[stof]

the signature for the method is saying string|null as values for shortcuts. false has never been a value covered by our contract (and it is a TypeError in Symfony 6.4 and 7).

So -1 for that PR. You should instead fix the code to properly use null or an empty string to represent the absence of shortcut.

[nicolas-grekas]

We used to use empty() on this line so it's fair to me to not break existing behaviors, especially when bool is allowed by the signature.

I'm fine with the change but a test case would be nice.

[derrabus]

I'm also fine with this change, if we ignore it on the merge-up to 6.4.

[jayminsilicon]

@derrabus Yes, that would be helpful for everyone who was using 5.4 as stable version for their repo.

[stof]

Well, as things were working before (due to implementation details), I'm OK patching that in 5.4, but I'm not OK up-merging it to 6.4 where false is a TypeError.
And maybe we should trigger a deprecation when passing false.

[derrabus]

@jayminsilicon Can you add a test for your scenario and look into @stof's proposal of triggering a deprecation?

[nicolas-grekas]

No new deprecations on 5.4, our policies don't allow it (as they don't allow new features on older branches)

[derrabus]

Okay, so only a test then. 🙂

[jayminsilicon]

@stof @derrabus Testcase added in PR. Now all should good to merge this PR.

[nicolas-grekas]

Thank you @jayminsilicon.

[jayminsilicon]

@nicolas-grekas Should we create release tag v 5.4.36 ?

[nicolas-grekas]

We do new releases once a month so this will be tagged in ~4 weeks.
We do faster sometimes but I'm not sure this needs an emergency fix.
At least if it does, I'd expect libraries that call this wrong to be able to also fix this on their side before.

[jayminsilicon]

@nicolas-grekas Oh, I get it. However this particular fix can solve our CI/CD deployments who are using this laravel-modules library. As many other people faces same issue since yesterday : nWidart/laravel-modules#1728. I will try to check on their side if they can fix but if they cant we need to just wait for either once as we are stuck in our deployments.
Thank you for your help so far !

[suryakaritva]

Hi, our deployments for all projects are stuck with "nwidart/laravel-modules": "^7.0", all CI/CD laravel deployments are throwing An option shortcut cannot be empty error
`> Illuminate\Foundation\ComposerScripts::postAutoloadDump

@php artisan package:discover --ansi

An option shortcut cannot be empty.

Script @php artisan package:discover --ansi handling the post-autoload-dump event returned with error code 1
`

Any quick advise you have for us to fix our CI/CD ?
thanks

[yivi]

@suryakaritva just skip this release of symfony/console: https://stackoverflow.com/a/77920770/1426539

[derrabus]

{
    "require": {
        "symfony/console": "~5.4.36@dev"
    }
}

[jayminsilicon]

Thank you @yivi and @derrabus both workaround works well for now !

[yivi]

@jayminsilicon the only problem with Derrabus approach, is that you would need to manually remove that require later in the future, so you are not "locked" to that version.

With the "conflict" approach, as soon as a new version is released you'd get it without doing anything.

[jayminsilicon]

@yivi Yes that is correct ! :)

[derrabus]

With the "conflict" approach, as soon as a new version is released you'd get it without doing anything.

Yeah, but you wouldn't know if the next release will actually fix the issue for you until it happens. 🤷