8000 [Console] ArgvInput::hasParameterOption confused by short options with values · Issue #25825 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Console] ArgvInput::hasParameterOption confused by short options with values #25825

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.

Sign up for GitHub

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

Jump to bottom
Closed
greg-1-anderson opened this issue Jan 18, 2018 · 11 comments
Closed

[Console] ArgvInput::hasParameterOption confused by short options with values #25825

greg-1-anderson opened this issue Jan 18, 2018 · 11 comments
Labels
Bug Console Status: Needs Review

Comments

@greg-1-anderson
Copy link
Contributor
Q A
Bug report? yes
Feature request? no
BC Break report? yes
RFC? yes
Symfony version 3.4.2 (and others)

#24987 introduced a bug in hasParameterOption. If a short option has a value, then hasParameterOption will search the value for additional short options.

#25487 submitted a fix for this bug, but the fix appears to be incorrect.

The current implementation of parseShortOption is as follows:

    /**
     * Parses a short option.
     *
     * @param string $token The current token
     */
    private function parseShortOption($token)
    {
        $name = substr($token, 1);

        if (strlen($name) > 1) {
            if ($this->definition->hasShortcut($name[0]) && $this->definition->getOptionForShortcut($name[0])->acceptValue()) {
                // an option with a value (with no space)
                $this->addShortOption($name[0], substr($name, 1));
            } else {
                $this->parseShortOptionSet($name);
            }
        } else {
            $this->addShortOption($name, null);
        }
    }

In particular, note the substr($name, 1). If there is a short option -e that takes a value, then -etest will provide the value test to the -e option. See also the implementation of parseShortOptionSet, which has similar logic. No = is assumed or accounted for.

#25487 fixed the bug of #24987 by assuming that the value of a short option was formatted as -e=test; however, this is inconsistent with the implementation of the parsing functions.

My testing seems to uphold these observations, but please LMK if I have made an error somewhere. I'm not sure if there is a better solution than reverting #24987.
@greg-1-anderson
Copy link
Contributor Author

@Simperfit your analysis here would be appreciated.

@Simperfit
Copy link
Contributor
Simperfit commented Jan 18, 2018 •
edited
Loading

Why should we revert #24987, it does fix a bug.

We have to implement a method that will respect the consistency of both method and fix the bug by checking this format too.

Do you want to try to fix this, I'm going to help you as I can ;).

@greg-1-anderson
Copy link
Contributor Author

@Simperfit The hasParameterOption method does not have access to $this->definition, as it is called before the argument and option definitions are processed and added to this record. Therefore, it is not possible for it to differentiate between short options that have values, and short options that do not have values.

In #25487, an attempt was made to fix a bug where someone using -etest (setting the value of the short option -e to test) would cause hasParameterOption to think that -s was also set. The fix of checking for an = was incorrect, because short options with values do not use an = character to separate the value from the short option.

We cannot alter the implementation of hasParameterOption to require the =, because that would be a B/C break. I therefore think that we are stuck, and cannot fix hasParameterOption. By my analysis, we must live with the historic limitation that short options such as -h, which are tested via hasParameterOption, cannot be grouped by the user with other short options. We could potentially detect the -h in -hq (test the first short option only), but we cannot detect the -h in -qh, because hasParameterOption does not know whether -q (or any other short option that comes before it) does or does not take a value.

I'd be happy to make a PR to revert #24987, or to make a PR to test just the first short option (instead of the old behavior of requiring short options appear by themselves when tested with hasParameterOption), but I do not see a way to fix this fully due to the limitations imposed on this method.

@greg-1-anderson greg-1-anderson mentioned this issue Jan 18, 2018
Closed
@greg-1-anderson
Copy link
Contributor Author

I have another thought on how we might proceed here:

  1. Revert [Console] Fix global console flag when used in chain #24987
  2. Mark hasParameterOption / getParameterOption deprecated, and get folks to stop using these methods.
  3. Postpone handling of the help (-h / --help) and version (-V / --version) options until Application::doRunCommand, after $input->bind($command->getDefinition());. Then, $input->getOption() may be used with correct result.

This will incur a slight performance penalty when using --help and --version, but this is no more impact than is already experienced by all other commands. The current performance optimization is, in my mind, not as important as correct operation. These commands may still be handled prior to the creation of the ConsoleCommandEvent, so the behavior of console applications will not change vis-a-vis event handling. There is some minor potential for behavior change for console applications that override the Application class, as the help and version commands will be passed to some methods (e.g. doRunCommand) they previously were not. The impact of this would probably be fine.

@fabpot You merged #25487, which I believe contained a logic error as explained above. Could you please review here and let me know if I have made any mistake in my analysis. I'd be happy to roll a PR to fix this once the desired remediation technique is agreed upon.

@Simperfit
Copy link
Contributor

@greg-1-anderson I've tested this behaviour on the whole supported version of symfony and I can't see where the -etest is working ?

According to my test it does nothing, it's not taken in account.

#( 01/19/18@11:31AM )( hamza@MBP-de-Amrouche ):~/projet/contrib/test34
   bin/console c:c -etest
 // Clearing the cache for the dev environment with debug true                                                                                                                                         
 [OK] Cache for the "dev" environment (debug=true) was successfully cleared.

For the linked issue about Drush, could you please tell me what commands does it run with the -D options ?

@chalasr
Copy link
Member
chalasr commented Jan 19, 2018

@Simperfit It seems like the Input::getParameterOption() behavior is inconsistent before your PRs, and I guess your PRs probably highlight that by breaking a working case.

We do support passing values to short options with no separator.
Example on 2.7:

<?php

namespace AppBundle\Command;

use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Input\InputOption;
use Symfony\Component\Console\Output\OutputInterface;

class FooCommand extends Command
{
    public function configure()
    {
        $this->setName('foo');
        $this->addOption('opt', 'o', InputOption::VALUE_OPTIONAL);
    }

    public function execute(InputInterface $input, OutputInterface $output)
    {
        print $input->getOption('opt')."\n";
        print $input->getOption('env')."\n";
        print $input->getParameterOption(array('--env', '-e'));
    }
}

Output:

php app/console foo -oval -etest
val
test

Note that there is no output for the third print, that's the bug which exists prior to your changes, the app/console script uses the very same line to set the kernel environment and use dev as fallback value, hence you always end up in dev.

Short option with value and no separator is tested here https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Console/Tests/Input/ArgvInputTest.php#L112 and is still green on all maintained branches.
So @greg-1-anderson your analysis is much appreciated, but could you please give us some code that precisely reproduce the bug you are facing (a command, how you run it, the expected and the actual result).

We need to fix the getParameterOption() bug though, but it's apparently unrelated to this issue.

@greg-1-anderson
Copy link
Contributor Author

@chalasr Thank you for the feedback. The existing unit test you reference above is in fact passing; it happens to not hit one of the non-working edge cases. I can provide a PR with a failing unit test and a failing integration test to illustrate the problem here.

@greg-1-anderson greg-1-anderson mentioned this issue Jan 20, 2018
Closed
greg-1-anderson added a commit to greg-1-anderson/symfony that referenced this issue Jan 20, 2018
@greg-1-anderson
WIP symfony#25825: Add a failing test to demonstrate bug introduced in #
01ed485 
symfony#24987.
@greg-1-anderson
Copy link
Contributor Author

@chalasr Please see the failing tests in #25852; hopefully this will clarify how the API was intended to behave vs. its actual behavior before and after #24987

@Simperfit Drush / Robo / Phing support defining configuration items on the commandline. For example, the following config file:

foo:
  bar: baz

Could also be represented on the command line:

drush -Dfoo.bar=baz status

@nicolas-grekas nicolas-grekas mentioned this issue Jan 21, 2018
Merged
greg-1-anderson added a commit to greg-1-anderson/symfony that referenced this issue Jan 22, 2018
@greg-1-anderson
Fixes symfony#25825: Back out symfony#24987 and fix edge case in getP…
542cf6d 
…arameterOption.
@greg-1-anderson greg-1-anderson mentioned this issue Jan 22, 2018
Merged
@greg-1-anderson
Copy link
Contributor Author

Proposed fix in #25893

@jakzal jakzal mentioned this issue Jan 26, 2018
Closed
@jakzal
Copy link
Contributor
jakzal commented Jan 26, 2018

Just noticed Behat tests are failing because of this issue (see here).

The following command started displaying a help message, instead of running the test suite:

behat --no-colors -sbig_brother -fpretty --format-settings='{\"paths\": true}' features

The cause of the issue is #24987 and reverting it would solve the problem. -sbig_brother contains the "h" letter which is treated as -h and the Application assumes it needs to display a help message:

symfony/src/Symfony/Component/Console/Application.php

Lines 164 to 172 in 7b4d519

$name = $this->getCommandName($input);
if (true === $input->hasParameterOption(array('--help', '-h'))) {
if (!$name) {
$name = 'help';
$input = new ArrayInput(array('command' => 'help'));
} else {
$this->wantHelps = true;
}
}

@jakzal jakzal mentioned this issue Jan 26, 2018
Merged
@greg-1-anderson
Copy link
Contributor Author

I think #25893 is ready to go; hopefully we can get that into some stable Symfony releases shortly.

greg-1-anderson added a commit to greg-1-anderson/symfony that referenced this issue Jan 29, 2018
@greg-1-anderson
Follow-on to symfony#25825: Fix edge case in getParameterOption.
35f98e2 
Add one test to demonstrate getParameterOption fix. Add several tests to demonstrate current limitations in hasParameterOption.
fabpot added a commit that referenced this issue Feb 7, 2018
@fabpot
bug #25893 [Console] Fix hasParameterOption / getParameterOption when…
f7d9701 
… used with multiple flags (greg-1-anderson)

