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] Fix input validation when required arguments are missing#15533

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
fabpot merged 2 commits intosymfony:2.3fromjakzal:console/argument-validation-fix
Sep 27, 2015

Conversation

@jakzal
Copy link
Contributor

QA
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#15505
LicenseMIT
Doc PR-

The rule that was here in place previously only works when arguments are passed from command line, as in command line there is no way of skipping an argument. The rule does not work for arguments set on the Input after a command is run.

@jakzal
Copy link
ContributorAuthor

If I used a foreach loop instead of array_filter, I could improve the exception message and say 'Not enough arguments. "name" is missing.'. What do you think?

@jakzal
Copy link
ContributorAuthor

This broke tests in FrameworkBundle. Looking into this.

@nochso
Copy link

I had a look at the tests for HelpCommand:

1) Symfony\Component\Console\Tests\Command\HelpCommandTest::testExecuteForCommandAliasRuntimeException: Not enough arguments.2) Symfony\Component\Console\Tests\Command\HelpCommandTest::testExecuteForApplicationCommandRuntimeException: Not enough arguments.3) Symfony\Component\Console\Tests\Command\HelpCommandTest::testExecuteForApplicationCommandWithXmlOptionRuntimeException: Not enough arguments.

testExecuteForCommandAlias specifically:

What is passed as arguments:
array('command_name' => 'li')

HelpCommand has no required arguments:
new InputArgument('command_name', InputArgument::OPTIONAL, 'The command name', 'help'),

getArgumentRequiredCount() returns 1

However, thecommand => 'help' is missing which is the argument that's actually required. That's the default required argument forApplication though, right?

@marek-pietrzak-tg
Copy link
Contributor

I had a quick look at these failing tests as well.command As you said@nochso adding$commandTester->execute(array('command' => 'help', 'command_name' => 'li')); solves the problem with failing tests for the help command test. Normally it's been added here:https://github.com/mheki/symfony/blob/2.3/src/Symfony/Component/Console/Application.php#L176-176 when you pass either--help or-h
As this PR checks also required arguments added during runtime, more difficult to fix is test for router match command as it usesrouter:debug internally and in order to make that test pass, thecommand argument should be injected here:https://github.com/mheki/symfony/blob/2.3/src/Symfony/Bundle/FrameworkBundle/Command/RouterMatchCommand.php#L83-83

Copy link
Contributor

Choose a reason for hiding this comment

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

if (count($requiredArguments) < $definition->getArgumentRequiredCount()) { would be shorter

@fabpot
Copy link
Member

@jakzal Any news on this one? And to answer your question, I would indeed give more information in the error message if possible.

Previous rule was only working when arguments are passed from command line, as in command line there is no way of skipping an argument. The rule does not work for arguments set on the Input after a command is run.
@jakzaljakzalforce-pushed theconsole/argument-validation-fix branch from30cbe80 tof12a4c1CompareSeptember 14, 2015 09:33
@jakzal
Copy link
ContributorAuthor

I added a better exception message:Not enough arguments (missing: "name").

Solving the problem with failing command tests is a bit more complex.

Thecommand is a required argument added from the application's definition. It's usually omitted when command is tested with CommandTester or if the command is called directly (like in RouterMatchCommand indicated by @mheki).

The previous version ofInput::validate() worked becase it was only comparing number of arguments and there was usually at least one argument given.

To fix this we need to either:

  • require the user to always pass thecommand argument

  • add the command in its run() method:

    if ($input->hasArgument('command') &&null ===$input->getArgument('command')) {$input->setArgument('command',$this->getName());    }

First solution would break lots of existing command tests, and the second doesn't seem like the right place (as the command argument is added by the application).

@jakzal
Copy link
ContributorAuthor

I went with option 2. Happy to receive feedback. Ping @symfony/deciders.

Copy link
Member

Choose a reason for hiding this comment

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

Can you explain when this happens? I think it deserves a comment.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Great suggestion. I'll add:

// The command name argument is often omitted when a command is executed directly with its run() method.// It will fail the validation since the command argument is required by the application.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Writing this comment gave me another idea: is the command argument really a required argument? What if we made it optional instead of setting it here?

@jakzaljakzalforce-pushed theconsole/argument-validation-fix branch 2 times, most recently fromf2145bd to45c1cc6CompareSeptember 16, 2015 08:02
@jakzaljakzalforce-pushed theconsole/argument-validation-fix branch from45c1cc6 to4982b02CompareSeptember 16, 2015 08:05
@fabpot
Copy link
Member

👍

1 similar comment
@weaverryan
Copy link
Member

👍

@xabbuh
Copy link
Member

@jakzal Usually, when using theCommandTester class, thecommand argument should be added automatically when missing:https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/Console/Tester/CommandTester.php#L60-65 Can't we just fix those lines to get rid of adding the argument in theConsole class itself?

@jakzal
Copy link
ContributorAuthor

@xabbuh that's not gonna fix the case of calling commands directly, like here:https://github.com/symfony/symfony/blob/2.3/src/Symfony/Bundle/FrameworkBundle/Command/RouterMatchCommand.php#L83-83

@xabbuh
Copy link
Member

Hm, indeed. 👍

Status: Reviewed

@fabpot
Copy link
Member

Thank you@jakzal.

@fabpotfabpot merged commit4982b02 intosymfony:2.3Sep 27, 2015
fabpot added a commit that referenced this pull requestSep 27, 2015
… missing (jakzal)This PR was merged into the 2.3 branch.Discussion----------[Console] Fix input validation when required arguments are missing| Q             | A| ------------- | ---| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#15505| License       | MIT| Doc PR        | -The rule that was here in place previously only works when arguments are passed from command line, as in command line there is no way of skipping an argument. The rule does not work for arguments set on the Input after a command is run.Commits-------4982b02 [Console] Add the command name to input arguments if it's missingf12a4c1 [Console] Fix input validation when required arguments are missing
@jakzaljakzal deleted the console/argument-validation-fix branchSeptember 27, 2015 13:38
This was referencedOct 27, 2015
@fabpotfabpot mentioned this pull requestOct 27, 2015
xabbuh added a commit to xabbuh/PandaBundle that referenced this pull requestDec 7, 2015
This makes test pass again given the improvements to the message of theexception that is thrown sincesymfony/symfony#15533 when a requiredargument is missing.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@jakzal@nochso@marek-pietrzak-tg@fabpot@weaverryan@xabbuh@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp