HSCAN in PHPRedis is not iterating properly

[ragingdave]

• Laravel Version: 5.6.21
• PHP Version: 7.1
• Database Driver & Version: NA
• Redis Extension Version: 4.0.2

Description:

The hscan and perhaps additional commands are broken when using phpredis vs predis.
Predis has a completely different return syntax, which includes the cursor and results.
PHPRedis returns only the results and seems to use the $cursor ($it in their examples) by reference rather than by value some something ends up getting lost. Additionally, predis starts with $it/$cursor of 0, wheras PHPRedis starts with null.

Steps To Reproduce:

Using laravel and it's redis facade:

for ($x = 0; $x < 100; $x++) {
    Redis::incrby('test_key', uniqid(), 1);
}

$it = null;
// Redis connection in Laravel already has the OPTs set shown in the readme.
do {
    $return = Redis::hscan('test_key'); // FAILS missing arg
    $return = Redis::hscan('test_key', 0); // Returns false
    $return = Redis::hscan('test_key', null); // Returns the same 10 records over and over forever
} while (!$empty($return));

[tillkruss]

Laravel uses Predis as a base line and it adjusts PhpRedis return values to same results in PhpRedisConnection.

If you can, please submit a PR to 5.6 to fix the return value HSCAN. I'd assume SCAN, SSCAN and ZSCAN are broken as well?

[ragingdave]

I hadn't specifically tested those as they weren't in the scope of feature I was working on but I would guess that's the case. I will work on getting a PR in place within the next few days.

[tillkruss]

Thanks!

I looks like ZSCAN and SSCAN actually refer to HSCAN, but just SCAN might be different?

• http://squizzle.me/php/predis/doc/Commands#hscan
• https://github.com/nrk/predis/blob/v1.0/examples/redis_collections_iterators.php

[ragingdave]

I have fixes in place for all the scan methods, which I'll be polishing and pushing up, but have run into a few snags. PHPRedis scan apparently doesn't automatically prefix scope the scan command I will have to do some more testing but this could also apply to the other scans (z, s, and h). Is this something that should be handled in this PR, or should it be worked to be resolved in PHPRedis directly. For reference, phpredis/phpredis#548. Otherwise until that is resolved on PHPredis, I'm not sure if this issue can even move forward.

[tillkruss]

Great to hear. Whatever gets us the closest experience to Predis we can handle internally. if PhpRedis gets an update, we can always adjust.

[ragingdave]

So in further testing, I might have uncovered an additonal bug that was previously untested....or maybe is expected, but based on how the unit test is wired (that predis and phpredis and then prefixed phpredis all have the same assertions) I would expect each output to be identical.

For example,
seeding redis in a single test without flushing the database between connection changes to ensure that there are entries that would better test the scope between prefixed/un-prefixed scans:
$redis->set('jeffrey', 1); $redis->set('matt', 5); $redis->set('matthew', 6); $redis->set('taylor', 10); $redis->set('dave', 12);
And then running a local mod that adds the prefixed version of predis to the tested connections, both the predis and phpredis connections with a prefix return nothing when using pretty much any scan with a match condion. This is slightly unexpected as I would assume that scans would run in the specific prefix they are configured for. For reference the normal un-prefixed connections return the expected results:
un-prefixed connections:
MATCH => matt* returns both matt and matthew
MATCH => matt returns only matt

prefixed connections:
MATCH => matt* returns no results
MATCH =>matt returns no results

I do have fixes ready, but I don't just want to break unit tests or have incorrect assumptions about the fix in trying to solve something.

My main issue is whether this is a bug or intended functionality for the prefixed connections.

[tillkruss]

Puh, yeah I'd assume the same, if the connection has a prefix, it should add it to the scan. I'd suggest you break up the PRs, one for fixing the SCAN methods, and one for prefix fixes. That way Taylor can decide whether or not to merge the prefix changes.

[ragingdave]

Would altering the assertions based on if the connection is prefixed be an acceptable bypass at this point to at least have the functions fixed and the unit tests pass? Either that or I could most likely just pass to the connections() method a param whether or not to add the prefixed connections. I'm guessing that committing default failed tests isn't the right way to handle this...

[tillkruss]

I'd test two separate connections.