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

[5.7] Fix broken db command code #27020

Merged
merged 1 commit into from
Jan 1, 2019
Merged

[5.7] Fix broken db command code #27020

merged 1 commit into from
Jan 1, 2019

Conversation

GrahamCampbell
Copy link
Member

@GrahamCampbell GrahamCampbell commented Dec 31, 2018

We can see by the changes to the tests that when force was set to false, --false was still actually being added!!! But, we do want all sub-commands to be forced though, actually, so we should just pass true to avoid confusion, because it otherwise looks like the user would have been asked again if they want to continue for subcommands, when they are not!

The other change is to filter out the defaults, so the empty string is not passed along for no reason.

@taylorotwell taylorotwell merged commit a0b65a2 into 5.7 Jan 1, 2019
@GrahamCampbell GrahamCampbell deleted the fix-broken-command-code branch January 1, 2019 13:51
@driesvints
Copy link
Member

We can see by the changes to the tests that when force was set to false, --false was still actually being added!!!

I don't see this in the changes. Can you point out where this is? I assume with --false you mean --force?

@GrahamCampbell
Copy link
Member Author

Yeh: https://github.com/laravel/framework/pull/27020/files#diff-62bd3530c9c892a12a0b037aa6e0ad10L65. See that ['--force' => false] was resulting in --force being added, so setting it always to true maintains the same behaviour, and is way less confusing.

@driesvints
Copy link
Member

@GrahamCampbell heya, forgive me but I don't see ['--force' => false] anywhere?

@GrahamCampbell
Copy link
Member Author

'--force' => $this->option('force'),

@driesvints
Copy link
Member

@GrahamCampbell gotcha!

@GrahamCampbell
Copy link
Member Author

:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants