wp_rand( 0, 0) result in random integer

[mlajo]

wp_rand( 0, 0) result in random integer instead of 0

[azouamauriac]

Hello welcome to WordPress trac! thanks for your report, you're right, I'm able to reproduce the issue, the function was introduce here [8728]. For what I understand it used to replace

mt_rand
}}} but "why" i don't know.

pinging @ryan as he was author of commit, may be he can enlighten us.
And @SergeyBiryukov for milestone. I think the ticket should be milestone for 6.0 if it's accepted.

Also the function always returns abs value(e.g wp_rand( -1, -1) = 1 instead of -1)

[swissspidy]

Ryan is not really active anymore, so I doubt you'll get a response :-)

The function does not really support $max = 0. $max = 0 means use the maximum upper bound, which is 4294967295.

So this works as expected.

And the function does not support negative values by design.

Perhaps the inline documentation could be improved a bit.

[SergeyBiryukov]

Replying to azouamauriac:

I'm able to reproduce the issue, the function was introduce here [8728]. For what I understand it used to replace mt_rand RE ​https://www.php.net/manual/en/function.mt-rand.php.
but "why" i don't know.

You can find more details in the ​WordPress 2.6.2 release post.

Looks like the current behavior is intentional, see for example this line added in [34981] / #28633:

$_max = ( 0 != $max ) ? $max : $max_random_number;

This is also consistent with mt_rand(), which returns a random integer and not 0 when parameters are omitted. So changing this behavior does not seem feasible due to security implications, but we can update the documentation to mention that the $max value is only taken into account if it's greater than zero.

[SergeyBiryukov]

Something like this I think would be correct:

 * @param int $min Optional. Lower limit for the generated number.
 *                 Accepts positive integers or zero. Default 0.
 * @param int $max Optional. Upper limit for the generated number.
 *                 Accepts positive integers. Defaults to 4294967295.

[tobifjellner]

@SergeyBiryukov
It would probably be good also to explicitly say that 0 is treated as Max...

[mlajo]

Replying to SergeyBiryukov:

This is also consistent with mt_rand(), which returns a random integer and not 0 when parameters are omitted. So changing this behavior does not seem feasible due to security implications, but we can update the documentation to mention that the $max value is only taken into account if it's greater than zero.

mt_rand( 0, 0 ) returns 0

[SergeyBiryukov]

Replying to mlajo:

Replying to SergeyBiryukov:

This is also consistent with mt_rand(), which returns a random integer and not 0 when parameters are omitted.

mt_rand( 0, 0 ) returns 0

Indeed, thanks! I meant mt_rand() without any parameters, in that case $max defaults to mt_getrandmax().

I guess wp_rand() too should have used null as the default value for both parameters, so that we could differentiate between wp_rand() and wp_rand( 0, 0 ), which is not currently possible as zero is the default value. But it's too late to change that now without security implications.

[mlajo]

This is not consistent with mt_rand and contra intuitive. It caused lots of trouble in our code until we pinpoint it to this function. Also coding standard ​https://github.com/WordPress/WordPress-Coding-Standards/blob/develop/WordPress/Sniffs/WP/AlternativeFunctionsSniff.php recommend to use it.
mt_rand() is discouraged. Use the far less predictable wp_rand() instead.

[SergeyBiryukov]

Replying to mlajo:

This is not consistent with mt_rand and contra intuitive.

What I meant by consistency is that both mt_rand() and wp_rand(), when called without any parameters, return a random number. But you're right that the results for mt_rand( 0, 0 ) and wp_rand( 0, 0 ) are inconsistent.

On second thought, maybe we could still fix it, see 55194.diff​. This would need a unit test and security review.

It caused lots of trouble in our code until we pinpoint it to this function.

Just out of curiousity, what would be the use case for a wp_rand( 0, 0 ) call?

[azouamauriac]

Replying to SergeyBiryukov:

Just out of curiousity, what would be the use case for a wp_rand( 0, 0 ) call?

may be $min and $max are dynamiques variables, then it can happen that both of them get 0 as value, if that wp_rand will not given expected result... that's my mind... let's @mlajo enlighten us.

[mlajo]

Replying to SergeyBiryukov:

Just out of curiousity, what would be the use case for a wp_rand( 0, 0 ) call?

