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

Use positive-int|0 for sleep() argument #6978

Closed
evannoronha opened this issue Nov 23, 2021 · 4 comments
Closed

Use positive-int|0 for sleep() argument #6978

evannoronha opened this issue Nov 23, 2021 · 4 comments

Comments

@evannoronha
Copy link

Problem

php-src says $seconds cannot be < 0. https://github.com/php/php-src/blob/ef5558a8cda5a8c62e8de7694202ac669d74f1b3/ext/standard/basic_functions.c#L1216-L1230

PHP_FUNCTION(sleep)
{
	zend_long num;

	ZEND_PARSE_PARAMETERS_START(1, 1)
		Z_PARAM_LONG(num)
	ZEND_PARSE_PARAMETERS_END();

	if (num < 0) {
		zend_argument_value_error(1, "must be greater than or equal to 0");
		RETURN_THROWS();
	}

	RETURN_LONG(php_sleep((unsigned int)num));
}

As does the php docs: https://www.php.net/manual/en/function.sleep.php#refsect1-function.sleep-errors

If the specified number of seconds is negative, this function will generate a E_WARNING.

sleep(0) has long been a way of coaxing PHP to yield to the OS.

In #6861, we modified the call map to specify that sleep() should only take a positive-int

Do This

Modify the call map to allow the $seconds argument passed to sleep() to be positive-int|0.

@psalm-github-bot
Copy link

Hey @evannoronha, can you reproduce the issue on https://psalm.dev ?

@evannoronha
Copy link
Author

https://psalm.dev/r/54897797d8

Seems like https://psalm.dev is running 4.12.0?

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/54897797d8
<?php

/**
 * @return array<string>
 */
function takesAnInt(string $i) {
    sleep(0);
    return [$i, "hello"];
}
Psalm output (using commit eca56c0):

No issues!

@orklah
Copy link
Collaborator

orklah commented Nov 23, 2021

No, it's running the future 4.14.0 where this is already changed ;)
#6955

I'll close this as solved!

@orklah orklah closed this as completed Nov 23, 2021
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

No branches or pull requests

2 participants