Skip to content

Conversation

iluuu1994
Copy link
Member

@iluuu1994 iluuu1994 commented Sep 30, 2025

Without this, one can trivially run into test failures by not connecting all tty pipes to the run-tests.php process, or by running it inside a bash script. E.g. this will cause test failures:

$ php run-tests.php tests/output 2> /dev/null

See GH-19975

@iluuu1994 iluuu1994 requested a review from Girgias September 30, 2025 14:36
@bwoebi
Copy link
Member

bwoebi commented Sep 30, 2025

Is there a better way to determine tty? The tty tests obviously also test the tty functions, of which you use one. If it were broken (e.g. return false if it shouldn't), well, the tests to indicate that would just not run either.

@iluuu1994
Copy link
Member Author

@bwoebi Well, any concrete suggestions? The tests do more than just check for istty(). I'm not sure what other solution we have.

@bwoebi
Copy link
Member

bwoebi commented Oct 1, 2025

@iluuu1994 Having the shall check for tty via [ -t 0 ] && [ -t 1 ] && [ -t 2 ]

@iluuu1994
Copy link
Member Author

Well, it would need to be portable and I don't think the risk of hiding true failures is big enough to invest too much time here, at least on my part.

@bwoebi
Copy link
Member

bwoebi commented Oct 1, 2025

@iluuu1994 I don't mind doing it the proposed way, it was merely a question / suggestion.

@iluuu1994
Copy link
Member Author

Okay, thanks. I understand the concern, I had the same thought. I just couldn't think of a simpler solution that's also portable.

@hakre
Copy link
Contributor

hakre commented Oct 1, 2025

Under circumstances, STDIN, STDOUT or STDERR may not be available as constants in PHP cli SAPI.

For the check to become more robust, it should check if the constants are actually define()-ed.

This might be useful if you are concerned about the stability, so just FYI @iluuu1994

@bwoebi
Copy link
Member

bwoebi commented Oct 1, 2025

@hakre Such as? I.e. when would that ever happen on cli?

@iluuu1994
Copy link
Member Author

I quickly asked Niels since he's more knowledgeable on streams, and this can only happen with preloading or explicitly closing stdin, out or err, which seems rather intentional. E.g. in bash you can do php test.php 2>&-. But this is not a realistic scenario we need to guard against.

@hakre
Copy link
Contributor

hakre commented Oct 1, 2025

@hakre Such as? I.e. when would that ever happen on cli?

When for example the PHP script is read from standard input. This is not the normal invocation of the run-tests.php script but a prominent example over PHP versions.

$ < run-tests.php | php -- -j2
...
PHP   5. stream_socket_accept($socket = resource(20621) of type (stream), $timeout = 5) Standard input code:1417
ERROR: Failed to accept connection from worker.

This example is construed, using the workes (-j2) provokes the FAILURE here.

So this is merely an FYI, we can always argue with GIGO but how I read the check as care-taking it might still be useful as the behaviour changed often in the past. The example on passing the script via standard input is stable because PHP can't offer the standard input to the script as STDIN as it has to consume it itself to read the script - this would just not make sense.

These three constants are comewhat special as they are added to the end of the Core and the overall constant table:

$ sapi/cli/php -r 'print_r(array_slice(get_defined_constants(true)["Core"], -3));'
Array
(
    [STDIN] => Resource id #1
    [STDOUT] => Resource id #2
    [STDERR] => Resource id #3
)
$ sapi/cli/php -r 'print_r(array_slice(get_defined_constants(), -3));'       
Array
(
    [STDIN] => Resource id #1
    [STDOUT] => Resource id #2
    [STDERR] => Resource id #3
)

@bwoebi
Copy link
Member

bwoebi commented Oct 1, 2025

explicitly closing stdin, out or err

@iluuu1994 The primary STDIN/STDERR/STDOUT constants will look at the stdin/stdout/stderr C symbols, which are always defined. Only explicitly opening php://stderr (etc.) at runtime may actually fail.

@iluuu1994
Copy link
Member Author

The constants are always defined, but they may be null, which will crash run-tests.php. But that seems acceptable, as it seems quite intentional.

@hakre
Copy link
Contributor

hakre commented Oct 1, 2025

The constants are always defined, but they may be null, which will crash run-tests.php. But that seems acceptable, as it seems quite intentional.

It's even more esoteric in the details from time to time. I've seen tests failing when invoking CLI where (one of) the constants actually were not defined (and therefore fatal error on using them), but as you wrote it's a consideration thing and we don't want to consider it here. All fine. (and yes normally the constants are always defined even if the descriptor is closed, as @bwoebi wrote).

@iluuu1994 iluuu1994 closed this in 8315977 Oct 2, 2025
@iluuu1994
Copy link
Member Author

Ok, I added defined() tests, they shouldn't hurt at least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants