Closed mortenscheel closed 8 months ago
I don't think we should have a method for this. The best practice should be the default.
Waiting for review from @jessarcher
I don't think we should have a method for this. The best practice should be the default.
Agreed. Based on the linked post, I think I can agree that using STDERR
makes sense for interactive prompts. I'm less certain about where output from functions like info
should go. Am I right in assuming that Symfony still uses STDOUT
for their equivalent methods, and by extension, so would Laravel's wrappers?
Any changes we make would need to consider the newline behaviour. We output a single blank line between prompts and other informational messages so we need to track the number of trailing newlines previously output so we know how many (if any) need to be output before the next prompt. If output is mixed between STDOUT
and STDERR
, we'd still need to track the newlines globally between them, but I could see this causing some layout problems if STDOUT
was being redirected somewhere else.
Am I right in assuming that Symfony still uses STDOUT for their equivalent methods, and by extension, so would Laravel's wrappers?
Symfony commands don't have convenience methods like info()
or error()
. You just receive an OutputInterface
like this
protected function execute(InputInterface $input, OutputInterface $output): int
{
$output->writeln('<info>This goes to STDOUT</info>');
// If you want to output to STDERR, you have to do it manually
$err = $output instanceof ConsoleOutputInterface ? $output->getErrorOutput() : $output;
$err->writeln('<error>This goes to STDERR</error>');
return self::SUCCESS;
}
Good point about info()
. I personally think that prompt functions that can be considered "output formatter helpers" (info, note, error, table, etc.) should still use STDOUT, because they're used deliberately to produce output. Those that only produce temporary output (spin and table), as well as those that involve user interaction, should use STDERR. You could of course argue that error()
messages should go to STDERR, but Artisan uses STDOUT, so it's probably best to stick with that.
If output is mixed between STDOUT and STDERR, we'd still need to track the newlines globally between them, but I could see this causing some layout problems if STDOUT was being redirected somewhere else.
We should be able to detect if STDOUT is being redirected by using this technique.
Good point about
info()
. I personally think that prompt functions that can be considered "output formatter helpers" (info, note, error, table, etc.) should still use STDOUT, because they're used deliberately to produce output. Those that only produce temporary output (spin and table), as well as those that involve user interaction, should use STDERR. You could of course argue thaterror()
messages should go to STDERR, but Artisan uses STDOUT, so it's probably best to stick with that.
Makes sense, although it could be a pain if you want the command to output well-formed JSON that can be redirected to STDOUT, but still want to show some informational messages that don't get redirected.
We should be able to detect if STDOUT is being redirected by using this technique.
Currently, we only check whether STDIN is a TTY (or whether the --non-interactive
option was passed to a Laravel command) to determine whether to prompt interactively.
What do you think about the following additional checks?
That should make things "just work" as expected for most scenarios.
- If STDOUT is a TTY, then continue as usual.
- Otherwise, if STDERR is a TTY, use that for interactive output (or perhaps all output).
- If neither is a TTY, then disable prompt interactivity.
Excellent solution 👍 I think I'll have time to implement it this week.
Looking forward to hearing your feedback. Here's an easy way to test it:
php playground/index.php > out.txt
The prompt should work as always, and out.txt should only contain the final var_dump() with the selected values.
Hey @mortenscheel, that seems to work really well!
The only problem is that when Prompts is used with Laravel, we configure it to use Laravel's OutputStyle
instance, instead of a ConsoleOutput
instance from this repo. The ConsoleOutput
class is only used when Prompts is used outside of Laravel.
This is so we can keep track of newlines output from both Prompts and Laravel to ensure the correct spacing between output from each source.
Rather than duplicate the logic in Laravel, can we move it within the Prompt
class so it works regardless of the OutputInterface
being used? Presumably, it will only be able to use STDERR if the output is an instance of ConsoleOutputInterface
as it defines the getErrorOutput
method.
Rather than duplicate the logic in Laravel, can we move it within the Prompt class so it works regardless of the OutputInterface being used?
Sure. @jessarcher just to make sure I understand correctly, you're suggesting that the different prompts should call static::write()
(where the counting happens before sending the message to an OutputInterface
) in stead of static::output()->write()
?
@jessarcher please take a look at the implementation.
I've refactored the newline capturing to a new PromptWriter
class, which handles the capturing regardless of which OutputInterface
is being used.
I was a bit confused by the fact that Prompt
sometimes rendered without "synchronizing" the number of captured newlines from ConsoleOutput
. So when I tried using a single newLinesWritten
for everything, the prompt started jumping around quite a bit. I just wanted to mention it, in case it isn't intentional. Let me know if you want an example.
Hi @mortenscheel,
I've just tested this out, and there's an issue with the newlines between Prompt output and Laravel output.
On the left is the current behaviour, and on the right is with this PR. Note the extra newlines between the Prompts output and the other output from Laravel. This is without redirecting any streams.
I'll need to do more testing to figure out what's going on, but I suspect it's to do with the two places where the $newLinesWritten
is now monitored. In the PromptWriter
class and in Laravel's OutputStyle
class. Previously, it would only be monitored by whichever output was being used, ConsoleOutput
or OutputStyle
(but never both).
Additionally, STDERR doesn't seem to be used when STDOUT is redirected when Prompts are used in Laravel. Only when used standalone.
Sending the prompt output to
stdout
(which is the default behavior) can be problematic in situations where the output of the command needs to be redirected to a file, or piped to another command.> data.json
, the prompt is sent directly to the file and won't be displayed in the console. The command will keep running, waiting for input, but the user won't know what's going on.| grep "foo"
, the prompt might also be invisible, or if the receiving command expects structured data like csv or json, it might cause an error.The fix is easy. Just send the prompt output to
stderr
in stead. That's what Symfony's QuestionHelper does by default. And according to this excellent post by Chris Fidao, it's actually a well established unix convention.Like I mentioned in #107, I think the best solution would be to use stderr by default, but I realize that this could be considered a breaking change. So in stead I've just added a simple
Prompt::useStderr()
method that makes it easy to send all prompts to stderr.I really wanted to add some tests that assert that actual output is sent to stdout, and prompts are send to stderr, but I honestly have no idea how to do that.