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

[Console][Feature Request] Required options #14716

Closed
zerkms opened this issue May 22, 2015 · 35 comments
Closed

[Console][Feature Request] Required options #14716

zerkms opened this issue May 22, 2015 · 35 comments
Labels

Comments

@zerkms
Copy link
Contributor

zerkms commented May 22, 2015

Disclaimer: yes, I've seen #3524 and I cannot agree with it.

The problem with current implementation is that you cannot have the required options. Even though we do have required arguments it does not save us too much, since not all possible semantics can be covered with what we currently have.

Example: we need to accept a list of files and a list of processors to run against them.

We cannot use required array arguments for both since you cannot distinguish between a file name and a processor name in a reliable way.

We cannot use required array arguments and optional array options, since the options are, well, optional in this case.

So what we do is we follow either and put constraint checks manually.

So, what prevents Symfony from having required option parameters? Are there any technical reasons against that?

@zerkms zerkms changed the title [Feature Request] Required options [Console][Feature Request] Required options May 22, 2015
@jakzal jakzal added the Console label May 22, 2015
@jakzal
Copy link
Contributor

jakzal commented May 22, 2015

@zerkms could you think of any existing command or application that requires you to provide options?

@zerkms
Copy link
Contributor Author

zerkms commented May 22, 2015

@jakzal symfony requires --env=prod in production.

PS: I know it can take it from environment variables as well, but still

@jakzal
Copy link
Contributor

jakzal commented May 22, 2015

@zerkms it's not an example of a required option. You only need to add it if you want to run a command in mode different than the default one (for example prod). It's still optional.

I was asking for an example of any command that forces you to provide an option all the time. It doesn't have to be Symfony nor php. Any command. I can't think of one, therefore I ask.

@zerkms
Copy link
Contributor Author

zerkms commented May 22, 2015

You only need to add it if you want to run a command in mode different than the default one (prod). It's still optional.

The default one is not prod. At least in the default symfony distribution. So if in production you want to run a command you MUST specify --env=prod.

I will think of it, but still - how lack of other commands prevents symfony from having such a feature?

The other commands that require more or less sophisticated configuration use config files, while I prefer to have everything in a command line, since it's more portable and much easier to manage.

@jakzal
Copy link
Contributor

jakzal commented May 22, 2015

The default one is not prod

I meant: if you want to run it in prod.

how lack of other commands prevents symfony from having such a feature?

I'd prefer to be consistent with globally accepted standards.

@zerkms
Copy link
Contributor Author

zerkms commented May 22, 2015

I meant: if you want to run it in prod.

If you want to run it in prod you MUST ALWAYS specify --env=prod

I'd prefer to be consistent with globally accepted standards.

What are they? Any reference that is accepted and explicitly/implicitly followed?

Again, having this feature does not mean that any of built-in commands will use it, but that developers will have more freedom

@jakzal
Copy link
Contributor

jakzal commented May 22, 2015

If you want to run it in prod you MUST ALWAYS specify --env=prod

The key here is IF ;)

@stof
Copy link
Member

stof commented May 22, 2015

@zerkms If you are forced to specify an option all the time (hint: this is not the case for --env as you put a IF in the sentence), it does not belong to the command options anymore. It should be an argument. Options are always optional. Otherwise it does not follow the semantic of CLI.

@jakzal
Copy link
Contributor

jakzal commented May 22, 2015

What are they? Any reference that is accepted and explicitly/implicitly followed?

Most of them comes from GNU and Unix standards. Some examples:

"Option" by definition is optional, that's what distinguishes it from an argument. That's the meaning of this word, and you probably won't find it explicitly written anywhere but dictionary.

@zerkms
Copy link
Contributor Author

zerkms commented May 22, 2015

It should be an argument

If only you can express it as an argument (which you cannot if you need to pass 2 sets as in my original example)

So guys, what stops Symfony as a general purpose framework have that and let developers to do what they want? It's an artificial limitation that does not make symfony better in any way.

"Option" by definition is optional, that's what distinguishes it from an argument. That's the meaning of this word, and you probably won't find it explicitly written anywhere but dictionary.

If it was a standard it would be there. Now we have a set of ancient historical conventions.

@saksmt
Copy link
Contributor

saksmt commented Jun 2, 2015

@jakzal The example of required options:

  • java -jar
  • nasm -f
  • dd
  • wicd-cli
  • Any app related to some list's of data that can be edited, like todo-manager
  • Any app where key specifies the mode

@xabbuh
Copy link
Member

xabbuh commented Jun 3, 2015

I can see some limited use-cases where it would make life easier if Symfony offered an option to support required options. Though I an not convinced that we should support it out if the box as it means more code that must not break while providing little value

@salemgolemugoo
Copy link

"Option" is OPTIONAL by default

@zerkms
Copy link
Contributor Author

zerkms commented Jun 7, 2015

@salemgolemugoo it depends on how you name it. I prefer the term "parameter"

The name "option" was taken from the assumption that something will stay "optional" forever, and that assumption was based on ... On what actually?

It's the logic rules - the name must express the aims, not the opposite.

@TomasVotruba
Copy link
Contributor

@zerkms I know you suggestion makes sense in logical point of view. It's just that standard way is sometimes the best way to go to prevent many WTF issues by not following it. It's drawback per benefits.

Many apps depends on standard, and we don't know how they use it. So if there is an easy manual way to do so, it's better to go with it. I've seen and made many great packages with really cool and use api methods. But they were absolutely non-standard with wast majority of others. So now they're useless and removed. They work and follow the logic still.

Solution for your use case

In ApiGen I made use required parameters like this.

apigen generate -s ./src -d ./docs

In command, delegate to options resolver.

That throws an exception, if parameters is missing.

@zerkms
Copy link
Contributor Author

zerkms commented Oct 31, 2015

@TomasVotruba what "standard" you're referring to? As far as I know - there is no one. Symfony just follows a convention that someone 30 years ago decided will be a good thing.

@TomasVotruba
Copy link
Contributor

@zerkms By "standard" I mean "people are used to use it like this for a long time".

I use sidewalks not because it's somewhere written, but because I've seen other people does it. Not because it's written somewhere.

@zerkms
Copy link
Contributor Author

zerkms commented Nov 1, 2015

@TomasVotruba and my point was to implement something that is convenient, not something that is artificially limited by a 30+ years old irrational decision.

We all like when our tools evolve: languages change with time - you do like php 5.6 and further php 7 don't you? And I'm sure if you were asked to use php 4.3.10 instead you wouldn't be that happy.

So what is the point to keep the heritage of this?

@TomasVotruba
Copy link
Contributor

@zerkms You can easily implement it by yourself.

@zerkms
Copy link
Contributor Author

zerkms commented Nov 1, 2015

@TomasVotruba I can do and I do myself a lot of things.

This issue is a proposal to move further for the tool/community.

@javiereguiluz
Copy link
Member

I'd say that we should close this issue as "won't fix".

@zerkms I agree with lots of the things you said ... but the proposed feature looks too narrow in its target. Besides, you can solve this problem by validating the options passed to the command and their values.

@TomasVotruba
Copy link
Contributor

@javiereguiluz I agree with you 👍

@zerkms
Copy link
Contributor Author

zerkms commented Jan 26, 2016

That's sad that the suggestion that will not break anything but will make symfony more flexible for an arbitrary use is discarded.

But okay, let be it.

@zerkms zerkms closed this as completed Jan 26, 2016
@Revisor
Copy link

Revisor commented Oct 20, 2016

If you reopen this request sometimes in the future, count me as a +1 vote.

I'd like to use what is now called "options" as required named arguments in order for a command to be understandable at first sight.
This
command --dir=foo --strategy=bar --processor=baz
is much more understandable than
command foo bar baz

