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

Add hasOptions(array) to Arguments concern. #1

Closed
wants to merge 3 commits into from
Closed

Add hasOptions(array) to Arguments concern. #1

wants to merge 3 commits into from

Conversation

bowyern
Copy link
Contributor

@bowyern bowyern commented Dec 19, 2019

I found myself wanting to do something similar to Laravel's $request->has(['param1', 'param2']) and noticed Arguments::hasOption doesn't support arrays. Adding a plural method felt better than adding array handling to hasOption, but either works for my purposes.

I didn't see any tests for hasOption so I didn't add one for this function.

@dalabarge
Copy link
Contributor

Not sure how I missed this PR. I think the way I'd handle it though is to make hasOption() support either a string, array of strings, or multiple string parameters. The biggest challenge though is determining if the logic should be to check if it has any or has all. For example you might want to check that all the options are present or that at least one option is present. This isn't supported currently by Arr::has() as it assumes that all keys given must be present to be considered truthful. Therefore I'm thinking it might make more sense to just rely directly on Arr::has($this->arguments(), ['option1', 'option2']) within project-land code.

If you feel strongly that this package should include the helper logic then I'd suggest maintaining the single hasOption() method and would welcome a refactor of hasOption(string $name) to hasOption($name) with some func_num_args() and is_array() checking to determine if the method was called with multiple string arguments, an array of names, or a single string name then once you have the $names to check pass it off to Arr::has($this->arguments(), $names).

If you can add a unit test to make sure the forward and backward compatible logic is handled then we can also probably refactor this to call to Arr statics instead of legacy array_ functions.

@dalabarge dalabarge closed this Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants