[Console] Refactor isTtySupported() #46971
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
| if (null === $isTtySupported && \function_exists('proc_open')) { | ||
| $isTtySupported = (bool) @proc_open('echo 1 >/dev/null', [['file', '/dev/tty', 'r'], ['file', '/dev/tty', 'w'], ['file', '/dev/tty', 'w']], $pipes); | ||
| if (null === $isTtySupported) { | ||
| $isTtySupported = ('/' === \DIRECTORY_SEPARATOR && stream_isatty(\STDOUT)); |
There was a problem hiding this comment.
IIUC, stream_isatty supports Windows, so do we need the '/' === \DIRECTORY_SEPARATOR part?
There was a problem hiding this comment.
Windows does not support echo 1 >/dev/null, so we need '/' === \DIRECTORY_SEPARATOR
There was a problem hiding this comment.
I don't get it, sorry. Are you saying that stream_isatty is using echo 1 >/dev/null behind the scenes?
There was a problem hiding this comment.
Sorry!
Windows does not support echo 1 >/dev/null, so we need '/' === \DIRECTORY_SEPARATOR.
Use stream_isatty() to determine if tty is supported.
There was a problem hiding this comment.
but you don't use echo 1 >/dev/null anymore, and stream_isatty supports windows natively. So this check become useless, isn't?
There was a problem hiding this comment.
To be clear, I think having $isTtySupported = stream_isatty(\STDOUT); is enough.
There was a problem hiding this comment.
getCurrentPosition() does not support in windows.
stream_isatty(\STDOUT) may also return true in windows.
php:
$cursor = new Cursor($output);
var_dump($cursor->getCurrentPosition());log:
'stty' is not recognized as an internal or external command,
operable program or batch file.
'stty' is not recognized as an internal or external command,
operable program or batch file.
'stty' is not recognized as an internal or external command,
operable program or batch file.
array(2) {
[0]=>
NULL
[1]=>
NULL
}
|
Thank you @Yurunsoft. |
|
This PR has broken the isTtySupported specifically in PHPStorm using the built in run window on a Mac M1 Pro Chip. By manually changing the method to the old code, everything works fine. How can we debug the issue here? |
I run this code in Windows+PHPStorm 2022.3.1+wsl and the result is consistent. Could you test this code? <?php
declare(strict_types=1);
var_dump((bool) @proc_open('echo 1 >/dev/null', [['file', '/dev/tty', 'r'], ['file', '/dev/tty', 'w'], ['file', '/dev/tty', 'w']], $pipes));
var_dump(stream_isatty(\STDOUT));
|
|
Also having this issue, please see screenshot below of your code, the upper is the phpstorm terminal, the lower is the phpstorm run window. Phpstorm 2022.3.1 on 2021 Mac M1 Pro.
|
|
Thanks for helping @audiojames |
stream_isatty(): https://www.php.net/manual/en/function.stream-isatty.php (PHP 7 >= 7.2.0, PHP 8)