Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Console] Allow false as a $shortcut in InputOption#53711

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 1 commit intosymfony:5.4fromjayminsilicon:patch-1
Feb 1, 2024

Conversation

jayminsilicon
Copy link
Contributor

@jayminsiliconjayminsilicon commentedJan 31, 2024
edited by nicolas-grekas
Loading

QA
Branch?5.4
Bug fix?yes
New feature?no
Deprecations?no
IssuesFix#53702
LicenseMIT

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.

dcblogdev and Ticlext-Altihaf reacted with thumbs up emoji
@carsonbot
Copy link

Hey!

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

Symfony has acontribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (seehttps://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://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

@carsonbot

This comment was marked as resolved.

@stof
Copy link
Member

the signature for the method is sayingstring|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 usenull or an empty string to represent the absence of shortcut.

xabbuh and DavidBadura reacted with thumbs up emojiKevalPanchalSilicon reacted with eyes emoji

@nicolas-grekas
Copy link
Member

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

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

derrabus reacted with thumbs up emojijayminsilicon and dcblogdev reacted with heart emoji

@nicolas-grekasnicolas-grekas changed the title[Console] Allow 'false' as a $shortcut in InputOption.php[Console] Allow false as a $shortcut in InputOption.phpFeb 1, 2024
@nicolas-grekasnicolas-grekas changed the title[Console] Allow false as a $shortcut in InputOption.php[Console] Allow false as a $shortcut in InputOptionFeb 1, 2024
@derrabus
Copy link
Member

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

jayminsilicon and dcblogdev reacted with heart emoji

@jayminsilicon
Copy link
ContributorAuthor

jayminsilicon commentedFeb 1, 2024
edited
Loading

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

@stof
Copy link
Member

stof commentedFeb 1, 2024

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 wherefalse is a TypeError.
And maybe we should trigger a deprecation when passingfalse.

derrabus and jayminsilicon reacted with thumbs up emoji

@derrabus
Copy link
Member

derrabus commentedFeb 1, 2024
edited
Loading

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

@nicolas-grekas
Copy link
Member

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

derrabus reacted with thumbs up emoji

@derrabus
Copy link
Member

Okay, so only a test then. 🙂

@jayminsilicon
Copy link
ContributorAuthor

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

@nicolas-grekas
Copy link
Member

Thank you@jayminsilicon.

jayminsilicon and dcblogdev reacted with heart emojiderrabus reacted with rocket emoji

@nicolas-grekasnicolas-grekas merged commitf0a9916 intosymfony:5.4Feb 1, 2024
@jayminsilicon
Copy link
ContributorAuthor

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

@nicolas-grekas
Copy link
Member

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
Copy link
ContributorAuthor

@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
Copy link

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
Copy link
Contributor

yivi commentedFeb 1, 2024

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

suryakaritva, dcblogdev, jayminsilicon, and sergeich5 reacted with thumbs up emoji

@derrabus
Copy link
Member

{"require": {"symfony/console":"~5.4.36@dev"    }}
jayminsilicon reacted with heart emoji

@jayminsilicon
Copy link
ContributorAuthor

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

@yivi
Copy link
Contributor

yivi commentedFeb 1, 2024

@jayminsilicon the only problem with Derrabus approach, is that you would need to manually remove thatrequire 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, oliworx, and Thijzer reacted with thumbs up emoji

@jayminsilicon
Copy link
ContributorAuthor

@yivi Yes that is correct ! :)

@derrabus
Copy link
Member

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. 🤷

This was referencedFeb 27, 2024
github-merge-queuebot pushed a commit to Lendable/composer-license-checker that referenced this pull requestFeb 29, 2024
[![MendRenovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)This PR contains the following updates:| Package | Change | Age | Adoption | Passing | Confidence ||---|---|---|---|---|---|| [symfony/console](https://symfony.com)([source](https://togithub.com/symfony/console)) | `6.4.3` -> `6.4.4` |[![age](https://developer.mend.io/api/mc/badges/age/packagist/symfony%2fconsole/6.4.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![adoption](https://developer.mend.io/api/mc/badges/adoption/packagist/symfony%2fconsole/6.4.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![passing](https://developer.mend.io/api/mc/badges/compatibility/packagist/symfony%2fconsole/6.4.3/6.4.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![confidence](https://developer.mend.io/api/mc/badges/confidence/packagist/symfony%2fconsole/6.4.3/6.4.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|| [symfony/process](https://symfony.com)([source](https://togithub.com/symfony/process)) | `6.4.3` -> `6.4.4` |[![age](https://developer.mend.io/api/mc/badges/age/packagist/symfony%2fprocess/6.4.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![adoption](https://developer.mend.io/api/mc/badges/adoption/packagist/symfony%2fprocess/6.4.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![passing](https://developer.mend.io/api/mc/badges/compatibility/packagist/symfony%2fprocess/6.4.3/6.4.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![confidence](https://developer.mend.io/api/mc/badges/confidence/packagist/symfony%2fprocess/6.4.3/6.4.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|---### Release Notes<details><summary>symfony/console (symfony/console)</summary>### [`v6.4.4`](https://togithub.com/symfony/console/releases/tag/v6.4.4)[CompareSource](https://togithub.com/symfony/console/compare/v6.4.3...v6.4.4)**Changelog**(symfony/console@v6.4.3...v6.4.4)- bug[symfony/symfony#54009](https://togithub.com/symfony/symfony/issues/54009)\[Console] Fix display of vertical Table on Windows OS([@&#8203;VincentLanglet](https://togithub.com/VincentLanglet))- bug[symfony/symfony#54001](https://togithub.com/symfony/symfony/issues/54001)\[Console] Fix display of Table on Windows OS([@&#8203;VincentLanglet](https://togithub.com/VincentLanglet))- bug[symfony/symfony#53707](https://togithub.com/symfony/symfony/issues/53707)\[Console] Fix color support for TTY output([@&#8203;theofidry](https://togithub.com/theofidry))- bug[symfony/symfony#53711](https://togithub.com/symfony/symfony/issues/53711)\[Console] Allow false as a $shortcut in InputOption([@&#8203;jayminsilicon](https://togithub.com/jayminsilicon))</details><details><summary>symfony/process (symfony/process)</summary>###[`v6.4.4`](https://togithub.com/symfony/process/compare/v6.4.3...v6.4.4)[CompareSource](https://togithub.com/symfony/process/compare/v6.4.3...v6.4.4)</details>---### Configuration📅 **Schedule**: Branch creation - At any time (no schedule defined),Automerge - At any time (no schedule defined).🚦 **Automerge**: Enabled.♻ **Rebasing**: Whenever PR becomes conflicted, or you tick therebase/retry checkbox.🔕 **Ignore**: Close this PR and you won't be reminded about theseupdates again.---- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, checkthis box---This PR has been generated by [MendRenovate](https://www.mend.io/free-developer-tools/renovate/). Viewrepository job log[here](https://developer.mend.io/github/Lendable/composer-license-checker).<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yMTIuMCIsInVwZGF0ZWRJblZlciI6IjM3LjIxMi4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
github-merge-queuebot pushed a commit to Lendable/composer-license-checker that referenced this pull requestMar 2, 2024
[![MendRenovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)This PR contains the following updates:| Package | Change | Age | Adoption | Passing | Confidence ||---|---|---|---|---|---|| [symfony/console](https://symfony.com)([source](https://togithub.com/symfony/console)) | `6.4.4` -> `7.0.4` |[![age](https://developer.mend.io/api/mc/badges/age/packagist/symfony%2fconsole/7.0.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![adoption](https://developer.mend.io/api/mc/badges/adoption/packagist/symfony%2fconsole/7.0.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![passing](https://developer.mend.io/api/mc/badges/compatibility/packagist/symfony%2fconsole/6.4.4/7.0.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![confidence](https://developer.mend.io/api/mc/badges/confidence/packagist/symfony%2fconsole/6.4.4/7.0.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|| [symfony/process](https://symfony.com)([source](https://togithub.com/symfony/process)) | `6.4.4` -> `7.0.4` |[![age](https://developer.mend.io/api/mc/badges/age/packagist/symfony%2fprocess/7.0.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![adoption](https://developer.mend.io/api/mc/badges/adoption/packagist/symfony%2fprocess/7.0.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![passing](https://developer.mend.io/api/mc/badges/compatibility/packagist/symfony%2fprocess/6.4.4/7.0.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|[![confidence](https://developer.mend.io/api/mc/badges/confidence/packagist/symfony%2fprocess/6.4.4/7.0.4?slim=true)](https://docs.renovatebot.com/merge-confidence/)|---### Release Notes<details><summary>symfony/console (symfony/console)</summary>### [`v7.0.4`](https://togithub.com/symfony/console/releases/tag/v7.0.4)[CompareSource](https://togithub.com/symfony/console/compare/v7.0.3...v7.0.4)**Changelog**(symfony/console@v7.0.3...v7.0.4)- bug[symfony/symfony#54009](https://togithub.com/symfony/symfony/issues/54009)\[Console] Fix display of vertical Table on Windows OS([@&#8203;VincentLanglet](https://togithub.com/VincentLanglet))- bug[symfony/symfony#54001](https://togithub.com/symfony/symfony/issues/54001)\[Console] Fix display of Table on Windows OS([@&#8203;VincentLanglet](https://togithub.com/VincentLanglet))- bug[symfony/symfony#53707](https://togithub.com/symfony/symfony/issues/53707)\[Console] Fix color support for TTY output([@&#8203;theofidry](https://togithub.com/theofidry))- bug[symfony/symfony#53711](https://togithub.com/symfony/symfony/issues/53711)\[Console] Allow false as a $shortcut in InputOption([@&#8203;jayminsilicon](https://togithub.com/jayminsilicon))### [`v7.0.3`](https://togithub.com/symfony/console/releases/tag/v7.0.3)[CompareSource](https://togithub.com/symfony/console/compare/v7.0.2...v7.0.3)**Changelog**(symfony/console@v7.0.2...v7.0.3)- bug[symfony/symfony#53516](https://togithub.com/symfony/symfony/issues/53516)\[Console] Allow '0' as a $shortcut in InputOption.php([@&#8203;lawsonjl-ornl](https://togithub.com/lawsonjl-ornl))- bug[symfony/symfony#53576](https://togithub.com/symfony/symfony/issues/53576)\[Console] Only execute additional checks for color support if theoutput ([@&#8203;theofidry](https://togithub.com/theofidry))### [`v7.0.2`](https://togithub.com/symfony/console/releases/tag/v7.0.2)[CompareSource](https://togithub.com/symfony/console/compare/v7.0.1...v7.0.2)**Changelog**(symfony/console@v7.0.1...v7.0.2)- bug[symfony/symfony#52940](https://togithub.com/symfony/symfony/issues/52940)\[Console] Fix color support check on non-Windows platforms([@&#8203;theofidry](https://togithub.com/theofidry))- bug[symfony/symfony#52941](https://togithub.com/symfony/symfony/issues/52941)\[Console] Fix xterm detection([@&#8203;theofidry](https://togithub.com/theofidry))### [`v7.0.1`](https://togithub.com/symfony/console/releases/tag/v7.0.1)[CompareSource](https://togithub.com/symfony/console/compare/v7.0.0...v7.0.1)**Changelog**(symfony/console@v7.0.0...v7.0.1)-   no significant changes### [`v7.0.0`](https://togithub.com/symfony/console/releases/tag/v7.0.0)[CompareSource](https://togithub.com/symfony/console/compare/v6.4.4...v7.0.0)**Changelog**(symfony/console@v7.0.0-RC2...v7.0.0)-   no significant changes</details><details><summary>symfony/process (symfony/process)</summary>### [`v7.0.4`](https://togithub.com/symfony/process/releases/tag/v7.0.4)[CompareSource](https://togithub.com/symfony/process/compare/v7.0.3...v7.0.4)**Changelog**(symfony/process@v7.0.3...v7.0.4)- bug[symfony/symfony#54006](https://togithub.com/symfony/symfony/issues/54006)\[Process] Fix the `command -v` exception (@&#8203;kayw-geek)- bug[symfony/symfony#53821](https://togithub.com/symfony/symfony/issues/53821)\[Process] Fix Inconsistent Exit Status in proc_get_status for PHPVersions Below 8.3 ([@&#8203;Luc45](https://togithub.com/Luc45))### [`v7.0.3`](https://togithub.com/symfony/process/releases/tag/v7.0.3)[CompareSource](https://togithub.com/symfony/process/compare/v7.0.2...v7.0.3)**Changelog**(symfony/process@v7.0.2...v7.0.3)- bug[symfony/symfony#53481](https://togithub.com/symfony/symfony/issues/53481)\[Process] Fix executable finder when the command starts with a dash([@&#8203;kayw-geek](https://togithub.com/kayw-geek))### [`v7.0.2`](https://togithub.com/symfony/process/releases/tag/v7.0.2)[CompareSource](https://togithub.com/symfony/process/compare/v7.0.0...v7.0.2)**Changelog**(symfony/process@v7.0.1...v7.0.2)- bug[symfony/symfony#52864](https://togithub.com/symfony/symfony/issues/52864)\[HttpClient]\[Mailer]\[Process] always pass microseconds to usleep asintegers ([@&#8203;xabbuh](https://togithub.com/xabbuh))### [`v7.0.0`](https://togithub.com/symfony/process/releases/tag/v7.0.0)[CompareSource](https://togithub.com/symfony/process/compare/v6.4.4...v7.0.0)**Changelog**(symfony/process@v7.0.0-RC2...v7.0.0)-   no significant changes</details>---### Configuration📅 **Schedule**: Branch creation - At any time (no schedule defined),Automerge - At any time (no schedule defined).🚦 **Automerge**: Enabled.♻ **Rebasing**: Whenever PR becomes conflicted, or you tick therebase/retry checkbox.🔕 **Ignore**: Close this PR and you won't be reminded about theseupdates again.---- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, checkthis box---This PR has been generated by [MendRenovate](https://www.mend.io/free-developer-tools/renovate/). Viewrepository job log[here](https://developer.mend.io/github/Lendable/composer-license-checker).<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yMjAuMiIsInVwZGF0ZWRJblZlciI6IjM3LjIyMC4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@GromNaNGromNaNGromNaN approved these changes

@chalasrchalasrAwaiting requested review from chalasrchalasr is a code owner

Assignees
No one assigned
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.

8 participants
@jayminsilicon@carsonbot@stof@nicolas-grekas@derrabus@suryakaritva@yivi@GromNaN

[8]ページ先頭

©2009-2025 Movatter.jp