Added getter and setter of QuestionHelper to SymfonyStyle #17136

SCIF · 2015-12-25T15:48:40Z

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

Current SymfonyStyle implementation disallow to test interactively input of command according docs.

SCIF · 2015-12-25T16:42:20Z

I did ./phpunit src/Symfony/Component/Console/ according contribute docs and it said «Ok». Also, process fail seems odd and not related to my changes.

Tobion · 2015-12-25T21:32:22Z

src/Symfony/Component/Console/Style/SymfonyStyle.php

@@ -356,6 +357,22 @@ public function write($messages, $newline = false, $type = self::OUTPUT_NORMAL)
    }

    /**
+     * @return QuestionHelper


could also be null

Tobion · 2015-12-25T21:34:34Z

What do you need the getter for?

ogizanagi · 2015-12-26T12:27:32Z

IMO, this should be merged in 2.7 and should add SymfonyStyle tests for the questions rendering, as done for other methods in SymfonyStyleTest.

As @Tobion said, I also think the getter won't be useful.

However, even with this PR, commands using the SymfonyStyle helper cannot be easily tested interactively, because on the contrary of the QuestionHelper, SymfonyQuestionHelper isn't registered in the helper set. So it'll require a setInputStream method or whatever in the command class. :/

SCIF · 2016-01-14T15:26:23Z

@Tobion, here is one example of using getter. Getter required to allow passthrough input stream though different helpers if you want to replace ones.

@ogizanagi

SymfonyQuestionHelper isn't registered in the helper set

It used by laravel framework for example (btw, as main helper, not in helperset). Symfony is component kit and should provide flexible features as i think.

SCIF · 2016-01-14T16:14:51Z

Fixed phpdoc. Thanks!

xabbuh · 2016-01-14T19:21:06Z

@SCIF The code you linked to doesn't make use of such a getter and I don't see why that would be necessary (the console helper set is something different).

SCIF · 2016-01-15T01:14:38Z

@xabbuh , that example shown that input stream sometimes required. If i want to replace QuestionHelper via setter — i would want to passtrough input stream hinted to original version of QuestionHelper to my own. It's not only testing purposes. Testing can be done without setter but substitution of QuestionHelper by different implementation can't be done without getter (to take user's input stream from it) and setter (to set new helper with hinted user's input stream).

localheinz · 2016-01-22T11:56:31Z

@SCIF

Do you want to have a look at #17468? @xabbuh was so friendly to point out that the referenced PR is a duplicate of this one here, do you have an opinion (would you like to follow up with tests here, shall I close mine)?

xabbuh · 2016-01-22T12:09:17Z

@SCIF Imho then it makes more sense to let the SymfonyStyle class return the input stream directly (via some method like getInputStream()).

SCIF · 2016-01-22T13:10:49Z

@xabbuh , i really don't know why you think that getter of QuestionHelper is bad idea. But i can replace getter of Helper with InputStream getter if you insist. My arguments are above, you decide please something and it should be closed.

@localheinz , thank you for your tests and work! Let i cherry pick commit with your tests and #17468 can be closed, isn't it?

localheinz · 2016-01-22T16:50:31Z

@SCIF

👍 If you like, go ahead!

marek-pietrzak-tg · 2016-01-26T16:54:53Z

src/Symfony/Component/Console/Style/SymfonyStyle.php

@@ -356,6 +357,22 @@ public function write($messages, $newline = false, $type = self::OUTPUT_NORMAL)
    }

    /**
+     * @return QuestionHelper|null


I personally dislike the idea of creating a getter which can return either an object or null. It will introduce additional complexity above - the need of checking if the given result is an instance of QuestionHelper

Maybe just create a SymfonyQuestionHelper in __construct(),
so getQuestionHelp() can always return an instance of QuestionHelper

for example:

diff --git a/Style/SymfonyStyle.php b/Style/SymfonyStyle.php index 47d7d12..3402918 100644 --- a/Style/SymfonyStyle.php +++ b/Style/SymfonyStyle.php @@ -47,6 +47,7 @@ class SymfonyStyle extends OutputStyle public function __construct(InputInterface $input, OutputInterface $output) { $this->input = $input; + $this->questionHelper = new SymfonyQuestionHelper(); $this->bufferedOutput = new BufferedOutput($output->getVerbosity(), false, clone $output->getFormatter()); // Windows cmd wraps lines as soon as the terminal width is reached, whether there are following chars or not. $this->lineLength = min($this->getTerminalWidth() - (int) (DIRECTORY_SEPARATOR === '\\'), self::MAX_LINE_LENGTH); @@ -323,10 +324,6 @@ class SymfonyStyle extends OutputStyle $this->autoPrependBlock(); } - if (!$this->questionHelper) { - $this->questionHelper = new SymfonyQuestionHelper(); - } - $answer = $this->questionHelper->ask($this->input, $this, $question); if ($this->input->isInteractive()) { @@ -356,6 +353,14 @@ class SymfonyStyle extends OutputStyle } /** + * @return QuestionHelper + */ + public function getQuestionHelper() + { + return $this->questionHelper; + } + + /** * {@inheritdoc} */ public function newLine($count = 1)

I don't feel like creating a SymfonyQuestionHelper object in the constructor is a good idea. There are many use cases where it's not used at all. Still, getQuestionHelper use case is very unclear for me, could you explain your point of view a little bit more please?

i want to test the action of interactively input:

creating a SymfonyQuestionHelper object in the constructor for making code simpler.

use getQuestionHelper() to get the SymfonyQuestionHelper object for test interactively input with mock via setInputStream()

SCIF · 2016-03-03T12:12:13Z

@javiereguiluz , which style getter do you want to be added by me? I asked that but didn't get any answer. Just don't want to push fixes for getQuestionHelper() and get decline of it by some of symfony members. I want to arrive at any decision and implement it.

sstok · 2016-05-12T11:28:28Z

Is there any chance of getting this merged? I'm currently using a hack to make my tests work, but I want to use a custom class to display a help text below the label and remove the "required" validator.

fabpot · 2016-06-15T08:40:16Z

Closing in favor of #18710 and #18999

carsonbot added the Status: Needs Review label Dec 25, 2015

Tobion reviewed Dec 25, 2015
View reviewed changes

Added getter and setter of QuestionHelper to SymfonyStyle

e5087fe

SCIF force-pushed the symfonystyle-patch branch from 9057999 to e5087fe Compare January 14, 2016 16:14

xabbuh added the Console label Jan 21, 2016

xabbuh mentioned this pull request Jan 21, 2016

[Console] Fix: Allow to inject QuestionHelper into SymfonyStyle #17468

Closed

2 tasks

marek-pietrzak-tg reviewed Jan 26, 2016
View reviewed changes

javiereguiluz added the Bug label Mar 3, 2016

This was referenced May 26, 2016

[Console] Simplify simulation of user inputs in CommandTester #18710

Merged

[Console] Add SymfonyStyle::setInputStream() to ease command testing #18902

Closed

fabpot closed this Jun 15, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added getter and setter of QuestionHelper to SymfonyStyle #17136

Added getter and setter of QuestionHelper to SymfonyStyle #17136

SCIF commented Dec 25, 2015

SCIF commented Dec 25, 2015

Tobion Dec 25, 2015

Tobion commented Dec 25, 2015

ogizanagi commented Dec 26, 2015

SCIF commented Jan 14, 2016

SCIF commented Jan 14, 2016

xabbuh commented Jan 14, 2016

SCIF commented Jan 15, 2016

localheinz commented Jan 22, 2016

xabbuh commented Jan 22, 2016

SCIF commented Jan 22, 2016

localheinz commented Jan 22, 2016

marek-pietrzak-tg Jan 26, 2016

edentsai Feb 6, 2016

marek-pietrzak-tg Feb 11, 2016

edentsai Feb 15, 2016

SCIF commented Mar 3, 2016

sstok commented May 12, 2016 •

edited

Loading

fabpot commented Jun 15, 2016

Added getter and setter of QuestionHelper to SymfonyStyle #17136

Added getter and setter of QuestionHelper to SymfonyStyle #17136

Conversation

SCIF commented Dec 25, 2015

SCIF commented Dec 25, 2015

Tobion Dec 25, 2015

Choose a reason for hiding this comment

Tobion commented Dec 25, 2015

ogizanagi commented Dec 26, 2015

SCIF commented Jan 14, 2016

SCIF commented Jan 14, 2016

xabbuh commented Jan 14, 2016

SCIF commented Jan 15, 2016

localheinz commented Jan 22, 2016

xabbuh commented Jan 22, 2016

SCIF commented Jan 22, 2016

localheinz commented Jan 22, 2016

marek-pietrzak-tg Jan 26, 2016

Choose a reason for hiding this comment

edentsai Feb 6, 2016

Choose a reason for hiding this comment

marek-pietrzak-tg Feb 11, 2016

Choose a reason for hiding this comment

edentsai Feb 15, 2016

Choose a reason for hiding this comment

SCIF commented Mar 3, 2016

sstok commented May 12, 2016 • edited Loading

fabpot commented Jun 15, 2016

sstok commented May 12, 2016 •

edited

Loading