Skip to content

Commit 598fece

Browse files
jduojduo
authored
Revert handling of command response errors to be return values instead of exceptions (#19)
Revert behavior for consistency with PHPRedis and update tests Signed-off-by: James Duong <[email protected]>
1 parent c5d9a05 commit 598fece

File tree

3 files changed

+49
-45
lines changed

3 files changed

+49
-45
lines changed

‎command_response.c‎

Lines changed: 32 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,6 @@
1515

1616
#include "command_response.h"
1717

18-
#include <zend_exceptions.h>
19-
2018
#include "ext/standard/php_var.h"
2119
#include "include/glide/command_request.pb-c.h"
2220
#include "include/glide/response.pb-c.h"
@@ -354,10 +352,10 @@ CommandResult* execute_command_with_route(const void* glide_client,
354352
if (!result) {
355353
printf("Error: Command execution returned NULL result\n");
356354
} else if (result->command_error) {
357-
zend_throw_exception(
358-
get_valkey_glide_exception_ce(), result->command_error->command_error_message, 0);
359-
free_command_result(result);
360-
return NULL;
355+
printf("Error: Command execution failed: %s\n",
356+
result->command_error->command_error_message
357+
? result->command_error->command_error_message
358+
: "Unknown error");
361359
}
362360

363361
return result;
@@ -386,12 +384,6 @@ CommandResult* execute_command(const void* glide_client,
386384
0 /* span pointer */
387385
);
388386

389-
if (result->command_error) {
390-
zend_throw_exception(
391-
get_valkey_glide_exception_ce(), result->command_error->command_error_message, 0);
392-
free_command_result(result);
393-
return NULL;
394-
}
395387
return result;
396388
}
397389

@@ -404,8 +396,10 @@ long handle_int_response(CommandResult* result, long* output_value) {
404396

405397
/* Check if there was an error */
406398
if (result->command_error) {
407-
zend_throw_exception(
408-
get_valkey_glide_exception_ce(), result->command_error->command_error_message, 0);
399+
printf("%s:%d - Error executing command: %s\n",
400+
__FILE__,
401+
__LINE__,
402+
result->command_error->command_error_message);
409403
free_command_result(result);
410404
return 0; /* False - failure */
411405
}
@@ -436,8 +430,10 @@ int handle_string_response(CommandResult* result, char** output, size_t* output_
436430

437431
/* Check if there was an error */
438432
if (result->command_error) {
439-
zend_throw_exception(
440-
get_valkey_glide_exception_ce(), result->command_error->command_error_message, 0);
433+
printf("%s:%d - Error executing command: %s\n",
434+
__FILE__,
435+
__LINE__,
436+
result->command_error->command_error_message);
441437
free_command_result(result);
442438
return -1;
443439
}
@@ -503,8 +499,10 @@ int handle_bool_response(CommandResult* result) {
503499

504500
/* Check if there was an error */
505501
if (result->command_error) {
506-
zend_throw_exception(
507-
get_valkey_glide_exception_ce(), result->command_error->command_error_message, 0);
502+
printf("%s:%d - Error executing command: %s\n",
503+
__FILE__,
504+
__LINE__,
505+
result->command_error->command_error_message);
508506
free_command_result(result);
509507
return -1;
510508
}
@@ -530,8 +528,10 @@ int handle_ok_response(CommandResult* result) {
530528

531529
/* Check if there was an error */
532530
if (result->command_error) {
533-
zend_throw_exception(
534-
get_valkey_glide_exception_ce(), result->command_error->command_error_message, 0);
531+
printf("%s:%d - Error executing command: %s\n",
532+
__FILE__,
533+
__LINE__,
534+
result->command_error->command_error_message);
535535
free_command_result(result);
536536
return -1;
537537
}
@@ -765,8 +765,10 @@ int handle_array_response(CommandResult* result, zval* output) {
765765

766766
/* Check if there was an error */
767767
if (result->command_error) {
768-
zend_throw_exception(
769-
get_valkey_glide_exception_ce(), result->command_error->command_error_message, 0);
768+
printf("%s:%d - Error executing command: %s\n",
769+
__FILE__,
770+
__LINE__,
771+
result->command_error->command_error_message);
770772
free_command_result(result);
771773
return -1;
772774
}
@@ -800,8 +802,10 @@ int handle_map_response(CommandResult* result, zval* output) {
800802

801803
/* Check if there was an error */
802804
if (result->command_error) {
803-
zend_throw_exception(
804-
get_valkey_glide_exception_ce(), result->command_error->command_error_message, 0);
805+
printf("%s:%d - Error executing command: %s\n",
806+
__FILE__,
807+
__LINE__,
808+
result->command_error->command_error_message);
805809
free_command_result(result);
806810
return -1;
807811
}
@@ -835,8 +839,10 @@ int handle_set_response(CommandResult* result, zval* output) {
835839

836840
/* Check if there was an error */
837841
if (result->command_error) {
838-
zend_throw_exception(
839-
get_valkey_glide_exception_ce(), result->command_error->command_error_message, 0);
842+
printf("%s:%d - Error executing command: %s\n",
843+
__FILE__,
844+
__LINE__,
845+
result->command_error->command_error_message);
840846
free_command_result(result);
841847
return -1;
842848
}

‎tests/TestSuite.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -882,7 +882,7 @@ public static function run(
882882
if ($e instanceof TestSkippedException) {
883883
$result = self::makeWarning('SKIPPED');
884884
} else {
885-
$class_name::$errors[] = "Uncaught exception '" . $e->getMessage() . "' ($name)\n";
885+
$class_name::$errors[] = "Uncaught exception '" . $e->getMessage() . "' ($name)\n" . $e->getTraceAsString() . "\n";
886886
$result = self::makeFail('FAILED');
887887
}
888888
}

‎tests/ValkeyGlideFeaturesTest.php‎

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -234,25 +234,23 @@ public function testConstructorWithRequestTimeoutExceeded()
234234
['host' => $this->getHost(), 'port' => $this->getPort()]
235235
];
236236

237-
try {
238-
if (!$this->getTLS()) {
239-
$valkey_glide = new ValkeyGlide($addresses, $this->getTLS(), null, ValkeyGlide::READ_FROM_PRIMARY, 10); // 10 milliseconds.
240-
} else {
241-
$advancedConfig = [
242-
'tls_config' => ['use_insecure_tls' => true]
243-
];
244-
$valkey_glide = new ValkeyGlide($addresses, use_tls: true, read_from: ValkeyGlide::READ_FROM_PRIMARY, request_timeout: 10, advanced_config: $advancedConfig);
245-
}
246-
$valkey_glide->rawcommand("DEBUG", "SLEEP", "2");
247-
$this->fail("Should have thrown a timeout exception.");
248-
} catch (Exception $e) {
249-
$this->assertStringContains("timed out", $e->getMessage(), "Exception should indicate authentication failure");
250-
} finally {
251-
// Sleep the test runner so that the server can finish the sleep command.
252-
sleep(2);
253-
// Clean up
254-
$valkey_glide?->close();
237+
if (!$this->getTLS()) {
238+
$valkey_glide = new ValkeyGlide($addresses, $this->getTLS(), null, ValkeyGlide::READ_FROM_PRIMARY, 10); // 10 milliseconds.
239+
} else {
240+
$advancedConfig = [
241+
'tls_config' => ['use_insecure_tls' => true]
242+
];
243+
$valkey_glide = new ValkeyGlide($addresses, use_tls: true, read_from: ValkeyGlide::READ_FROM_PRIMARY, request_timeout: 10, advanced_config: $advancedConfig);
255244
}
245+
$res = $valkey_glide->rawcommand("DEBUG", "SLEEP", "2");
246+
247+
// Sleep the test runner so that the server can finish the sleep command.
248+
sleep(2);
249+
// Clean up
250+
$valkey_glide?->close();
251+
252+
// Validate that the debug command failed.
253+
$this->assertFalse($res);
256254
}
257255

258256
public function testConstructorWithReconnectStrategy()

0 commit comments

Comments
 (0)