This PR was merged into the 2.7 branch.

Discussion
----------

[Console] Fix hasParameterOption / getParameterOption when used with multiple flags

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no (Fixes BC break in #24987)
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #25825
| License       | MIT
| Doc PR        | n/a

Proposed resolution to #25825:
- Back out #24987
- Fix getParameterOption for short options with values, e.g. `-edev`

Commits
-------

35f98e2 Follow-on to #25825: Fix edge case in getParameterOption.
@fabpot fabpot closed this as completed Feb 7, 2018
xabbuh added a commit that referenced this issue Feb 9, 2018
@xabbuh
Merge branch '2.7' into 2.8
641a46b 
* 2.7:
  [SecurityBundle] Backport test
  Fix misspelling variable
  [DI] minor: use a strict comparision in setDecoratedService
  Follow-on to #25825: Fix edge case in getParameterOption.
  keep the context when validating forms
xabbuh added a commit that referenced this issue Feb 9, 2018
@xabbuh
Merge branch '2.8' into 3.4
05a0452 
* 2.8:
  [SecurityBundle] Backport test
  [Security] fix merge of 2.7 into 2.8 + add test case
  backport regression test from 3.4
  Fix misspelling variable
  [DI] minor: use a strict comparision in setDecoratedService
  Follow-on to #25825: Fix edge case in getParameterOption.
  keep the context when validating forms
BAD4
xabbuh added a commit that referenced this issue Feb 9, 2018
@xabbuh
Merge branch '3.4' into 4.0
064acb6 
* 3.4:
  Env var maps to undefined constant.
  [SecurityBundle] Backport test
  [Security] fix merge of 2.7 into 2.8 + add test case
  backport regression test from 3.4
  do not mock the container builder or definitions
  fixed CS
  [TwigBundle] Register TwigBridge extensions first
  [WebProfilerBundle] Fix sub request link
  PhpDocExtractor::getTypes() throws fatal error when type omitted
  Fix misspelling variable
  use libsodium to run Argon2i related tests
  [DI] minor: use a strict comparision in setDecoratedService
  [HttpKernel] fix FC
  Follow-on to #25825: Fix edge case in getParameterOption.
  keep the context when validating forms
xabbuh added a commit that referenced this issue Feb 9, 2018
@xabbuh
Merge branch '4.0'
347438e 
* 4.0:
  fix merge
  Env var maps to undefined constant.
  [SecurityBundle] Backport test
  [Security] fix merge of 2.7 into 2.8 + add test case
  backport regression test from 3.4
  do not mock the container builder or definitions
  fixed CS
  [TwigBundle] Register TwigBridge extensions first
  [WebProfilerBundle] Fix sub request link
  PhpDocExtractor::getTypes() throws fatal error when type omitted
  Fix misspelling variable
  use libsodium to run Argon2i related tests
  [DI] minor: use a strict comparision in setDecoratedService
  [HttpKernel] fix FC
  Follow-on to #25825: Fix edge case in getParameterOption.
  keep the context when validating forms
@greg-1-anderson greg-1-anderson mentioned this issue Feb 12, 2018
Closed
ostrolucky pushed a commit to ostrolucky/symfony that referenced this issue Mar 25, 2018
@xabbuh
Merge branch '3.4' into 4.0
cb720f3 
* 3.4:
  Env var maps to undefined constant.
  [SecurityBundle] Backport test
  [Security] fix merge of 2.7 into 2.8 + add test case
  backport regression test from 3.4
  do not mock the container builder or definitions
  fixed CS
  [TwigBundle] Register TwigBridge extensions first
  [WebProfilerBundle] Fix sub request link
  PhpDocExtractor::getTypes() throws fatal error when type omitted
  Fix misspelling variable
  use libsodium to run Argon2i related tests
  [DI] minor: use a strict comparision in setDecoratedService
  [HttpKernel] fix FC
  Follow-on to symfony#25825: Fix edge case in getParameterOption.
  keep the context when validating forms
ostrolucky pushed a commit to ostrolucky/symfony that referenced this issue Mar 25, 2018
@xabbuh
Merge branch '4.0'
dec1c1d 
* 4.0:
  fix merge
  Env var maps to undefined constant.
  [SecurityBundle] Backport test
  [Security] fix merge of 2.7 into 2.8 + add test case
  backport regression test from 3.4
  do not mock the container builder or definitions
  fixed CS
  [TwigBundle] Register TwigBridge extensions first
  [WebProfilerBundle] Fix sub request link
  PhpDocExtractor::getTypes() throws fatal error when type omitted
  Fix misspelling variable
  use libsodium to run Argon2i related tests
  [DI] minor: use a strict comparision in setDecoratedService
  [HttpKernel] fix FC
  Follow-on to symfony#25825: Fix edge case in getParameterOption.
  keep the context when validating forms
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Console Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@fabpot @jakzal @greg-1-anderson @xabbuh @Simperfit @chalasr @carsonbot
0