[Console] Centralize input stream in base Input class #18999
Conversation
| @@ -151,4 +151,6 @@ public function isInteractive(); | |||
| * @param bool $interactive If the input should be interactive | |||
| */ | |||
| public function setInteractive($interactive); | |||
|
|
|||
| public function getStream(); | |||
There was a problem hiding this comment.
Thank's @ogizanagi, and thank's for having the idea of this.
0feb08a to
f7f23b9
Compare
|
Implementation looks good to me & conform to what we talked about 👍 |
f7f23b9 to
f47684c
Compare
| } 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.
Missing $input instanceof StreamableInputInterface check.
f47684c to
a00bef0
Compare
|
I think (but not sure about the policy for such cases) some legacy tests should be kept, but marked with the |
a61d607 to
36f9bfc
Compare
|
Re added old tests as legacy. |
| @@ -84,6 +91,8 @@ public function setInputStream($stream) | |||
| /** | |||
| * Returns the helper's input stream. | |||
| * 8000 | |||
| * @deprecated Will be removed in 4.0, use StreamableInputInterface::getStream() instead | |||
There was a problem hiding this comment.
Why did you remove the deprecation trigger ?
There was a problem hiding this comment.
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.
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 keep getInputStream() and add setInputStream().
There was a problem hiding this comment.
just keep getInputStream() and add setInputStream()
Does that mean I should only depreciate setInputStream() and keep getInputStream() as is?
There was a problem hiding this comment.
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.
Not sure that we are talking about the same thing, my description is surely not clear.
I introduced a new StreamableInputInterface implemented by Symfony\Component\Console\Input\Input containing two methods getStream() and setStream().
These methods would replace the old QuestionHelper::getInputStream() and QuestionHelper::setInputStream(), that's why I depreciate them here.
The problem is that Application::configureIO gets the input stream from QuestionHelper::getInputStream() before, so we need to keep it intact before totally remove it later. But, this call causes deprecations in all ApplicationTest tests that call Application::run() because of the call of the deprecated QuestionHelper::getInputStream().
346ac94 to
80776d5
Compare
|
I moved the current QuestionHelperTest tests into a separated LegacyQuestionHelperTest class and made the changes (line notes), hope that clarifies what I mean. |
|
@chalasr would be better to avoid moving them, but only marking them as legacy, so that we have less conflicts when merging branches |
80776d5 to
362e03d
Compare
|
@stof Reverted. |
dfe0d7e to
8248a77
Compare
|
|
|
In the description, you mention |
|
@fabpot Yes the SymfonyQuestionHelper class is not changed, it would just become testable via the CommandTester as well as the QuestionHelper (see this 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? |
|
@chalasr Here is how you can make it work: add a |
Add StreamableInputInterface Removed commented code Prevent BC by looking for QuestionHelper:: Logic fixes Check for that implements StreamableInputInterface Rollback E_USER_DEPRECATED notice in getInputStream Remove legacy tests to avoid KO because of deprecations Add missing use Keep old tests marked as legacy Undeprecate getInputStream, CS Fixes Move legacy tests in separated class Revert separated legacy test class Keep legacy createInputInterfaceMock() Depreciate QuestionHelper::getInputStream()
8248a77 to
54d3d63
Compare
|
The |
|
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 yet Actually 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 | ||
| */ | ||
| public function getInputStream() | ||
| public function getInputStream($triggerDeprecationError = true) |
There was a problem hiding this comment.
adding a new argument here is a BC break
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#6623 After @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 #18999 Commits ------- 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
|
@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)? |
|
@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/a Commits ------- 4cde3b9 [Console] Remove deprecated features
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.