something as @azouamauriac said, we have code with wp_rand( 0, $var ) where $var can sometimes be 0. Now we added one "if check" before to check if $var is 0 but it was confusing to debug because mt_rand() worked as expected.

If it stays like this it would be nice to put some kind of notice for that case in documentation.

[acoulombe]

Replying to SergeyBiryukov:

On second thought, maybe we could still fix it, see 55194.diff​. This would need a unit test and security review.

I'm working on writing tests based on your fix.

For my knowledge, i'm relatively new to contributing WordPress. How do you normally work, can I make a pull request according to your .diff or do you want to make one yourself ?

Trac ticket: https://core.trac.wordpress.org/ticket/55194

[peterwilsoncc]

I'm moving this to the 6.1 milestone as it's technically a backward compatibility break, which doesn't seem great in the hours leading up to the RC. The change in behavior will also need a dev-note and there isn't time to put one together.

I did a search of the plugin repo and found an instance of a plugin using wp_rand(0, 0) to generate a random key. I found one so prior to commit someone will need to get in touch with the author of the plugin.

[audrasjb]

@peterwilsoncc I didn't find any plugin using this with wpdirectory search. Could you please provide a few insights about how you found them? Thanks!

[peterwilsoncc]

@audrasjb Sorry, I intended to link it but must have been rushing. This is the search I was using ​https://wpdirectory.net/search/01G37HMMWQRZ6QHH9CVMMSGP8R

[audrasjb]

Ah thank you I didn't succeed to find the plugin, but my regular expression was wrong.
I'll try to reach out to the plugin author!

[marekdedic]

Hi,
plugin author here, thanks for the heads up. I'll change it so that you can go forward with this change.

I am a bit stumped as to how this code even came to be - I was definitely counting on it being a random number and not 0... :/

[marekdedic]

Hi,
we have updated the plugin to no longer depend on this behaviour.

Thanks for the heads up

[audrasjb]

Great thank you @marekdedic!
I think we're good to ship this once the suggested change in @costdev’s PR is implemented :)

[costdev]

@audrasjb PR updated with version numbers as requested by Sergey. I believe that's all changes done.

Adding needs-dev-note per comment 15.

[audrasjb]

Self assigning for final review and commit.

[audrasjb]

In 53473:

General: Ensure wp_rand() returns 0 when $min and $max values are equal to 0.

This changeset ensures wp_rand() returns zero instead of a random number when both $min and $max values are equal to zero.

Fixes #55194.

[audrasjb]

Oops I missed the props line in my commit, to credit the contributors listed in this ticket. I'll add them right now manually, using the tool provided by Make/Core P2 website. Sorry.

Committed in https://core.trac.wordpress.org/changeset/53473

[audrasjb]

credits added manually :)

[SergeyBiryukov]

In 53477:

Tests: Use more descriptive names for wp_rand() test methods.

This aims to clarify the intention of the tests.

Follow-up to [53473].

Props mlajo, costdev, acoulombe, azouamauriac, swissspidy, tobifjellner, peterwilsoncc, audrasjb, marekdedic, SergeyBiryukov.
See #55194.

[SergeyBiryukov]

It looks like the tests are not quite correct, this failure now ​happens occasionally:

1) Tests_Pluggable_wpRand::test_wp_rand_should_return_a_positive_integer with data set "-1 and 99" (-1, 99)
The value was not greater than 0
Failed asserting that 0 is greater than 0.

/var/www/tests/phpunit/tests/pluggable/wpRand.php:22
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97

Looking at the test, it makes the assumption that the return value should always be greater than zero, due to using absint() on the final result. However, since ​random_int() works with negative numbers too, given a range between -1 and 99, it can return zero as a valid result, which is then returned by wp_rand() unchanged.

55194.3.diff​ corrects the affected assertion.

[SergeyBiryukov]

In 53479:

Tests: Correct an assertion in wp_rand() tests.

The function returns non-negative integers, which includes zero.

Follow-up to [53473], [53477].

See #55194.

[SergeyBiryukov]

Replying to audrasjb:

In 53473:

General: Ensure wp_rand() returns 0 when $min and $max values are equal to 0.

This changeset ensures wp_rand() returns zero instead of a random number when both $min and $max values are equal to zero.

Fixes #55194.

It might be helpful to mention in the dev note that this brings parity with the random_int() and mt_rand() native PHP functions.