Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Console] Centralize input stream in base Input class#18999
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| */ | ||
| publicfunctionsetInteractive($interactive); | ||
| publicfunctiongetStream(); |
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.
To revert
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.
Thank's@ogizanagi, and thank's for having the idea of this.
ogizanagi commentedJun 8, 2016
Implementation looks good to me & conform to what we talked about 👍 |
| }elseif (function_exists('posix_isatty') &&$this->getHelperSet()->has('question')) { | ||
| $inputStream =$this->getHelperSet()->get('question')->getInputStream(); | ||
| }elseif (function_exists('posix_isatty')) { | ||
| $inputStream =$input->getStream(); |
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.
Missing$input instanceof StreamableInputInterface check.
ogizanagi commentedJun 8, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I think (but not sure about the policy for such cases) some legacy tests should be kept, but marked with the |
a61d607 to36f9bfcComparechalasr commentedJun 8, 2016
Re added old tests as legacy. |
| /** | ||
| * Returns the helper's input stream. | ||
| * | ||
| * @deprecated Will be removed in 4.0, use StreamableInputInterface::getStream() instead |
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.
Why did you remove the deprecation trigger ?
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.
Because we have a call to this getter in Application, so lots of tests have deprecations and don't pass anymore.
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.
That does not work like that. You MUST add the deprecation notice and fix the tests accordingly. But instead of doing that, I would recommend to just keepgetInputStream() and addsetInputStream().
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.
just keep getInputStream() and add setInputStream()
Does that mean I should only depreciatesetInputStream() and keepgetInputStream() as is?
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.
I was more talking about renaming the method in the interface to getInputStream and setInputStream, would that work?
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.
Not sure that we are talking about the same thing, my description is surely not clear.
I introduced a newStreamableInputInterface implemented bySymfony\Component\Console\Input\Input containing two methodsgetStream() andsetStream().
These methods would replace the oldQuestionHelper::getInputStream() andQuestionHelper::setInputStream(), that's why I depreciate them here.
The problem is thatApplication::configureIO gets the input stream fromQuestionHelper::getInputStream() before, so we need to keep it intact before totally remove it later. But, this call causes deprecations in all ApplicationTest tests that callApplication::run() because of the call of the deprecatedQuestionHelper::getInputStream().
346ac94 to80776d5Comparechalasr commentedJun 9, 2016
I moved the current QuestionHelperTest tests into a separated LegacyQuestionHelperTest class and made the changes (line notes), hope that clarifies what I mean. |
stof commentedJun 9, 2016
@chalasr would be better to avoid moving them, but only marking them as legacy, so that we have less conflicts when merging branches |
chalasr commentedJun 9, 2016
@stof Reverted. |
dfe0d7e to8248a77Comparefabpot commentedJun 14, 2016
|
fabpot commentedJun 14, 2016
In the description, you mention |
chalasr commentedJun 14, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@fabpot Yes the SymfonyQuestionHelper class is not changed, it would just become testable via the CommandTester as well as the QuestionHelper (seethis test in another PR based on this one). The changes are inherited from the parent About the As there is no reason to call What do you think about this? |
fabpot commentedJun 15, 2016
@chalasr Here is how you can make it work: add a |
Add StreamableInputInterfaceRemoved commented codePrevent BC by looking for QuestionHelper::Logic fixesCheck for that implements StreamableInputInterfaceRollback E_USER_DEPRECATED notice in getInputStreamRemove legacy tests to avoid KO because of deprecationsAdd missing useKeep old tests marked as legacyUndeprecate getInputStream, CS FixesMove legacy tests in separated classRevert separated legacy test classKeep legacy createInputInterfaceMock()Depreciate QuestionHelper::getInputStream()
chalasr commentedJun 15, 2016
The |
fabpot commentedJun 16, 2016
Thank you@chalasr. |
…(chalasr)This PR was merged into the 3.2-dev branch.Discussion----------[Console] Centralize input stream in base Input class| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets |#10844| License | MIT| Doc PR | not yetActually we have two classes that have an `inputStream` property with its getter+setter:the QuestionHelper ('question' helper) and the SymfonyQuestionHelper (used via SymfonyStyle, that inherits the inputStream from the QuestionHelper class).This situation makes command testing really hard.ATM we can't test a command that uses `SymfonyStyle::ask` without ugly hacks, and we can't find a generic way to set the inputs passed to a command from the CommandTester, as it need to be done on a specific helper that may not be the one used by the command (SymfonyStyle case).What I propose here is to add a `stream` property (and its getter+setter) to the abstract `Symfony\Component\Console\Input\Input` class.For now I just made the two helpers setting their `inputStream` from `Input::getStream`, as both QuestionHelper and SymfonyQuestionHelper classes have an `ask` method that takes the command Input as argument.In a next time (4.0), we could remove the `getInputStream` and `setInputStream` methods from the QuestionHelper class (this only deprecates them in favor of Input `setStream` and `getStream`).This would close PR#18902 (trying to make interactive command testing with SymfonyStyle easier).This would also make PR#18710 widely better by setting the input stream in a generic way (working for both helpers without caring about if they are used and which one is used).Please give me your opinions.Commits-------54d3d63 Centralize input stream in base Input class
| * @return resource | ||
| */ | ||
| publicfunctiongetInputStream() | ||
| publicfunctiongetInputStream($triggerDeprecationError =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.
adding a new argument here is a BC break
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.
see#19072
This PR was merged into the 3.2-dev branch.Discussion----------[Console] remove BC breaking argument| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#18999 (comment)| License | MIT| Doc PR |Commits-------f574330 remove BC breaking argument
…dTester (chalasr)This PR was merged into the 3.2-dev branch.Discussion----------[Console] Simplify simulation of user inputs in CommandTester| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR |symfony/symfony-docs#6623After@javiereguiluz pointed it in#17470, I open this PR to simplify the simulation of user inputs for testing a Command.It would be done by calling `CommandTester::setUserInputs()` with an array of inputs as argument, and so make the CommandTester creating an input stream from the inputs set by the developer, then call `QuestionHelper::setInputStream` and assign it to the helperSet of the command, sort as all is done automatically in one call.Depends on#18999Commits-------c7ba38a [Console] Set user inputs from CommandTester
This PR was merged into the 3.2-dev branch.Discussion----------[Console] Test SymfonyStyle::ask() output| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | ~| License | MIT| Doc PR | ~Now that we can test an interactive command that uses SymfonyStyle (after#18999), we should test their output.Commits-------e870758 [Console] Test SymfonyStyle::ask() output
nicolas-grekas commentedJun 29, 2016
@chalasr can you please check the remaining deprecated call on master and verify that all usage of the deprecated method have been removed from master (except when testing it for legacy reasons)? |
chalasr commentedJun 29, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@nicolas-grekas Just verified, there is no more calls to these methods in master, except for tests marked as legacy in |
This PR was merged into the 4.0-dev branch.Discussion----------[Console] Remove deprecated features| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | yes| Deprecations? | no| Tests pass? | yes| Fixed tickets | ~~#18999#19012~~| License | MIT| Doc PR | n/aCommits-------4cde3b9 [Console] Remove deprecated features
Uh oh!
There was an error while loading.Please reload this page.
Actually we have two classes that have an
inputStreamproperty with its getter+setter:the QuestionHelper ('question' helper) and the SymfonyQuestionHelper (used via SymfonyStyle, that inherits the inputStream from the QuestionHelper class).
This situation makes command testing really hard.
ATM we can't test a command that uses
SymfonyStyle::askwithout ugly hacks, and we can't find a generic way to set the inputs passed to a command from the CommandTester, as it need to be done on a specific helper that may not be the one used by the command (SymfonyStyle case).What I propose here is to add a
streamproperty (and its getter+setter) to the abstractSymfony\Component\Console\Input\Inputclass.For now I just made the two helpers setting their
inputStreamfromInput::getStream, as both QuestionHelper and SymfonyQuestionHelper classes have anaskmethod that takes the command Input as argument.In a next time (4.0), we could remove the
getInputStreamandsetInputStreammethods from the QuestionHelper class (this only deprecates them in favor of InputsetStreamandgetStream).This would close PR#18902 (trying to make interactive command testing with SymfonyStyle easier).
This would also make PR#18710 widely better by setting the input stream in a generic way (working for both helpers without caring about if they are used and which one is used).
Please give me your opinions.