Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Automatically skip tty tests if not on tty #20013

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
iluuu1994 wants to merge 1 commit into php:PHP-8.3 from iluuu1994:auto-skip-tty-tests

Conversation

Copy link
Member

@iluuu1994 iluuu1994 commented Sep 30, 2025
edited
Loading

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

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.

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.

Copy link
Member

bwoebi commented Oct 1, 2025

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

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.

Copy link
Member

bwoebi commented Oct 1, 2025

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

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.

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

Copy link
Member

bwoebi commented Oct 1, 2025
edited
Loading

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

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.

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
)

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.

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.

Copy link
Contributor

hakre commented Oct 1, 2025
edited
Loading

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).

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
Reviewers

@Girgias Girgias Girgias approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /