From 7e562e75dfad806ef4d7ee53a8dd6a8fcfbd930e Mon Sep 17 00:00:00 2001 From: michael-grunder Date: Thu, 13 Aug 2026 17:00:37 -0700 Subject: [PATCH 1/5] ci: Switch from `KeyDB` to `Dragonfly` in CI KeyDB is no longer under active development but Dragonfly is so it makes more sense to test against Dragonfly instead. --- .github/workflows/ci.yml | 90 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 84 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 99c3b6362c..eff81e9406 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -74,7 +74,7 @@ jobs: fail-fast: false matrix: php: ['7.4', '8.0', '8.1', '8.2', '8.3', '8.4', '8.5'] - server: ['redis', 'valkey'] + server: ['redis', 'dragonfly', 'valkey'] steps: - name: Checkout @@ -106,6 +106,16 @@ jobs: sudo apt-get update --allow-releaseinfo-change-label sudo apt-get install redis + - name: Install Dragonfly + if: matrix.server == 'dragonfly' + run: | + sudo curl -Lo /usr/share/keyrings/dragonfly-keyring.public \ + https://packages.dragonflydb.io/pgp-key.public + sudo curl -Lo /etc/apt/sources.list.d/dragonfly.sources \ + https://packages.dragonflydb.io/dragonfly.sources + sudo apt-get update --allow-releaseinfo-change-label + sudo apt-get install dragonfly redis-server redis-tools + - name: Install ValKey if: matrix.server == 'valkey' run: | @@ -126,9 +136,15 @@ jobs: echo 'extension = redis.so' | sudo tee -a "$(php-config --ini-dir)"/90-redis.ini - name: Attempt to shutdown default server - run: ${{ matrix.server }}-cli SHUTDOWN NOSAVE || true + run: | + if [ "${{ matrix.server }}" = dragonfly ]; then + sudo systemctl stop dragonfly redis-server || true + else + ${{ matrix.server }}-cli SHUTDOWN NOSAVE || true + fi - name: Start ${{ matrix.server }}-server + if: matrix.server != 'dragonfly' run: | for PORT in {6379..6382} {32767..32769}; do ${{ matrix.server }}-server \ @@ -144,7 +160,35 @@ jobs: --aclfile tests/users.acl \ --acl-pubsub-default allchannels + - name: Start dragonfly + if: matrix.server == 'dragonfly' + run: | + for PORT in {6379..6382} {32767..32769}; do + mkdir -p "/tmp/dragonfly-data-$PORT" + dragonfly \ + --port="$PORT" \ + --dir="/tmp/dragonfly-data-$PORT" \ + --aclfile=tests/users.acl \ + --proactor_threads=1 \ + --maxmemory=256mb \ + --maxclients=1000 \ + --version_check=false \ + >"/tmp/dragonfly-$PORT.log" 2>&1 & + done + mkdir -p /tmp/dragonfly-data-unixsocket + dragonfly \ + --port=0 \ + --unixsocket=/tmp/redis.sock \ + --dir=/tmp/dragonfly-data-unixsocket \ + --aclfile=tests/users.acl \ + --proactor_threads=1 \ + --maxmemory=256mb \ + --maxclients=1000 \ + --version_check=false \ + >/tmp/dragonfly-unixsocket.log 2>&1 & + - name: Start ${{ matrix.server }} cluster + if: matrix.server != 'dragonfly' run: | mkdir -p tests/nodes echo -n > tests/nodes/nodemap @@ -159,13 +203,34 @@ jobs: echo 127.0.0.1:"$PORT" >> tests/nodes/nodemap done + - name: Start dragonfly cluster + if: matrix.server == 'dragonfly' + run: | + mkdir -p tests/nodes + mkdir -p /tmp/dragonfly-data-cluster + echo 127.0.0.1:7000 > tests/nodes/nodemap + dragonfly \ + --port=7000 \ + --cluster_mode=emulated \ + --dir=/tmp/dragonfly-data-cluster \ + --aclfile=tests/users.acl \ + --proactor_threads=1 \ + --maxmemory=256mb \ + --maxclients=1000 \ + --version_check=false \ + >/tmp/dragonfly-cluster.log 2>&1 & + - name: Start ${{ matrix.server }} sentinel run: | wget raw.githubusercontent.com/redis/redis/7.0/sentinel.conf + SERVER=${{ matrix.server }}-server + if [ "${{ matrix.server }}" = dragonfly ]; then + SERVER=redis-server + fi for PORT in {26379..26380}; do cp sentinel.conf "$PORT.conf" sed -i '/^sentinel/Id' "$PORT.conf" - ${{ matrix.server }}-server "$PORT.conf" \ + "$SERVER" "$PORT.conf" \ --port "$PORT" \ --daemonize yes \ --sentinel monitor mymaster 127.0.0.1 6379 1 \ @@ -175,12 +240,21 @@ jobs: - name: Wait for ${{ matrix.server }} instances run: | WAIT_TIMEOUT_SECONDS=30 + CLI=${{ matrix.server }}-cli + CLUSTER_PORTS=({7000..7005}) + if [ "${{ matrix.server }}" = dragonfly ]; then + CLI=redis-cli + CLUSTER_PORTS=(7000) + fi - for PORT in {6379..6382} {7000..7005} {32767..32768} {26379..26380}; do + for PORT in {6379..6382} "${CLUSTER_PORTS[@]}" {32767..32768} {26379..26380}; do START_TIME=$SECONDS - until echo PING | ${{ matrix.server }}-cli -p "$PORT" 2>&1 | grep -qE 'PONG|NOAUTH'; do + until echo PING | "$CLI" -p "$PORT" 2>&1 | grep -qE 'PONG|NOAUTH'; do if (( SECONDS - START_TIME >= WAIT_TIMEOUT_SECONDS )); then echo "Timed out waiting for ${{ matrix.server }} on port $PORT after ${WAIT_TIMEOUT_SECONDS}s" + if [ "${{ matrix.server }}" = dragonfly ]; then + cat /tmp/dragonfly*.log + fi exit 1 fi echo "Still waiting for ${{ matrix.server }} on port $PORT" @@ -188,9 +262,12 @@ jobs: done done START_TIME=$SECONDS - until echo PING | ${{ matrix.server }}-cli -s /tmp/redis.sock 2>&1 | grep -qE 'PONG|NOAUTH'; do + until echo PING | "$CLI" -s /tmp/redis.sock 2>&1 | grep -qE 'PONG|NOAUTH'; do if (( SECONDS - START_TIME >= WAIT_TIMEOUT_SECONDS )); then echo "Timed out waiting for ${{ matrix.server }} at /tmp/redis.sock after ${WAIT_TIMEOUT_SECONDS}s" + if [ "${{ matrix.server }}" = dragonfly ]; then + cat /tmp/dragonfly*.log + fi exit 1 fi echo "Still waiting for ${{ matrix.server }} at /tmp/redis.sock" @@ -198,6 +275,7 @@ jobs: done - name: Initialize ${{ matrix.server }} cluster + if: matrix.server != 'dragonfly' run: | echo yes | ${{ matrix.server }}-cli --cluster create 127.0.0.1:{7000..7005} \ --cluster-replicas 1 --user phpredis -a phpredis From 615cab5057d99cecb6c60d35f32f6507703fed29 Mon Sep 17 00:00:00 2001 From: michael-grunder Date: Thu, 13 Aug 2026 17:19:00 -0700 Subject: [PATCH 2/5] ci: Get the test suite passing with Dragonfly There are a few edge cases where Dragonfly has slightly different functionality or returns a slightly different RESP shape. I'll open an issue with them to see whether they want to be more compatible. In the meantime they're pretty rare edge cases so we can handle them here so we can run the vast majority of tests against Dragonfly now. --- tests/RedisClusterTest.php | 3 + tests/RedisTest.php | 113 ++++++++++++++++++++++++++----------- tests/TestSuite.php | 1 + 3 files changed, 85 insertions(+), 32 deletions(-) diff --git a/tests/RedisClusterTest.php b/tests/RedisClusterTest.php index aad40acd5b..426d5d4150 100644 --- a/tests/RedisClusterTest.php +++ b/tests/RedisClusterTest.php @@ -566,6 +566,9 @@ public function testInfoCommandStats() { $this->assertIsArray($info); if (is_array($info)) { foreach($info as $k => $value) { + if ($this->is_dragonfly && strpos($k, 'unknown_') === 0) + continue; + $this->assertStringContains('cmdstat_', $k); } } diff --git a/tests/RedisTest.php b/tests/RedisTest.php index 1b3998e137..ea2acd5411 100644 --- a/tests/RedisTest.php +++ b/tests/RedisTest.php @@ -69,6 +69,11 @@ protected function detectKeyDB($info) { isset($info['keydb']) || isset($info['mvcc_depth']); } + protected function detectDragonfly($info) { + return is_array($info) && (isset($info['dragonfly_version']) || + ($info['executable'] ?? '') === 'dragonfly'); + } + protected function detectValkey($info) { return is_array($info) && (($info['server_name'] ?? NULL) === 'valkey' || @@ -98,6 +103,7 @@ public function setUp() { $this->version = $info['redis_version'] ?? '0.0.0'; $this->valkey_version = $info['valkey_version'] ?? '0.0.0'; + $this->is_dragonfly = $this->detectDragonfly($info); $this->is_keydb = $this->detectKeyDB($info); $this->is_valkey = $this->detectValKey($info); } @@ -355,7 +361,7 @@ public function testBitsets() { } public function testLcs() { - if ( ! $this->minVersionCheck('7.0.0') || $this->is_keydb) + if ( ! $this->minVersionCheck('7.0.0') || $this->is_keydb || $this->is_dragonfly) $this->markTestSkipped(); $key1 = '{lcs}1'; $key2 = '{lcs}2'; @@ -920,6 +926,8 @@ public function testSetNX() { public function testExpireAtWithLong() { if (PHP_INT_SIZE != 8) $this->markTestSkipped('64 bits only'); + if ($this->is_dragonfly) + $this->markTestSkipped('Dragonfly caps expiry values at 2^28 - 1 seconds'); $large_expiry = 3153600000; $this->redis->del('key'); @@ -1048,6 +1056,12 @@ public function testTouch() { $this->redis->del('notakey'); $this->assertTrue($this->redis->mset(['{idle}1' => 'beep', '{idle}2' => 'boop'])); + + if ($this->is_dragonfly) { + $this->assertEquals(2, $this->redis->touch('{idle}1', '{idle}2', '{idle}notakey')); + return; + } + usleep(1100000); $this->assertGT(0, $this->redis->object('idletime', '{idle}1')); $this->assertGT(0, $this->redis->object('idletime', '{idle}2')); @@ -2552,6 +2566,9 @@ public function testClient() { if (version_compare($this->version, '5.0.0') >= 0) { $this->assertGT(0, $this->redis->client('id')); + if ($this->is_dragonfly) + return; + if (version_compare($this->version, '6.0.0') >= 0) { $this->assertEquals(-1, $this->redis->client('getredir')); $this->assertTrue($this->redis->client('tracking', 'on', ['optin' => true])); @@ -2590,7 +2607,7 @@ public function testSlowlog() { public function testWait() { // Closest we can check based on redis commit history - if (version_compare($this->version, '2.9.11') < 0) + if (version_compare($this->version, '2.9.11') < 0 || ! $this->haveCommand('WAIT')) $this->markTestSkipped(); // We could have slaves here, so determine that @@ -2640,18 +2657,20 @@ public function testInfo() { 'total_commands_processed', 'role' ]; - if (version_compare($this->version, '2.5.0') < 0) { - array_push($keys, - 'changes_since_last_save', - 'bgsave_in_progress', - 'last_save_time' - ); - } else { - array_push($keys, - 'rdb_changes_since_last_save', - 'rdb_bgsave_in_progress', - 'rdb_last_save_time' - ); + if ( ! $this->is_dragonfly) { + if (version_compare($this->version, '2.5.0') < 0) { + array_push($keys, + 'changes_since_last_save', + 'bgsave_in_progress', + 'last_save_time' + ); + } else { + array_push($keys, + 'rdb_changes_since_last_save', + 'rdb_bgsave_in_progress', + 'rdb_last_save_time' + ); + } } foreach ($keys as $k) { @@ -2692,7 +2711,9 @@ public function testServerInfo() { return false; } - $this->assertEquals($hello['server'], $this->redis->serverName()); + $server = $this->is_dragonfly ? 'dragonfly' : $hello['server']; + + $this->assertEquals($server, $this->redis->serverName()); $this->assertEquals($hello['version'], $this->redis->serverVersion()); $info = $this->redis->info(); @@ -2700,7 +2721,7 @@ public function testServerInfo() { $cmd1 = $info['total_commands_processed']; /* Shouldn't hit the server */ - $this->assertEquals($hello['server'], $this->redis->serverName()); + $this->assertEquals($server, $this->redis->serverName()); $this->assertEquals($hello['version'], $this->redis->serverVersion()); $info = $this->redis->info(); @@ -2727,6 +2748,9 @@ public function testInfoCommandStats() { return; foreach ($info as $k => $value) { + if ($this->is_dragonfly && strpos($k, 'unknown_') === 0) + continue; + $this->assertStringContains('cmdstat_', $k); } } @@ -2737,7 +2761,7 @@ public function testSelect() { } public function testSwapDB() { - if (version_compare($this->version, '4.0.0') < 0) + if (version_compare($this->version, '4.0.0') < 0 || ! $this->haveCommand('SWAPDB')) $this->markTestSkipped(); $this->assertTrue($this->redis->swapdb(0, 1)); @@ -3485,6 +3509,10 @@ public function testZRandMember() { $this->MarkTestSkipped(); return; } + if ($this->is_dragonfly) { + $this->MarkTestSkipped('Dragonfly returns an array when COUNT is omitted'); + return; + } $this->redis->del('key'); $this->redis->zAdd('key', 0, 'a', 1, 'b', 2, 'c', 3, 'd', 4, 'e'); $this->assertInArray($this->redis->zRandMember('key'), ['a', 'b', 'c', 'd', 'e']); @@ -3746,6 +3774,9 @@ public function testSetRange() { } public function testObject() { + if ($this->is_dragonfly) + $this->markTestSkipped(); + /* Version 3.0.0 (represented as >= 2.9.0 in redis info) and moving * forward uses 'embstr' instead of 'raw' for small string values */ if (version_compare($this->version, '2.9.0') < 0) { @@ -5939,7 +5970,8 @@ public function testDumpRestore() { /* Ensure we can set an IDLETIME */ $this->assertTrue($this->redis->restore('foo', 0, $d_bar, ['REPLACE', 'IDLETIME' => 200])); - $this->assertGT(100, $this->redis->object('idletime', 'foo')); + if ( ! $this->is_dragonfly) + $this->assertGT(100, $this->redis->object('idletime', 'foo')); /* We can't neccissarily check this depending on LRU policy, but at least attempt to use the FREQ option */ @@ -6069,17 +6101,25 @@ public function testEval() { $nested_script = " return { 1,2,3, { - redis.call('get', '{eval-key}-str1'), - redis.call('get', '{eval-key}-str2'), - redis.call('lrange', 'not-any-kind-of-list', 0, -1), + redis.call('get', KEYS[1]), + redis.call('get', KEYS[2]), + redis.call('lrange', KEYS[3], 0, -1), { - redis.call('zrange', '{eval-key}-zset', 0, -1), - redis.call('lrange', '{eval-key}-list', 0, -1) + redis.call('zrange', KEYS[4], 0, -1), + redis.call('lrange', KEYS[5], 0, -1) } } } "; + $nested_args = [ + '{eval-key}-str1', + '{eval-key}-str2', + '{eval-key}-nolist', + '{eval-key}-zset', + '{eval-key}-list', + ]; + $expected = [ 1, 2, 3, [ 'hello, world', @@ -6093,7 +6133,7 @@ public function testEval() { ]; // Now run our script, and check our values against each other - $eval_result = $this->redis->eval($nested_script, ['{eval-key}-str1', '{eval-key}-str2', '{eval-key}-zset', '{eval-key}-list'], 4); + $eval_result = $this->redis->eval($nested_script, $nested_args, count($nested_args)); $this->assertTrue( is_array($eval_result) && count($this->array_diff_recursive($eval_result, $expected)) == 0 @@ -6111,7 +6151,7 @@ public function testEval() { foreach ($modes as $mode) { $this->redis->multi($mode); for ($i = 0; $i < $num_scripts; $i++) { - $this->redis->eval($nested_script, ['{eval-key}-dummy'], 1); + $this->redis->eval($nested_script, $nested_args, count($nested_args)); } $replies = $this->redis->exec(); @@ -6456,14 +6496,14 @@ public function testPrefix() { public function testReplyLiteral() { $this->redis->setOption(Redis::OPT_REPLY_LITERAL, false); $this->assertTrue($this->redis->rawCommand('set', 'foo', 'bar')); - $this->assertTrue($this->redis->eval("return redis.call('set', 'foo', 'bar')", [], 0)); + $this->assertTrue($this->redis->eval("return redis.call('set', KEYS[1], 'bar')", ['foo'], 1)); $rv = $this->redis->eval("return {redis.call('set', KEYS[1], 'bar'), redis.call('ping')}", ['foo'], 1); $this->assertEquals([true, true], $rv); $this->redis->setOption(Redis::OPT_REPLY_LITERAL, true); $this->assertEquals('OK', $this->redis->rawCommand('set', 'foo', 'bar')); - $this->assertEquals('OK', $this->redis->eval("return redis.call('set', 'foo', 'bar')", [], 0)); + $this->assertEquals('OK', $this->redis->eval("return redis.call('set', KEYS[1], 'bar')", ['foo'], 1)); // Nested $rv = $this->redis->eval("return {redis.call('set', KEYS[1], 'bar'), redis.call('ping')}", ['foo'], 1); @@ -6546,6 +6586,9 @@ public function testConfig() { if ( ! $this->minVersionCheck('7.0.0')) return; + if ($this->is_dragonfly) + return; + /* Test getting multiple values */ $settings = $this->redis->config('get', ['timeout', 'databases', 'set-max-intset-entries']); $this->assertTrue(is_array($settings) && isset($settings['timeout']) && @@ -7534,7 +7577,7 @@ public function testGeoSearchStoreByPolygon() { } public function testGeoSearchStore() { - if ( ! $this->minVersionCheck('6.2.0')) + if ( ! $this->minVersionCheck('6.2.0') || ! $this->haveCommand('GEOSEARCHSTORE')) $this->markTestSkipped(); $this->addCities('{gk}src'); @@ -7731,6 +7774,9 @@ public function testXGroup() { if ( ! $this->minVersionCheck('7.0.0')) return; + if ($this->is_dragonfly) + return; + /* ENTRIESREAD */ $this->assertEquals(1, $this->redis->del('s')); $this->assertTrue($this->redis->xGroup('create', 's', 'mygroup', '$', true, 1337)); @@ -8549,7 +8595,7 @@ public function testInvalidAuthArgs() { } public function testAcl() { - if ( ! $this->minVersionCheck('6.0')) + if ( ! $this->minVersionCheck('6.0') || $this->is_dragonfly) $this->markTestSkipped(); /* ACL USERS/SETUSER */ @@ -9177,7 +9223,7 @@ public function testTlsConnect() { } public function testReset() { - if (version_compare($this->version, '6.2.0') < 0) + if (version_compare($this->version, '6.2.0') < 0 || ! $this->haveCommand('RESET')) $this->markTestSkipped(); $this->assertTrue($this->redis->multi()->select(2)->set('foo', 'bar')->reset()); @@ -9207,6 +9253,9 @@ public function testCommand() { $this->assertIsArray($commands); $this->assertEquals(count($commands), $this->redis->command('count')); + if ($this->is_dragonfly) + return; + if ( ! $this->is_keydb && $this->minVersionCheck('7.0')) { $infos = $this->redis->command('info'); $this->assertIsArray($infos); @@ -9232,7 +9281,7 @@ public function testCommand() { } public function testFunction() { - if (version_compare($this->version, '7.0') < 0) + if (version_compare($this->version, '7.0') < 0 || ! $this->haveCommand('FUNCTION')) $this->markTestSkipped(); $this->assertTrue($this->redis->function('flush', 'sync')); @@ -9253,7 +9302,7 @@ protected function execWaitAOF() { } public function testWaitAOF() { - if ( ! $this->minVersionCheck('7.2.0')) + if ( ! $this->minVersionCheck('7.2.0') || ! $this->haveCommand('WAITAOF')) $this->markTestSkipped(); $res = $this->execWaitAOF(); diff --git a/tests/TestSuite.php b/tests/TestSuite.php index 845de85e18..eb13ca5a7b 100644 --- a/tests/TestSuite.php +++ b/tests/TestSuite.php @@ -17,6 +17,7 @@ class TestSuite /* Redis server version */ protected $version; protected string $valkey_version; + protected bool $is_dragonfly; protected bool $is_keydb; protected bool $is_valkey; From 866c76a360db48c3aac8f595f667664224b56e8e Mon Sep 17 00:00:00 2001 From: michael-grunder Date: Tue, 18 Aug 2026 12:32:03 -0700 Subject: [PATCH 3/5] fix: Update reply handler to handle embedded integers We've got a generic multibulk handler that didn't handle elements that were non-strings. This seems like a bug in general but also fixes it so that our reply handler works for `TIME` in both `Redis` and `Dragonfly` --- library.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/library.c b/library.c index 9a7a85687f..ff5ece1822 100644 --- a/library.c +++ b/library.c @@ -3621,7 +3621,12 @@ redis_mbulk_reply_raw(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, ZVAL_EMPTY_ARRAY(&z_multi_result); } else { array_init_size(&z_multi_result, numElems); /* pre-allocate array for multi's results. */ - redis_mbulk_reply_loop(redis_sock, &z_multi_result, numElems, UNSERIALIZE_NONE); + if (redis_mbulk_reply_loop(redis_sock, &z_multi_result, numElems, + UNSERIALIZE_NONE) == FAILURE) { + zval_ptr_dtor_nogc(&z_multi_result); + REDIS_RESPONSE_ERROR(redis_sock, z_tab); + return FAILURE; + } } REDIS_RETURN_ZVAL(redis_sock, z_tab, z_multi_result); @@ -3712,7 +3717,7 @@ redis_sock_read_bulk_zstr(RedisSock *redis_sock, int bytes) * no intermediate copy. Returns NULL for null/error replies, mirroring * redis_sock_read's NULL semantics. */ static zend_string * -redis_sock_read_zstr(RedisSock *redis_sock) +redis_sock_read_zstr(RedisSock *redis_sock, REDIS_REPLY_TYPE *reply_type) { char inbuf[4096]; size_t len; @@ -3721,7 +3726,9 @@ redis_sock_read_zstr(RedisSock *redis_sock) return NULL; } - switch (inbuf[0]) { + *reply_type = inbuf[0]; + + switch (*reply_type) { case '-': redis_sock_set_err(redis_sock, inbuf + 1, len - 1); redis_error_throw(redis_sock); @@ -3746,13 +3753,15 @@ redis_sock_read_zstr(RedisSock *redis_sock) if (len > 2 && memcmp(inbuf + 1, "-1", 2) == 0) { return NULL; } + if (len > 1) { + return zend_string_init(inbuf, len, 0); + } REDIS_FALLTHROUGH; case '+': case ':': - /* Single line reply (+OK or :123), kept verbatim like - * redis_sock_read does, including the leading type byte. */ + /* Materialize inline values without their RESP type byte. */ if (len > 1) { - return zend_string_init(inbuf, len, 0); + return zend_string_init(inbuf + 1, len - 1, 0); } REDIS_FALLTHROUGH; default: @@ -3769,6 +3778,7 @@ PHP_REDIS_API int redis_mbulk_reply_loop(RedisSock *redis_sock, zval *z_tab, int count, int unserialize) { + REDIS_REPLY_TYPE reply_type; zval z_value; zend_string *zstr; int i; @@ -3780,7 +3790,7 @@ redis_mbulk_reply_loop(RedisSock *redis_sock, zval *z_tab, int count, redis_sock->compression != REDIS_COMPRESSION_NONE; for (i = 0; i < count; ++i) { - if ((zstr = redis_sock_read_zstr(redis_sock)) == NULL) { + if ((zstr = redis_sock_read_zstr(redis_sock, &reply_type)) == NULL) { add_next_index_bool(z_tab, 0); if (EG(exception) || redis_sock->stream == NULL || redis_sock->status == REDIS_SOCK_STATUS_FAILED @@ -3799,7 +3809,7 @@ redis_mbulk_reply_loop(RedisSock *redis_sock, zval *z_tab, int count, (unserialize == UNSERIALIZE_VALS && i % 2 != 0) ); - if (unwrap && repack) { + if (unwrap && repack && reply_type == TYPE_BULK) { redis_unpack(redis_sock, ZSTR_VAL(zstr), ZSTR_LEN(zstr), &z_value); zend_string_release(zstr); } else { From ebdb75889f64d3686477de7715347501a7dd3a89 Mon Sep 17 00:00:00 2001 From: michael-grunder Date: Tue, 18 Aug 2026 12:53:42 -0700 Subject: [PATCH 4/5] fix: Improve GEOSEARCH reply handling This fixes a test failure when running against Dragonfly but is also an improvement over just using PHPREDIS_CTX_PTR variants to figure out what the reply handler should do. --- cluster_library.c | 2 +- common.h | 1 - library.c | 112 ++++++++++++++++++++++++++++++++------------ library.h | 9 +++- redis_commands.c | 12 +++-- tests/RedisTest.php | 6 +++ 6 files changed, 106 insertions(+), 36 deletions(-) diff --git a/cluster_library.c b/cluster_library.c index 72cc3798b8..2175f60163 100644 --- a/cluster_library.c +++ b/cluster_library.c @@ -1918,7 +1918,7 @@ cluster_geosearch_resp(INTERNAL_FUNCTION_PARAMETERS, redisCluster *c, c->cmd_sock->null_mbulk_as_null = c->flags->null_mbulk_as_null; if (c->reply_type != TYPE_MULTIBULK || redis_read_geosearch_response(&zret, c->cmd_sock, c->reply_len, - ctx.mode == REDIS_CTX_GEO_WITHMETA) < 0) + (uintptr_t)ctx.ptr) < 0) { ZVAL_FALSE(&zret); } diff --git a/common.h b/common.h index 4d0fffd9f8..81d58d67d9 100644 --- a/common.h +++ b/common.h @@ -327,7 +327,6 @@ typedef enum RedisCtxMode { REDIS_CTX_WITHSCORES, REDIS_CTX_WITHVALUES, REDIS_CTX_INCR, - REDIS_CTX_GEO_WITHMETA, REDIS_CTX_XAUTOCLAIM, REDIS_CTX_DECODE_JSON, REDIS_CTX_HELLO_SERVER, diff --git a/library.c b/library.c index ff5ece1822..62f2f41efe 100644 --- a/library.c +++ b/library.c @@ -1664,14 +1664,78 @@ redis_mbulk_reply_zipped(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, return 0; } +static int geosearch_cast_double(zval *zv) +{ + if (Z_TYPE_P(zv) == IS_ARRAY) + return FAILURE; + + convert_to_double(zv); + + return SUCCESS; +} + static int -geosearch_cast(zval *zv) +geosearch_cast(zval *zv, uintptr_t options) { - if (Z_TYPE_P(zv) == IS_ARRAY) { - zend_hash_apply(Z_ARRVAL_P(zv), geosearch_cast); - } else if (Z_TYPE_P(zv) == IS_STRING) { - convert_to_double(zv); + zval *zcoord, *zsub; + HashTable *ht; + zend_long hash; + zend_ulong idx = 1; + uint32_t expected = 1; + + expected += (options & REDIS_GEOSEARCH_WITHDIST) != 0; + expected += (options & REDIS_GEOSEARCH_WITHHASH) != 0; + expected += (options & REDIS_GEOSEARCH_WITHCOORD) != 0; + + if (Z_TYPE_P(zv) != IS_ARRAY || + zend_hash_num_elements(Z_ARRVAL_P(zv)) != expected) + { + return FAILURE; + } + + ht = Z_ARRVAL_P(zv); + zsub = zend_hash_index_find(ht, 0); + if (zsub == NULL || Z_TYPE_P(zsub) == IS_ARRAY) + return FAILURE; + + if (options & REDIS_GEOSEARCH_WITHDIST) { + zsub = zend_hash_index_find(ht, idx++); + if (zsub == NULL || geosearch_cast_double(zsub) == FAILURE) + return FAILURE; + } + + if (options & REDIS_GEOSEARCH_WITHHASH) { + zsub = zend_hash_index_find(ht, idx++); + if (zsub == NULL) + return FAILURE; + + if (Z_TYPE_P(zsub) == IS_STRING) { + if (is_numeric_string(Z_STRVAL_P(zsub), Z_STRLEN_P(zsub), + &hash, NULL, 0) != IS_LONG) + { + return FAILURE; + } + zval_ptr_dtor_nogc(zsub); + ZVAL_LONG(zsub, hash); + } else if (Z_TYPE_P(zsub) != IS_LONG) { + return FAILURE; + } } + + if (options & REDIS_GEOSEARCH_WITHCOORD) { + zsub = zend_hash_index_find(ht, idx); + if (zsub == NULL || Z_TYPE_P(zsub) != IS_ARRAY || + zend_hash_num_elements(Z_ARRVAL_P(zsub)) != 2) + { + return FAILURE; + } + + ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(zsub), zcoord) { + if (geosearch_cast_double(zcoord) == FAILURE) + return FAILURE; + } ZEND_HASH_FOREACH_END(); + } + return SUCCESS; } @@ -1757,11 +1821,10 @@ redis_mpop_response(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, PHP_REDIS_API int redis_read_geosearch_response(zval *zdst, RedisSock *redis_sock, - long long elements, int with_aux_data) + long long elements, uintptr_t options) { zval z_multi_result, z_sub, *z_ele, *zv; zend_string *zkey; - int status = SUCCESS; /* Handle the trivial "empty" result first */ if (elements < 0 && redis_sock->null_mbulk_as_null) { @@ -1771,7 +1834,7 @@ redis_read_geosearch_response(zval *zdst, RedisSock *redis_sock, array_init_size(zdst, elements > 0 ? elements : 0); - if (with_aux_data == 0) { + if (options == 0) { redis_mbulk_reply_loop(redis_sock, zdst, elements, UNSERIALIZE_NONE); } else { array_init_size(&z_multi_result, elements > 0 ? elements : 0); @@ -1779,30 +1842,19 @@ redis_read_geosearch_response(zval *zdst, RedisSock *redis_sock, if (redis_read_multibulk_recursive(redis_sock, elements, 0, &z_multi_result) == FAILURE) { - status = FAILURE; + goto fail; } ZEND_HASH_FOREACH_VAL(Z_ARRVAL(z_multi_result), z_ele) { - // The first item in the sub-array is always the name of the returned item - if (Z_TYPE_P(z_ele) != IS_ARRAY) { - status = FAILURE; - break; - } + if (geosearch_cast(z_ele, options) == FAILURE) + goto fail; + // The first item in the sub-array is always the name of the returned item zv = zend_hash_index_find(Z_ARRVAL_P(z_ele), 0); - if (zv == NULL) { - status = FAILURE; - break; - } - zkey = zval_get_string(zv); zend_hash_index_del(Z_ARRVAL_P(z_ele), 0); - // The other information is returned in the following order as successive - // elements of the sub-array: distance, geohash, coordinates - zend_hash_apply(Z_ARRVAL_P(z_ele), geosearch_cast); - // Reindex elements so they start at zero */ ZVAL_ARR(&z_sub, zend_array_to_list(Z_ARRVAL_P(z_ele))); @@ -1812,14 +1864,14 @@ redis_read_geosearch_response(zval *zdst, RedisSock *redis_sock, // Cleanup zval_ptr_dtor_nogc(&z_multi_result); - - if (status == FAILURE) { - zval_ptr_dtor_nogc(zdst); - ZVAL_UNDEF(zdst); - } } - return status; + return SUCCESS; + +fail: + zval_ptr_dtor_nogc(&z_multi_result); + zval_ptr_dtor_nogc(zdst); + return FAILURE; } PHP_REDIS_API int @@ -1831,7 +1883,7 @@ redis_geosearch_response(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, if (read_mbulk_header(redis_sock, &elements) < 0 || redis_read_geosearch_response(&zret, redis_sock, elements, - ctx.mode == REDIS_CTX_GEO_WITHMETA) < 0) + (uintptr_t)ctx.ptr) < 0) { ZVAL_FALSE(&zret); } diff --git a/library.h b/library.h index 13531b46ae..3c2a70de83 100644 --- a/library.h +++ b/library.h @@ -10,6 +10,12 @@ #define CLUSTER_THROW_EXCEPTION(msg, code) \ zend_throw_exception(redis_cluster_exception_ce, (msg), code) +typedef enum { + REDIS_GEOSEARCH_WITHCOORD = 1 << 0, + REDIS_GEOSEARCH_WITHDIST = 1 << 1, + REDIS_GEOSEARCH_WITHHASH = 1 << 2, +} RedisGeoSearchOptions; + #if PHP_VERSION_ID < 80000 /* use RedisException when ValueError not available */ #define REDIS_VALUE_EXCEPTION(m) REDIS_THROW_EXCEPTION(m, 0) @@ -193,7 +199,8 @@ PHP_REDIS_API int redis_zadd_response(INTERNAL_FUNCTION_PARAMETERS, RedisSock *r PHP_REDIS_API int redis_zrandmember_response(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, zval *z_tab, RedisCmdCtx ctx); PHP_REDIS_API int redis_zdiff_response(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, zval *z_tab, RedisCmdCtx ctx); PHP_REDIS_API int redis_set_response(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, zval *z_tab, RedisCmdCtx ctx); -PHP_REDIS_API int redis_read_geosearch_response(zval *zdst, RedisSock *redis_sock, long long elements, int with_aux_data); +PHP_REDIS_API int redis_read_geosearch_response(zval *zdst, RedisSock *redis_sock, + long long elements, uintptr_t options); PHP_REDIS_API int redis_geosearch_response(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, zval *z_tab, RedisCmdCtx ctx); PHP_REDIS_API int redis_hrandfield_response(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, zval *z_tab, RedisCmdCtx ctx); PHP_REDIS_API int redis_pop_response(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, zval *z_tab, RedisCmdCtx ctx); diff --git a/redis_commands.c b/redis_commands.c index fa15e7c3a0..09263bdf05 100644 --- a/redis_commands.c +++ b/redis_commands.c @@ -4554,6 +4554,7 @@ RedisCmd * redis_geosearch_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock) { zval *position, *shape, *z_ele; + uint64_t response_options = 0; geoSearchOptions sopts = {0}; zend_string *zkey, *zstr; zend_string *key, *unit; @@ -4628,9 +4629,14 @@ redis_geosearch_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock) redis_cmd_cat_literal_if(cmd, gopts.any, "ANY"); } - if (gopts.withcoord + gopts.withdist + gopts.withhash > 0) { - redis_cmd_set_ctx_mode(cmd, REDIS_CTX_GEO_WITHMETA); - } + if (gopts.withcoord) + response_options |= REDIS_GEOSEARCH_WITHCOORD; + if (gopts.withdist) + response_options |= REDIS_GEOSEARCH_WITHDIST; + if (gopts.withhash) + response_options |= REDIS_GEOSEARCH_WITHHASH; + + redis_cmd_set_ctx_u64(cmd, response_options); return cmd; } diff --git a/tests/RedisTest.php b/tests/RedisTest.php index ea2acd5411..e6bd9fa28b 100644 --- a/tests/RedisTest.php +++ b/tests/RedisTest.php @@ -7541,6 +7541,12 @@ public function testGeoSearch() { $this->addCities('gk'); $this->assertEquals(['Chico'], $this->redis->geosearch('gk', 'Chico', 1, 'm')); + $this->assertValidate($this->redis->geosearch('gk', 'Chico', 1, 'm', ['withhash']), function ($v) { + $this->assertArrayKey($v, 'Chico', 'is_array'); + $this->assertEquals(count($v['Chico']), 1); + $this->assertArrayKey($v['Chico'], 0, 'is_int'); + return true; + }); $this->assertValidate($this->redis->geosearch('gk', 'Chico', 1, 'm', ['withcoord', 'withdist', 'withhash']), function ($v) { $this->assertArrayKey($v, 'Chico', 'is_array'); $this->assertEquals(count($v['Chico']), 3); From db1540ab8f3f8782b66cfe828ab7d0ea42174d1a Mon Sep 17 00:00:00 2001 From: michael-grunder Date: Tue, 25 Aug 2026 11:25:51 -0700 Subject: [PATCH 5/5] tests: Ensure `testRandomkey` works in isolation Previously the test just iterated 1000 times getting a randomkey and then ensuring that the key existed. This will pretty much always work if you run the whole test suite, but running it in isolation on an empty dtabase would fail. ```php // Returns false $key = $redis->randomkey(); $this->assertKeyExists($key); // fails ``` --- tests/RedisTest.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/RedisTest.php b/tests/RedisTest.php index e6bd9fa28b..c3ccd187aa 100644 --- a/tests/RedisTest.php +++ b/tests/RedisTest.php @@ -706,8 +706,14 @@ public function testGetDel() { } public function testRandomKey() { - for ($i = 0; $i < 1000; $i++) { + /* Make sure we can run this test in isolation */ + for ($i = 0; $i < 10; $i++) { + $this->redis->set('{key}' . $i, 'val' . $i); + } + + for ($i = 0; $i < 10; $i++) { $k = $this->redis->randomKey(); + $this->assertIsString($k); $this->assertKeyExists($k); } }