8000 [Console] fixed PHPDoc for setArgument/setOption in InputInterface by liarco · Pull Request #28374 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Console] fixed PHPDoc for setArgument/setOption in InputInterface #28374

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

Merged
merged 1 commit into from
Sep 10, 2018
Merged

[Console] fixed PHPDoc for setArgument/setOption in InputInterface #28374

merged 1 commit into from
Sep 10, 2018

Conversation

liarco
Copy link
Contributor
@liarco liarco commented Sep 5, 2018
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.

@nicolas-grekas nicolas-grekas added this to the 2.8 milestone Sep 6, 2018
Copy link
Member
@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

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

@liarco thanks for reporting this issue ... and for solving it yourself. Also, congrats on your first Symfony contribution!

@fabpot
Copy link
Member
fabpot commented Sep 10, 2018

I think I'm against being that broad. Conceptually, an argument can only be string|string[] and an option string|string[]|bool. In any case, adding support for more is not a bug fix, so not for 2.8.

Note: it's not because something happens to work that we should make it official. That's one of these case IMHO

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

@fabpot thank you for your reply, I see your point (the original proposal was exactly that "strict"). There was a reason behind the inclusion of any object implementing __toString(), but I understand that it might be too broad and lead to misunderstandings and unpredictable behaviors. (I think that some explicit casts won't hurt)

May I change the doc as you suggested or should I wait for further investigation?

@fabpot
Copy link
Member
fabpot commented Sep 10, 2018

I think you can go ahead and make the changes.

@fabpot
Copy link
Member
fabpot commented Sep 10, 2018

Thank you @liarco.

@fabpot fabpot merged commit 61529f3 into symfony:2.8 Sep 10, 2018
fabpot added a commit that referenced this pull request Sep 10, 2018
…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
@liarco liarco deleted the ticket_28354 branch September 10, 2018 08:27
fabpot added a commit that referenced this pull request Sep 12, 2018
This PR was merged into the 2.8 branch.

Discussion
----------

[Console] Fix input values allowed types

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

Continuation of #28374

Commits
-------

0c16cd9 [Console] Fix input values allowed types
@curry684
Copy link
Contributor
curry684 commented Oct 12, 2018

@fabpot:

I think I'm against being that broad. Conceptually, an argument can only be string|string[] and an option string|string[]|bool. In any case, adding support for more is not a bug fix, so not for 2.8.

You forgot integers, as mentioned in #28448 (comment) even the documentation examples use those.

I understand the argument of type safety btw in why an int would be invalid as an option value, but in that case we do need to fix the docs, and accept that this is actually a weird B/C break in some way from the IDE or static analysis perspective (my builds started failing after upgrade from 4.1.4 to 4.1.6 because of this).

curry684 added a commit to curry684/symfony that referenced this pull request Oct 12, 2018
fabpot added a commit that referenced this pull request Oct 12, 2018
This PR was squashed before being merged into the 2.8 branch (closes #28834).

Discussion
----------

Allow integers as default console option value

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

B/C break fix for static analysis tools.

Ref #28374 (comment)

Commits
-------

064950a Allow integers as default console option value
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants
0