Bug #79805 [PATCH]: sapi_windows_vt100_support throws TypeError when not able to analyze the stream

Edit report at https://bugs.php.net/bug.php?id=79805&edit=1 ID: 79805 Patch added by: cmb@php.net Reported by: ondrej at mirtes dot cz Summary: sapi_windows_vt100_support throws TypeError when not able to analyze the stream Status: Assigned Type: Bug Package: Streams related Operating System: Windows PHP Version: 8.0.0alpha1 Assigned To: cmb Block user comment: N Private report: N New Comment: The following pull request has been associated: Patch Name: Fix #79805: sapi_windows_vt100_support throws TypeError On GitHub: https://github.com/php/php-src/pull/5863 Patch: https://github.com/php/php-src/pull/5863.patch Previous Comments: ------------------------------------------------------------------------ [2020-07-08 10:36:13] cmb@php.net Not even emitting a warning when used as getter is fine for me as well, but I think consequently when used as setter, the function shouldn't throw (but raise a warning only). ------------------------------------------------------------------------ [2020-07-08 08:29:17] nikic@php.net I think I agree with @nicolasgrekas here that this should just return false (without warning) for non-consoles (in fact it already does for some of them -- but only those that are castable to FDs). ------------------------------------------------------------------------ [2020-07-07 17:46:45] cmb@php.net Ugh. First, it should be noted that the function throws a TypeError instead of a warning with PHP 7 if strict_types are enabled. That suggest that the function was intended to only be called on valid "ttys", so I tend to change this ticket to doc bug. I'm not really sure about this, so further input would be welcome. ------------------------------------------------------------------------ [2020-07-07 15:29:17] requinix@php.net It appears to be deliberate in the sense that the problem was classified as a sort of "type error" and was thus updated to throw actual TypeErrors. Quickly scanning over the source, the logic for whether that exception is thrown is also used with whether stream_isatty() returns true. Thus I would suggest a change like if (\DIRECTORY_SEPARATOR === '\\') { return (\function_exists('sapi_windows_vt100_support') + && stream_isatty($this->stream) - && @sapi_windows_vt100_support($this->stream)) + && sapi_windows_vt100_support($this->stream)) || false !== getenv('ANSICON') || 'ON' === getenv('ConEmuANSI') || 'xterm' === getenv('TERM'); } Before I found stream_isatty() I would have agreed that a warning is more appropriate, but now I'm not so sure. Either way, I think the docs for sapi_windows_vt100_support() should mention stream_isatty() too. ------------------------------------------------------------------------ [2020-07-07 15:26:50] nicolasgrekas@php.net I may be missing something, but neither the warning nor the TypeError look legit to me. The function should just return false instead IIUC. ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=79805 -- Edit this bug report at https://bugs.php.net/bug.php?id=79805&edit=1