-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Console] Argument consumption #30244
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
Conversation
A new argument has been added in order to consume or not the argument.
The tests have been started Small fix on interface implementation.
The tests have been fixed. Same things for the ArgvInput.
The key is now used.
@@ -294,7 +294,7 @@ public function hasParameterOption($values, $onlyParams = false) | ||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function getParameterOption($values, $default = false, $onlyParams = false) | ||
public function getParameterOption($values, $default = false, $onlyParams = false, $consume = false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only this class should be changed: all the other changes should be reverted as they are unneeded BC breaks.
On this class, the argument should be commented, mentionned in the docblock and accessed using func_get_arg().
See eg https://github.com/symfony/symfony/pull/30212/files#diff-94f4ef2d14dbabcc4f9c798fea31defbR350
The consumed argument is now optional (BC promise).
The code style has been fixed, consumption to define. Small fix on the dataprovider during tests.
The dataprovider has been updated. The code should be finished.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, some comments to help move forward.
[['app/console', 'foo:bar', '-e', 'dev'], '-e', false, false, 'dev'], | ||
[['app/console', 'foo:bar', '-e', 'dev'], '-e', false, true, ''], | ||
[['app/console', 'foo:bar', '-e', 'dev', '-f', 'bar'], ['-e', '-f'], false, false, 'dev'], | ||
[['app/console', 'foo:bar', '-e', 'dev', '-f', 'bar'], ['-e', '-f'], false, true, ''], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when consume is true, the first call should return true, and the next ones should return false, because the first call "consumed" the info
[['app/console', 'foo:bar', '-e', 'dev', '--foo', 'bar'], '-f', false, false, ''], | ||
[['app/console', 'foo:bar', '-e', 'dev', '--foo', 'bar'], '-f', false, true, ''], | ||
[['app/console', 'foo:bar', '-e', 'dev', '--foo', 'bar'], '--foo', false, false, 'bar'], | ||
[['app/console', 'foo:bar', '-e', 'dev', '--foo', 'bar'], '-f', false, true, ''], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are move ways to set the dev env: --env=dev
and --env dev
- we should test them all, not only -e dev
please rebase also, there are unrelated commits in the current history. |
Closing as the current approach is not the correct one and this is stalling, I'll take over, thanks for trying! |
Hi everyone,
This PR is linked to #23343, as explained by @nicolas-grekas, a new argument has been added in order to remove the parameter if needed.
Tests should be completed (in progress), same things for the code (not every case are covered).