Merge branch 'hotfix/hmget_invalid_args'

Author: michael-grunder

redis.c

@@ -4918,93 +4918,92 @@ PHP_METHOD(Redis, hIncrBy)
 
 }
 
-
+/* {{{ array Redis::hMget(string hash, array keys) */
 PHP_METHOD(Redis, hMget) {
     zval *object;
     RedisSock *redis_sock;
-    char *key = NULL, *cmd;
-    int key_len, cmd_len, key_free;
-    zval *z_array;
-    zval **z_keys;
-    int nb_fields, i;
-	char *old_cmd = NULL;
-
-	zval **data;
-    HashTable *arr_hash;
-    HashPosition pointer;
+    char *key = NULL;
+    zval *z_array, **z_keys, **data;
+    int field_count, i, valid, key_len, key_free;
+    HashTable *ht_array;
+    HashPosition ptr;
+    smart_str cmd = {0};
 
-    if (zend_parse_method_parameters(ZEND_NUM_ARGS() TSRMLS_CC, getThis(), "Osa",
-                                     &object, redis_ce,
-                                     &key, &key_len, &z_array) == FAILURE) {
+    // Make sure we can grab our arguments properly
+    if(zend_parse_method_parameters(ZEND_NUM_ARGS() TSRMLS_CC, getThis(), "Osa",
+                                    &object, redis_ce, &key, &key_len, &z_array)
+                                    == FAILURE)
+    {
         RETURN_FALSE;
     }
 
-    if (redis_sock_get(object, &redis_sock TSRMLS_CC, 0) < 0) {
+    // We'll need our socket
+    if(redis_sock_get(object, &redis_sock TSRMLS_CC, 0) < 0) {
         RETURN_FALSE;
     }
-    nb_fields = zend_hash_num_elements(Z_ARRVAL_P(z_array));
 
-    if( nb_fields == 0) {
+    // Grab member count and abort if we don't have any
+    if((field_count = zend_hash_num_elements(Z_ARRVAL_P(z_array))) == 0) {
         RETURN_FALSE;
     }
 
-    z_keys = ecalloc(nb_fields, sizeof(zval *));
+    // Prefix our key if we need to
+    key_free = redis_key_prefix(redis_sock, &key, &key_len TSRMLS_CC);
 
-	key_free = redis_key_prefix(redis_sock, &key, &key_len TSRMLS_CC);
+    // Allocate enough memory for the number of keys being requested
+    z_keys = ecalloc(field_count, sizeof(zval *));
 
-    cmd_len = redis_cmd_format(&cmd,
-                    "*%d" _NL
-                    "$5" _NL
-                    "HMGET" _NL
+    // Grab our HashTable
+    ht_array = Z_ARRVAL_P(z_array);
 
-                    "$%d" _NL   /* key */
-                    "%s" _NL
-                    , nb_fields + 2
-                    , key_len, key, key_len);
-	if(key_free) efree(key);
+    // Iterate through our keys, grabbing members that are valid
+    for(valid=0, zend_hash_internal_pointer_reset_ex(ht_array, &ptr);
+        zend_hash_get_current_data_ex(ht_array, (void**)&data, &ptr)==SUCCESS;
+        zend_hash_move_forward_ex(ht_array, &ptr))
+    {
+        // Make sure the data is a long or string, and if it's a string that
+        // it isn't empty.  There is no reason to send empty length members.
+        if((Z_TYPE_PP(data) == IS_STRING && Z_STRLEN_PP(data)>0) ||
+            Z_TYPE_PP(data) == IS_LONG) 
+        {
+            // This is a key we can ask for, copy it and set it in our array
+            MAKE_STD_ZVAL(z_keys[valid]);
+            *z_keys[valid] = **data;
+            zval_copy_ctor(z_keys[valid]);
+            convert_to_string(z_keys[valid]);
+
+            // Increment the number of valid keys we've encountered
+            valid++;
+        }
+    }
 
-    arr_hash = Z_ARRVAL_P(z_array);
+    // If we don't have any valid keys, we can abort here
+    if(valid == 0) {
+        if(key_free) efree(key);
+        efree(z_keys);
+        RETURN_FALSE;
+    }
 
-    for (i = 0, zend_hash_internal_pointer_reset_ex(arr_hash, &pointer);
-                    zend_hash_get_current_data_ex(arr_hash, (void**) &data,
-                            &pointer) == SUCCESS;
-                    zend_hash_move_forward_ex(arr_hash, &pointer)) {
+    // Build command header.  One extra argument for the hash key itself
+    redis_cmd_init_sstr(&cmd, valid+1, "HMGET", sizeof("HMGET")-1);
 
-			if (Z_TYPE_PP(data) == IS_LONG || Z_TYPE_PP(data) == IS_STRING) {
+    // Add the hash key
+    redis_cmd_append_sstr(&cmd, key, key_len);
 
-				old_cmd = cmd;
-				if (Z_TYPE_PP(data) == IS_LONG) {
-				    cmd_len = redis_cmd_format(&cmd, "%s" "$%d" _NL "%d" _NL
-                                    , cmd, cmd_len
-                                    , integer_length(Z_LVAL_PP(data)), (int)Z_LVAL_PP(data));
-				} else if (Z_TYPE_PP(data) == IS_STRING) {
-				    cmd_len = redis_cmd_format(&cmd, "%s" "$%d" _NL "%s" _NL
-                                    , cmd, cmd_len
-                                    , Z_STRLEN_PP(data), Z_STRVAL_PP(data), Z_STRLEN_PP(data));
-				}
-				efree(old_cmd);
-                /* save context */
-                MAKE_STD_ZVAL(z_keys[i]);
-                *z_keys[i] = **data;
-                zval_copy_ctor(z_keys[i]);
-                convert_to_string(z_keys[i]);
+    // Free key memory if it was prefixed
+    if(key_free) efree(key);
 
-                i++;
-            }
+    // Iterate our keys, appending them as arguments
+    for(i=0;i<valid;i++) {
+        redis_cmd_append_sstr(&cmd, Z_STRVAL_P(z_keys[i]), Z_STRLEN_P(z_keys[i]));
     }
 
-    // This is a failure if none of the keys were valid
-    if(i == 0) {
-        efree(cmd);
-        efree(z_keys);
-        RETURN_FALSE;
+    // Kick off our request
+    REDIS_PROCESS_REQUEST(redis_sock, cmd.c, cmd.len);
+    IF_ATOMIC() {
+        redis_sock_read_multibulk_reply_assoc(INTERNAL_FUNCTION_PARAM_PASSTHRU, redis_sock, NULL, z_keys);
     }
-
-	REDIS_PROCESS_REQUEST(redis_sock, cmd, cmd_len);
-	IF_ATOMIC() {
-		redis_sock_read_multibulk_reply_assoc(INTERNAL_FUNCTION_PARAM_PASSTHRU, redis_sock, NULL, z_keys);
-	}
-	REDIS_PROCESS_RESPONSE_CLOSURE(redis_sock_read_multibulk_reply_assoc, z_keys);
+    REDIS_PROCESS_RESPONSE_CLOSURE(redis_sock_read_multibulk_reply_assoc, z_keys);
 }
 
 PHP_METHOD(Redis, hMset)

tests/TestRedis.php

@@ -2285,6 +2285,9 @@ public function testHashes() {
     // Test with an array populated with things we can't use as keys
     $this->assertTrue($this->redis->hmget('h', Array(false,NULL,false)) === FALSE);
 
+    // Test with some invalid keys mixed in (which should just be ignored)
+    $this->assertTrue(array('x'=>'123','y'=>'456','z'=>'abc') === $this->redis->hMget('h',Array('x',null,'y','','z',false)));
+
 	// hmget/hmset with numeric fields
 	$this->redis->del('h');
 	$this->assertTrue(TRUE === $this->redis->hMset('h', array(123 => 'x', 'y' => 456)));