Revert "bug #33897 [Console] Consider STDIN interactive (ostrolucky)"

[bendavies]

Q	A
Branch?	4.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	#35988, #33897
License	MIT
Doc PR

This reverts #33897, as it removes detection of a (pseudo-)TTY, while trying to fix a separate issue. it broke many more use cases than it solved.

Some more discussion here.

I'm opening here as @ostrolucky said on slack he didn't want to continue discussion on old PR's. That's fair.

My specific grief with the change was that it breaks docker run when not using a tty.

This should not be interactive:

docker run --rm IMAGE bin/console command

this should be interactive:

docker run --rm -it IMAGE bin/console command

with #33897, both the above commands make the console interactive.

[ostrolucky]

We should consider changing QuestionHelper so it falls back to non-interactive behaviour if it's unable to read the input. That would cover both use cases.

[nicolas-grekas]

I'm with @ostrolucky about the unexpected behavior this produced.
I know no other unix commands that behave like that.
Let's consider #36031 first.

[teohhanhui]

@nicolas-grekas #33897 muddles the water on what (non-)interactive means, using a definition which is inconsistent with the existing UNIX command line tools and literature: #30726 (comment)

As @ostrolucky has admitted in #30726 (comment), it's possible to fix #30726 by untying the behaviour of QuestionHelper from the interactive flag.

That should make sure we don't redefine what "interactive" means and risk breaking many users.

[teohhanhui]

I hope the maintainers can take a good look at the arguments in this and related issues and PRs. Can we come to the agreement that changing the definition of (non-)interactive is a BC break? Fixing QuestionHelper (which is possible to do without changing the definition of interactive anyway) is not enough to mitigate this as there might be a lot of existing code that depends on whether they're being run in (non-) interactive mode.

[ostrolucky]

Nobody was able to provide example of package having such code.

And in same way as changing definition of interactive flag can in theory break code, dropping its usage from QuestionHelper would cause same issue - setting it to false manually would no longer change anything in helper, which is breaking peoples assumption in pretty much same way.

Regarding BC break, yes. But you need to realize every bug fix is BC break for everybody relying on such bug. Change of behaviour is not covered by Symfony BC promise, only change of API.

[teohhanhui]

Nobody was able to provide example of package having such code.

changing definition of interactive flag can in theory break code