I have made the --dir=foo options required in the code, of course, but in the command signature help they still show up as [optional].

@mattab
Copy link

mattab commented Jan 15, 2017

fyi you can add helper method to do this yourself, as we do in @Piwik

    protected function checkAllRequiredOptionsAreNotEmpty(InputInterface $input)
    {
        $options = $this->getDefinition()->getOptions();
        foreach ($options as $option) {
            $name  = $option->getName();
            $value = $input->getOption($name);
            if ($option->isValueRequired() && empty($value)) {
                throw new \InvalidArgumentException(sprintf('The required option %s is not set', $name));
            }
        }
    }

@zerkms
Copy link
Contributor Author

zerkms commented Jan 17, 2017

@mattab a minor issue in your code: the empty in the condition would cause the false negatives when the value passed is '0'

@mattab
Copy link

mattab commented Jan 17, 2017

Good spotting @zerkms - pull request welcome :-)

@edmondscommerce
Copy link

I also need required options. Fair enough not to have them by default and require a little custom code. I took the above and came up with:

    protected function checkAllRequiredOptionsAreNotEmpty(InputInterface $input)
    {
        $errors = [];
        $options = $this->getDefinition()->getOptions();
        foreach ($options as $option) {
            $name = $option->getName();
            $value = $input->getOption($name);
            if ($option->isValueRequired() && ($value === null || $value === '')) {
                $errors[] = sprintf('The required option --%s is not set or is empty', $name);
            }
        }
        if (count($errors)) {
            throw new \InvalidArgumentException(implode("\n\n", $errors));
        }
    }

edmondscommerce pushed a commit to edmondscommerce/doctrine-static-meta that referenced this issue Oct 18, 2017
@liverbool
Copy link
Contributor

Options is NOT alway OPTIONAL, When option have value they are choices we need to select one.

@liverbool
Copy link
Contributor

@edmondscommerce $option->isValueRequired() can't represent the required options it just require option's value.

It should be like this:

private function checkAllRequiredOptionsAreNotEmpty(InputInterface $input)
    {
        $errors = [];
        $option1 = $this->getDefinition()->getOption('option1');

        /** @var InputOption $option */
        foreach ([$option1] as $option) {
            $name = $option->getName();
            $value = $input->getOption($name);

            if ($value === null || $value === '' || ($option->isArray() && empty($value))) {
                $errors[] = sprintf('The required option --%s is not set or is empty', $name);
            }
        }

        if (count($errors)) {
            throw new \InvalidArgumentException(implode("\n\n", $errors));
        }
    }

@Destroy666x
Copy link

Destroy666x commented Jun 15, 2018

Lack of required named parameters makes OOP in commands more clunky for no good reason... Since I disagree with your "low usage" reasoning, this simpliest example can be fairly common in well-written applications:

  • abstract class X that configures an array parameter that should be required
  • X extending class Y that configures a regular parameter that should be required

Using arguments for both is logically impossible, unless you call the parent after the definition, which may make the command look messy/inconsistent with other commands (e.g. Z that defines 2 required parameters) and still doesn't work with 2 required array parameters. Using option for any requires additional checks mentioned above. I hope you will reconsider this. @xabbuh @javiereguiluz

@victortodoran
Copy link

One word comes to mind. Lazy

@lenar
Copy link
Contributor

lenar commented Feb 25, 2019

One phrase comes to mind: Rename "Options" to "Named Arguments"or something like that without the
ordering requirement of standard arguments. That way the semantics of word 'options' won't be showstopper. Much more useful.

@philraj
Copy link

philraj commented Feb 27, 2019

The fact that arguments can be optional makes it hard to believe the reasoning that "options" are for optional values and arguments are better for required values. "Named arguments" is a better term, like @lenar and others suggest, and it would be nice to have this built-in.

@c33s
Copy link

c33s commented Nov 18, 2022

doesn't make it the most sense to introduce a InputOption::REQUIRED? see #48244

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

Successfully merging a pull request may close this issue.