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

Properly handle input options #1793

Merged

Conversation

kick-the-bucket
Copy link
Contributor

Q A
Bug fix? Yes
New feature? No
BC breaks? No
Deprecations? No
Fixed tickets #1792 comment

@kick-the-bucket kick-the-bucket force-pushed the fix-value-none-option-handling branch from b4f76d9 to c04f4a9 Compare January 15, 2019 16:02
@kick-the-bucket kick-the-bucket force-pushed the fix-value-none-option-handling branch from c04f4a9 to 30c8b21 Compare January 15, 2019 16:05
@antonmedv
Copy link
Member

Awesome!

@kick-the-bucket
Copy link
Contributor Author

kick-the-bucket commented Jan 15, 2019

Looks like it's still handling the array options wrong in some edge cases, but I'll try to fix those as well.
Both nulls and empty strings are legal array-optional values and an empty string is a legal array-required value, but all these values are currently ignored/stripped from the original input.

@antonmedv
Copy link
Member

@kick-the-bucket so? How it's going? Maybe delete declare(strict_types=1); if it helps?

Copy link
Contributor

@rvitaliy rvitaliy left a comment

Choose a reason for hiding this comment

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

LGTM!
minor changes required, not mandatory

src/Executor/ParallelExecutor.php Outdated Show resolved Hide resolved
src/Console/Input/Option.php Outdated Show resolved Hide resolved
test/src/Console/Input/OptionTest.php Outdated Show resolved Hide resolved
@rvitaliy
Copy link
Contributor

@kick-the-bucket
I did a quick analysis
👍 both null and empty string in an array can be managed.
keep in mind that both will be transformed into empty string.
we can't pass null by command 😢

@kick-the-bucket
Copy link
Contributor Author

kick-the-bucket commented Jan 16, 2019

@kick-the-bucket so? How it's going? Maybe delete declare(strict_types=1); if it helps?

The option handling issues are logical, rather than type-related, so sadly this won't help.
Option handling might have never been fully correct (it certainly never worked with array options)...

@rvitaliy I did find a way to pass null to both VALUE_OPTIONAL and VALUE_IS_ARRAY | VALUE_OPTIONAL- it has to be either:

  • the last option in the list, with no equals sign or space following
  • followed by a space (but no equals sign) and another option

So it looks like the only thing that needs to be done for null values is to remove the equals sign.

@kick-the-bucket kick-the-bucket force-pushed the fix-value-none-option-handling branch 2 times, most recently from 13d8719 to 933a1ef Compare January 16, 2019 10:18
@kick-the-bucket kick-the-bucket force-pushed the fix-value-none-option-handling branch from 933a1ef to 8ce9f92 Compare January 16, 2019 10:43
@antonmedv
Copy link
Member

So, everything looks ok to me. @kick-the-bucket @rvitaliy merging?

And please update changelog by php bin/changelog command.

Copy link
Contributor

@rvitaliy rvitaliy left a comment

Choose a reason for hiding this comment

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

InputOption::VALUE_REQUIRED
accept empty string but not null

test/src/Console/Input/OptionTest.php Outdated Show resolved Hide resolved
test/src/Console/Input/OptionTest.php Outdated Show resolved Hide resolved
test/src/Console/Input/OptionTest.php Show resolved Hide resolved
src/Console/Input/Option.php Outdated Show resolved Hide resolved
@kick-the-bucket kick-the-bucket changed the title Properly handle VALUE_NONE options Properly handle input options Jan 16, 2019
@kick-the-bucket
Copy link
Contributor Author

Looks like all possible options values are now being handled correctly which was probably never the case since the feature was implemented almost 3 years ago 😅

@rvitaliy
Copy link
Contributor

LGTM 🍻

@antonmedv antonmedv merged commit c27aa03 into deployphp:master Jan 17, 2019
@kick-the-bucket kick-the-bucket deleted the fix-value-none-option-handling branch January 17, 2019 09:52
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.

3 participants