10000 mGet return false when no keys are provided · Issue #1810 · phpredis/phpredis · GitHub
[go: up one dir, main page]
Skip to content

mGet return false when no keys are provided #1810

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

Jump to bottom
Open
dFayet opened this issue Jul 9, 2020 · 14 comments · May be fixed by #2422
Open

mGet return false when no keys are provided #1810

dFayet opened this issue Jul 9, 2020 · 14 comments · May be fixed by #2422
Labels
documentation problem

Comments

@dFayet
Copy link
dFayet commented Jul 9, 2020 •
edited
Loading

Expected behaviour

According to the mGet doc:

Return value
Array: Array containing the values related to keys in argument

I expect to receive an array in any cases (as long as the keys parameter is an array)

Actual behaviour

If I give an empty array to mGet the return will be false

var_dump($redis->mget([])); // will output bool(false)

I'm seeing this behaviour on

  • OS: Linux
  • Redis: 5.0.6
  • PHP: 7.3
  • phpredis: 5.2.2

Have a good day.
@yatsukhnenko
Copy link
Member

@dFayet mget requires minimum one key

@dFayet
Copy link
Author
dFayet commented Jul 9, 2020 •
edited
Loading

@yatsukhnenko The mget behaviour is probably right, but the doc (and the function return type) is incorrect, or it misses an exception if the $array parameter is empty

@greg0ire
Copy link
greg0ire commented Jul 9, 2020

@yatsukhnenko I would expect that requirement to be enforced somehow (with an exception, for instance)

@yatsukhnenko
Copy link
Member

@greg0ire this will be breaking change and we can't do it until next major release

@greg0ire
Copy link
greg0ire commented Jul 9, 2020

Sure! I'm not in a hurry ;)

@yatsukhnenko
Copy link
Member

I'm not sure about this change. @greg0ire why do you think that returning correct response on incorrect request is right behaviour?

@greg0ire
Copy link
greg0ire commented Jul 9, 2020 •
edited
Loading

If by "correct response", you mean false, I don't see how it is correct: it's not documented, and it makes the return type a union type. I'd much rather get an exception than just false. Many PHP functions return false in case of failure, but I believe this is the legacy of a time when exceptions did not exist. And PHP is moving away from that in some cases: https://wiki.php.net/rfc/json_throw_on_error

@michael-grunder
Copy link
Member

I think what @yatsukhnenko is saying is that MGET with no keys is a syntax error

127.0.0.1:6379> mget
(error) ERR wrong number of arguments for 'mget' command

Phpredis is identifying this and avoiding a round trip to the server. That said I'm not opposed to just returning an empty array here but like Pavlo mentioned it would be a breaking change so it has to wait until PhpRedis 6.

@greg0ire
Copy link
greg0ire commented Jul 9, 2020

I'm not suggesting you return an empty array, I'm suggesting you throw an exception, which would make the signature of mget be mGet(array $keys): array rather than mGet(array $keys): array|false (and should wait until PhpRedis 6 too, for obvious reasons).

@yatsukhnenko
Copy link
Member

@greg0ire json_decode throws exception because it may return false/null is success and failure cases which doesn't allow to identify error. In phpredis we don't have this problem and use false may definetly identify error. Also I think that simple compare to false is better to read and understand than wrapping code to try/catch blocks

@dFayet
Copy link
Author
dFayet commented Jul 9, 2020 •
edited
Loading

@yatsukhnenko It might be easier but you will allow an incorrect behaviour, as the exception is thrown because the user tries to mget with no keys (which is not supported, as you said earlier).
From a DX pov I think it is more easier to debug an exception than a false.

@greg0ire
Copy link
greg0ire commented Jul 9, 2020

Also I think that simple compare to false is better to read and understand than wrapping code to try/catch blocks

It is indeed simpler! The thing is, that kind of exception is not meant to be caught, and should instead result in a rewrite of the calling code, just like @dFayet and I rewrote our code today to avoid calling mGet with an empty array. Having a clear exception message telling us should not call mGet with an empty array would have IMO been a far better experience than getting a TypeError about false not being an array as we did.

@nesl247 nesl247 mentioned this issue Sep 16, 2020
Closed
@dkarlovi dkarlovi mentioned this issue Nov 21, 2023
Closed
@dkarlovi
Copy link
dkarlovi commented Nov 21, 2023 •
edited
Loading

This should be revisited because the signature is now \Redis::mget(): \Redis|array, it mustn't return false (or the stub needs to be fixed), currently it says it will do one thing and then does another.

@alexandre-daubois alexandre-daubois linked a pull request Nov 23, 2023 that will close this issue
Open
@sirian
Copy link
sirian commented Apr 4, 2024 •
edited
Loading

false also returned in case of errors like noAUTH Authentication required. Tested with phpredis 6.0.2, so stubs are wrong

phpredis/redis.stub.php

Line 2178 in 62cf943

public function mget(array $keys): Redis|array;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
documentation problem
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Throw when passing an empty array to mget() instead or returning false alexandre-daubois/phpredis
0