10000 Php documentation for methods "setOption()/setArgument()" doesn't allow arrays as values · Issue #28354 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Php documentation for methods "setOption()/setArgument()" doesn't allow arrays as values #28354

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
liarco opened this issue Sep 4, 2018 · 5 comments
Closed

Php documentation for methods "setOption()/setArgument()" doesn't allow arrays as values #28354

liarco opened this issue Sep 4, 2018 · 5 comments
Labels
Bug Console Status: Needs Review
Milestone
2.8

Comments

@liarco
Copy link
Contributor
liarco commented Sep 4, 2018

Hi, I'm working on a console application that deals with options registered as InputOption::VALUE_IS_ARRAY, but when I call $input->setOptions('my-option', ['value1', 'value2']) during initialization/validation my IDE complains about the second argument not matching the documentation (string|bool). The same applies to the setArgument($name, $value) method which is set to string only.

Since I might have array values for both options and arguments, should it be included into the documentation? Is it right to set array values for options/arguments in a this way?

Here is my proposal for the doc update: (I don't know if "array" is better than "string[]" here...)

    // File: symfony/console/Input/InputInterface.php

    // ...

    /**
     * Sets an argument value by name.
     *
     * @param string          $name  The argument name
     * @param string|string[] $value The argument value
     *
     * @throws InvalidArgumentException When argument given doesn't exist
     */
    public function setArgument($name, $value);

    // ...

    /**
     * Sets an option value by name.
     *
     * @param string               $name  The option name
     * @param string|string[]|bool $value The option value
     *
     * @throws InvalidArgumentException When option given doesn't exist
     */
    public function setOption($name, $value);

    // ...

Thank you for your time, please let me know if you want me to send a pull request for this.
@xabbuh xabbuh added the Console label Sep 4, 2018
@liarco liarco changed the title Php documentation for methods "setOption()/setArgument()" don't allow arrays as values Php documentation for methods "setOption()/setArgument()" doesn't allow arrays as values Sep 4, 2018
@chalasr
Copy link
Member
chalasr commented Sep 5, 2018

Actually both methods should accept a value of any type except objects not implementing __toString().
I would say that both should say mixed and mention the exception in the param's description.
Pull request welcomed anyways, this should be fixed on the 2.8 branch.

8000

@chalasr
Copy link
Member
chalasr commented Sep 5, 2018

Additionally mixed would be consistent with InputArgument::setDefault() and InputOption::setDefault().

@liarco liarco mentioned this issue Sep 5, 2018
Merged
@xabbuh
Copy link
Member
xabbuh commented Sep 6, 2018

@chalasr What would be the use case to support anything besides strings and arrays of strings? I'd say that using anything else is likely to break under certain circumstances.

@liarco
Copy link
Contributor Author
liarco commented Sep 6, 2018

@xabbuh imho that would be helpful in case you need to validate/build values with complex operations returning objects as results. Those results might have a meaningful default "string" representation which can be used as value. (e.g. imagine wrapping some strings with something like danielstjules/Stringy to perform some validation/transformation, then it would be possible to drop the resulting object without explicit casting)

Does it make sense?

@chalasr
Copy link
AD55
Member
chalasr commented Sep 7, 2018

@xabbuh I was thinking to the case where input values would be replaced after being casted/resolved e.g. by a console event listener (see also #19441).
Right now I don't see a place where such code could break, the __toString() method of core InputInterface implementations need to be able to cast the input values to strings for escaping, which would be fine with the proposed set of types.

If you still think this is more risky than useful, then we should probably fix the setDefault() docblock for InputArgument and InputOption to reflect the current InputInterface::setArgument and setOption docblocks.

@javiereguiluz javiereguiluz added this to the 2.8 milestone Sep 10, 2018
@fabpot fabpot closed this as completed Sep 10, 2018
fabpot added a commit that referenced this issue Sep 10, 2018
@fabpot
minor #28374 [Console] fixed PHPDoc for setArgument/setOption in Inpu…
1c759a1 
…tInterface (liarco)

This PR was squashed before being merged into the 2.8 branch (closes #28374).

Discussion
----------

[Console] fixed PHPDoc for setArgument/setOption in InputInterface

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #28354
| License       | MIT
| Doc PR        |

Methods now accept a value of any type except objects not implementing __toString().

**Example use case:** when using array arguments/options I can't set them programmatically without getting errors about type mismatch (from the IDE). With this patch it now works as expected.

Commits
-------

61529f3 [Console] fixed PHPDoc for setArgument/setOption in InputInterface
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
2.8
Development

No branches or pull requests
6 participants
@fabpot @javiereguiluz @liarco @xabbuh @chalasr @carsonbot
0