Accept null for microseconds argument in stream_select()

[kelunik]

The deprecation of passing null is thrown otherwise.

If the timeout is calculated conditionally before calling stream_select(), passing 0 to avoid the deprecation seems unreasonable, example:

https://github.com/amphp/amp/blob/7d4bbc6e0b47c6bb39b6cce1a4b5942e0c5125fb/lib/Loop/NativeDriver.php#L286

[nikic]

I think this makes sense. If $sec is nullable, then $usec should be too.

cc @kocsismate

[nikic]

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

[kelunik]

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

[nikic]

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.

[kelunik]

Should we deprecate passing 0 or just allow it?

[kelunik]

Downgraded passing 0 to a deprecation message.

[kocsismate]

I think this makes sense. If $sec is nullable, then $usec should be too.

+1

[zobo]

I understand this has as been in the 8.1 tree a while, but I just stumbled on it in the exact same spot as the mentioned issue. Writing code that fully satisfies PHP <= 8.0 and >= 8.1 is.. well.. impossible and results in ugly patches as this https://github.com/sabre-io/event/pull/88/files

Since <= 8.0 did not allow $usec to be NULL there is no way to avoid a deprecation notice - or write version specific split code.

I would suggest to allow $usec to be either NULL or 0.

[nikic]

I'd be okay with dropping the deprecation. Don't see much value in it.

[zobo]

Can I help in any way?

[kelunik]

@zobo I haven't seen any issue with passing $usec = null on PHP 7.4 and earlier. Do you have a reproduction script that shows this isn't allowed?

[zobo]

PHP 8.0 and before define the $usec argument as Z_PARAM_LONG https://github.com/php/php-src/blob/PHP-7.4.25/ext/standard/streamsfuncs.c#L764
if you enable declare(strict_types=1); it will throw an error

7.4.13:
Fatal error: Uncaught TypeError: stream_select() expects parameter 5 to be int, null given in ...

8.0.12:
Fatal error: Uncaught TypeError: stream_select(): Argument #5 ($microseconds) must be of type int, null given in ...

code

<?php

declare(strict_types=1);

$r = $w = $e = [STDIN];
stream_select($r, $w, $e, null, null);

[kelunik]

Strict types have been the missing piece, we're not using them at @amphp, so we didn't see any TypeError.

Regarding the usefulness of the deprecation:

var_dump(stream_select($stream, $empty, $empty, 0, null));

Returns immediately.

var_dump(stream_select($stream, $empty, $empty, null, 0));

Blocks forever.

Given this difference, the deprecation still seems relevant to me.

[zobo]

Yes. But thats controlled by the 4th parameter $sec. Deprecation notice is about the 5th.

[zobo]

I have created a PR that makes the proposed change, removing the deprecation warning.