Changing the definition of interactivity to go against extensive existing command line tools and literature is not sensible and should never have been done. If the intent really is to have a Symfony-specific definition of interactivity (why?), then it must be well documented (and even then, it'll be a major gotcha) and certainly not something that should be done in a patch release.

dropping its usage from QuestionHelper would cause same issue - setting it to false manually would no longer change anything in helper, which is breaking peoples assumption in pretty much same way.

That's comparing apples to oranges. If people have assumptions about how QuestionHelper works (internally), they are wrong to make those assumptions, and indeed they have no one to blame if their code breaks. On the other hand, having assumptions of what interactivity means is very reasonable considering it's something defined in the POSIX sh standard, among other things.

[ostrolucky]

We already had this disccussion in #30726 (comment)

I don't see any paragraph in your links which would forbid labeling command interactive if it can receive input from user programatically without pressing individual keys. They even go contrary to your opinion: First link mentioning read which is designed to accept input via pipe, second link mentioning sh -i even though people commonly run sh without this flag. If anything, this confirms previous symfony/console had was non-standard.

It's also clearly not such a big BC break because nobody bothered to post link to public project that this change breaks.

I agree it would be better to not do this in patch release. I didn't intend to do that, but it's done.

[teohhanhui]

Let us get into the nitty-gritty of it then.

From https://www.tldp.org/LDP/abs/html/intandnonint.html, if you look at the last code snippet, there's a -t option for test:

-t FD
file descriptor FD is opened on a terminal

Apparently, PHP has stream_isatty (requires PHP 7.2) and posix_isatty (requires the posix extension, of course). But I suppose there could be other means of achieving the same?

First link mentioning read which is designed to accept input via pipe

It wasn't implying that read is only used in interactive shells. Quite the contrary, see:

Let us consider an interactive script to be one that requires input from the user, usually with read statements (see Example 15-3). "Real life" is actually a bit messier than that. For now, assume an interactive script is bound to a tty, a script that a user has invoked from the console or an xterm.

and

Non-interactive scripts can run in the background, but interactive ones hang, waiting for input that never comes. Handle that difficulty by having an expect script or embedded here document feed input to an interactive script running as a background job. In the simplest case, redirect a file to supply input to a read statement (read variable <file). These particular workarounds make possible general purpose scripts that run in either interactive or non-interactive modes.

second link mentioning sh -i even though people commonly run sh without this flag.

There's something that you've missed. From https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sh.html:

If the -i option is present, or if there are no operands and the shell's standard input and standard error are attached to a terminal, the shell is considered to be interactive.

and

Standard input and standard error are the files that determine whether a shell is interactive when -i is not specified.

[teohhanhui]

Why this is important

Compliance to POSIX standards ensures the best chance of interoperability with other command line tools. This is not just about PHP programs. Users will experience unexpected behaviours otherwise (because we generally assume that command line tools play by POSIX rules), that they will then have to work around.

[ostrolucky]

It's ironic that this was done to be more POSIX compliant, as there are no POSIX tools that accept user input, but don't accept them via redirection. This was broken in symfony/console for years, then again my PR was without proper feedback for months, even though title clearly explained very same thing you are arguing against now. My goal was achieved and despite that, I was willing to create 3rd PR provided somebody would link me public project for which such change is BC breaking. Nobody did, so I assume either it doesn't exist or nobody was motivated enough to find it. I don't have energy to make it "perfect" for one set of people, only so another set of people will come complaining about question helper that stopped respecting interactive flag and they have difficulties testing it. If you think this is important, I think it would be better if you create a PR with your sugggestions, then you provide support for these people. You have my support. If you don't like that, watch pull requests of symfony more often so you can provide feedback when there is right time for it.

[danepowell]

This is definitely a BC break. It just killed our project. Here's a passing test (on Symfony Console 4.4.4) and a failing one (on 4.4.5):

• https://travis-ci.com/github/acquia/blt/jobs/299616341
• https://travis-ci.com/github/acquia/blt/jobs/300034549#L953

Note that the text before the failure ("We strive...") comes from the following line and should not appear in the context of this non-interactive command (see the isInteractive() conditional):
https://github.com/acquia/blt/blob/12.x/src/Robo/Hooks/CommandEventHook.php#L55

This should not have changed in a point release, it was very hard to diagnose the root cause. I'm not sure how we're supposed to work around it now.

[nicolas-grekas]

@danepowell can you confirm #36031 fixes the issue for you too?

[danepowell]

@nicolas-grekas thanks, indeed that seems to fix it. Thanks for the reference, I didn't catch that in all of the other discussion.

[helhum]

@ostrolucky Here is a breaking scenario:
A command implements interact, which uses SymfonyStyle::askHidden with a validator that reject empty input.

Calling the command without attached terminal, will still call the interact method, which calls SymfonyStyle::askHidden leading to an infinite loop and the message stty: stdin isn't a terminal and the validation error message that the input must not be empty.

Yes, the command would have failed before as well. But it would have failed with a proper error message that the argument is missing.

Now an infinite loop is triggered, because the hidden response is empty, but it is checked for false

Besides that, even for not hidden questions the output of the command is quite different. Before this change interact was not called which lead to a concise error message, that the argument with "name" is missing. Now users of this command see the question and a MissingInputException and Aborted.

While this can be handled by catching the MissingInputException by the implementer of the command, a code change is required to provide the same user experience, which in fact is a BC promise break.

That said, my conclusion would be, that this change should rather be reverted in Symfony < 5 and the hidden question behaviour needs to be fixed in 5.x

[chalasr]

@helhum Can you please transform your comment into a new issue (following the bug report template)? That would be great.

[ostrolucky]

Indeed, please create issue. Example command would be great as well, because I wasn't able to quite follow what should I do to reproduce this.

[helhum]

@chalasr @ostrolucky here we go: #36565

[ostrolucky]

Issue you created looks awesome! I'm actually surprised of effort you must have put into this. Nevertheless, thanks! 🥇 I'll look into this in detail ASAP.

[helhum]

I'm an OSS maintainer myself and I know how helpful good bug reports are 😃