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

Relax requirements on set's expire argument #1830

Merged
merged 3 commits into from
Aug 30, 2020

Conversation

michael-grunder
Copy link
Member

See: #1783

redis_commands.c Outdated
} else if (z_opts && Z_TYPE_P(z_opts) == IS_LONG) {
expire = Z_LVAL_P(z_opts);
} else if (z_opts && Z_TYPE_P(z_opts) != IS_NULL) {
if (Z_TYPE_P(z_opts) != IS_LONG && Z_TYPE_P(z_opts) != IS_STRING) {
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need add check for IS_DOUBLE and additional check is_numeric_string for IS_STRING?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, we should accept IS_DOUBLE zvals as well.

I'm not sure how much is_numeric_string gets us because I believe non-strings will resolve to 0 which will fail before sending the command because I verify that the TTL is greater than zero.

I suppose it would let us be even more specific about the error but I'm not sure it's worth the extra logic.

@michael-grunder
Copy link
Member Author

@yatsukhnenko Does the latest change look good to you?

@yatsukhnenko
Copy link
Member

@michael-grunder I want to check how it will work with non-numeric strings

@michael-grunder
Copy link
Member Author

What happens is that a non-numeric string (e.g. 'foo') converts to 0 and you get a warning about TTL < 1

<?php

$obj_r = new Redis();
$obj_r->connect('localhost', 6379);
var_dump($obj_r->set('foo', 'bar', 'notastring'));
?>
$ sapi/cli/php setex.php 

Warning: Redis::set(): EXPIRE can't be < 1 in /home/mike/dev/phpfarm/src/php-7.4.8-debug/setex.php on line 6
bool(false)

I suppose there is a risk of doing something like this:

$obj_r->set('foo', 'bar', '12345notastring');
?>

I don't mind also using is_numeric_string to make it impossible to use incorrectly.

@yatsukhnenko
Copy link
Member

I worried about strings like that 12345notastring

@michael-grunder michael-grunder merged commit 3645807 into develop Aug 30, 2020
@michael-grunder
Copy link
Member Author

Merged, but now that I look at the code I think I should probably also use redis_try_get_expiry in the case where our user passes an options array.

@michael-grunder michael-grunder deleted the issue.1783-set-expiry branch August 30, 2020 20:52
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

Successfully merging this pull request may close these issues.

None yet

2 participants