Skip to content
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

Avoid accessing undefined variables in run-tests.php #8669

Merged
merged 1 commit into from Jun 7, 2022

Conversation

IMSoP
Copy link
Contributor

@IMSoP IMSoP commented May 31, 2022

The $php_cgi and $phpdbg cases here are definitely real bugs, and
caused the script to bail out under certain ini settings.

The other paths may actually be unreachable in practice, but were
highlighted by PhpStorm.

run-tests.php Outdated
Comment on lines 938 to 943
if (!getenv('NO_INTERACTION') && !TRAVIS_CI) {
$fp = fopen("php://stdin", "r+");
if (getenv('NO_INTERACTION') || TRAVIS_CI) {
$stdin = null;
$user_input = '';
}
else {
$stdin = fopen("php://stdin", "r");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code path would be refactored if #8657 lands

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bah! I tried to search for relevant open PRs, and found none. Maybe I should have stuck to the actual fix I needed, which was the $php_cgi var.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, please remove the changes to mail sending, and leave that to the other PR; other that that, this patch looks good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased, so now just has the smaller variable fixes.

The $php_cgi and $phpdbg cases here are definitely real bugs, and
caused the script to bail out under certain ini settings.

The other paths may actually be unreachable in practice, but were
highlighted by PhpStorm.
@Girgias Girgias merged commit ed6dab2 into php:master Jun 7, 2022
@Girgias
Copy link
Member

Girgias commented Jun 7, 2022

Thank you!

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

Successfully merging this pull request may close these issues.

None yet

4 participants