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.

Sign up for GitHub

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

Accept null for microseconds argument in stream_select() #6879

Merged
merged 4 commits into from
Apr 22, 2021
Merged

Accept null for microseconds argument in stream_select() #6879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
10684e4
Accept null for microseconds argument in stream_select()
kelunik Apr 18, 2021
bc3eaf8
Fix tests
kelunik Apr 18, 2021
44f451e
Downgrade passing microseconds 0 to deprecation message
kelunik Apr 20, 2021
091714f
Adjust message
kelunik Apr 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ext/standard/basic_functions.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1209,7 +1209,7 @@ function soundex(string $string): string {}

/* streamsfuncs.c */

function stream_select(?array &$read, ?array &$write, ?array &$except, ?int $seconds, int $microseconds = 0): int|false {}
function stream_select(?array &$read, ?array &$write, ?array &$except, ?int $seconds, ?int $microseconds = null): int|false {}

/** @return resource */
function stream_context_create(?array $options = null, ?array $params = null) {}
Expand Down
4 changes: 2 additions & 2 deletions ext/standard/basic_functions_arginfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* This is a generated file, edit the .stub.php file instead.
* Stub hash: 2d92e992837a61a052eea6d0257837aaccc54be6 */
* Stub hash: 55f11c2d6cc7ba342b1062104f3838866ad9135d */

ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_set_time_limit, 0, 1, _IS_BOOL, 0)
ZEND_ARG_TYPE_INFO(0, seconds, IS_LONG, 0)
Expand Down Expand Up @@ -1847,7 +1847,7 @@ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(arginfo_stream_select, 0, 4, MAY_BE_LONG
ZEND_ARG_TYPE_INFO(1, write, IS_ARRAY, 1)
ZEND_ARG_TYPE_INFO(1, except, IS_ARRAY, 1)
ZEND_ARG_TYPE_INFO(0, seconds, IS_LONG, 1)
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, microseconds, IS_LONG, 0, "0")
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, microseconds, IS_LONG, 1, "null")
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_INFO_EX(arginfo_stream_context_create, 0, 0, 0)
Expand Down
12 changes: 11 additions & 1 deletion ext/standard/streamsfuncs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,6 +745,7 @@ PHP_FUNCTION(stream_select)
int retval, sets = 0;
zend_long sec, usec = 0;
bool secnull;
bool usecnull = 1;
int set_count, max_set_count = 0;

ZEND_PARSE_PARAMETERS_START(4, 5)
Expand All @@ -753,7 +754,7 @@ PHP_FUNCTION(stream_select)
Z_PARAM_ARRAY_EX2(e_array, 1, 1, 0)
Z_PARAM_LONG_OR_NULL(sec, secnull)
Z_PARAM_OPTIONAL
Z_PARAM_LONG(usec)
Z_PARAM_LONG_OR_NULL(usec, usecnull)
ZEND_PARSE_PARAMETERS_END();

FD_ZERO(&rfds);
Expand Down Expand Up @@ -788,6 +789,15 @@ PHP_FUNCTION(stream_select)

PHP_SAFE_MAX_FD(max_fd, max_set_count);

if (secnull && !usecnull) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should allow || usec == 0 here, so those tests wouldn't have broken? After all, this was documented as the default.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about that, too, but there's no reason to pass 0 in these tests.

Copy link
Member

Choose a reason for hiding this comment

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

I'm mainly concerned about people who have defined a wrapper around this function that uses $usec = 0 as the default value, because that's the default we used to document. They would now get an exception.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we deprecate passing 0 or just allow it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Downgraded passing 0 to a deprecation message.

if (usec == 0) {
php_error_docref(NULL, E_DEPRECATED, "Argument #5 ($microseconds) should be null instead of 0 when argument #4 ($seconds) is null");
} else {
zend_argument_value_error(5, "must be null when argument #4 ($seconds) is null");
RETURN_THROWS();
}
}

/* If seconds is not set to null, build the timeval, else we wait indefinitely */
if (!secnull) {
if (sec < 0) {
Expand Down
2 changes: 1 addition & 1 deletion ext/standard/tests/general_functions/proc_open_sockets1.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ while ($pipes) {
$w = null;
$e = null;

if (!stream_select($r, $w, $e, null, 0)) {
if (!stream_select($r, $w, $e, null)) {
throw new Error("Select failed");
}

Expand Down
2 changes: 1 addition & 1 deletion ext/standard/tests/general_functions/proc_open_sockets2.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ function poll($pipe, $read = true)
$w = ($read == false) ? [$pipe] : null;
$e = null;

if (!stream_select($r, $w, $e, null, 0)) {
if (!stream_select($r, $w, $e, null)) {
throw new \Error("Select failed");
}
}
Expand Down
2 changes: 1 addition & 1 deletion ext/standard/tests/general_functions/proc_open_sockets3.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ function poll($pipe, $read = true)
$w = ($read == false) ? [$pipe] : null;
$e = null;

if (!stream_select($r, $w, $e, null, 0)) {
if (!stream_select($r, $w, $e, null)) {
throw new \Error("Select failed");
}
}
Expand Down
36 changes: 36 additions & 0 deletions ext/standard/tests/streams/stream_select_null_usec.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
--TEST--
stream_select allows null for microsecond timeout if timeout is null
--FILE--
<?php

$read = [fopen(__FILE__, 'r')];
$write = null;
$except = null;

error_reporting(-1);
set_error_handler(function ($errno, $errstr) {
print $errno . " " . $errstr . "\n";
});

stream_select($read, $write, $except, null, null);
var_dump($read);

print "\n";

stream_select($read, $write, $except, null, 0);

stream_select($read, $write, $except, null, 1);
?>
--EXPECTF--
array(1) {
[0]=>
resource(%d) of type (stream)
}

8192 stream_select(): Argument #5 ($microseconds) should be null instead of 0 when argument #4 ($seconds) is null

Fatal error: Uncaught ValueError: stream_select(): Argument #5 ($microseconds) must be null when argument #4 ($seconds) is null in %s
Stack trace:
#0 %s stream_select(Array, NULL, NULL, NULL, 1)
#1 {main}
%s