Skip to content
This repository

Fix problem with floatvalue-scores in PHP 5.4 #189

Merged
merged 5 commits into from over 1 year ago

4 participants

Max Beutel Nicolas Favre-Felix Lars Strojny Michael Grunder
Max Beutel

Due to a bug/weird behaviour in PHP 5.4 with _php_math_number_format float values were transformed to invalid values.

Test script:

$redis = new Redis();
$redis->connect('redis01');
$redis->zAdd('foo', 1337706476.8469, 'VAL');

Redis command sent with rev. f8f552e

"ZADD" "foo" "1" "VAL"

Expected command (at least within a certain precision range)

"ZADD" "foo" "1337706476.84689999" "VAL"

This patch circumvents the problem by not using _php_math_number_format at all and using spprintf which is also used internally in _php_math_number_format for getting the string length of the input.

See https://bugs.php.net/bug.php?id=62112
PHP Version: PHP 5.4.1 on Debian Squeeze

Max Beutel

Other functions like incrbyfloat might be affected as well because _php_math_number_format is also used in library.c which I didnt notice before, we only use zAdd.

I think it would make sense to replace all occurences of _php_math_number_format with spprintf, if you are ok with this solution proposed?

Nicolas Favre-Felix
Owner

Hello Max,

I think this is how I had implemented it in the first place, but there were issues with several locales, notably French which would use a comma instead of a period as a decimal separator.
Have you tried your change with these?

Nicolas

Max Beutel

Hi, I think you are right, that is an issue with sprintf and the like... I updated the pull request, we can use _php_math_number_format_ex in PHP 5.4 and give the thousand separator a zero length, then the function returns a correct value.

Max Beutel

This changes breaks the tests, didn´t check it again unter PHP 5.4, will fix it soon.

Nicolas Favre-Felix
Owner

That looks pretty good, I'll try it tonight.

Max Beutel

This should work now, I used the API version for comparision, thats also used in library.c. Thanks a lot to @lstrojny for help!

Lars Strojny

Can we get some feedback here?

Michael Grunder
Collaborator

Sorry guys,

It looks fine to me. I'll clone your fork and do some testing to see if anything breaks on various versions of PHP.

Thanks,
Mike

Lars Strojny

Thanks!

Michael Grunder michael-grunder referenced this pull request from a commit June 15, 2012
Michael Grunder Use a custom method to encode float values, depending on our PHP vers…
…ion.

Many thanks to @maxbeutel and @lstrojny for reporting this issue and
coming up with a resolution!

This is related to fix/pull #189
b105920
Michael Grunder
Collaborator

Hey guys,

I have incorporated your changes into a new branch php-numencode that I will merge to the master branch after a bit more testing (over the weekend). So far it is working great for me. @nicolasff had some concerns about different locales, so I will test that specifically.

Apologies for not simply merging your changes, but I wasn't exactly sure of the process to manually merge changes within git, so I just typed them in.

Cheers!
Mike

Lars Strojny

The patch uses PHP's number_format() which is not locale aware.

Michael Grunder
Collaborator

@lstrojny I think this is OK though, because it's using \0 as the thousands separator, which (for the different versions of PHP I've tried this on) works.

Lars Strojny

Yep, it may not be locale aware. Using \0 as thousand separator no longer works with 5.4 though, that’s all the patch is about.

Michael Grunder
Collaborator

Man, i'm sorry. I must be confused about something. I tested with 5.4 and 5.3.10 and both work properly for me, whereas the described bug occures for me without the patch.

What am I missing?

Lars Strojny

Nothing. The patch fixes the issue with PHP 5.4 and we use it in production for a few weeks now.

Michael Grunder
Collaborator

Cool. I have since figured out how to manually merge in git, so I'll delete the branch I did and import the changes from the pull request so proper credit is given. :)

Cheers guys,
Mike

Lars Strojny

Any news?

Michael Grunder
Collaborator

I added the InterNations upstream and manually merged into a new branch. If it looks good to @nicolasff we'll merge into the master.

Lars Strojny

Any news? This pull requests has been sitting waiting for a while now, could we get it merged. If you need anything from us, please let us know.

Michael Grunder michael-grunder merged commit c12a873 into from August 16, 2012
Michael Grunder michael-grunder closed this August 16, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
6  library.c
@@ -259,8 +259,7 @@ redis_cmd_format_static(char **ret, char *keyword, char *format, ...) {
259 259
 			case 'f':
260 260
 			case 'F': {
261 261
 				double d = va_arg(ap, double);
262  
-				dbl_str = _php_math_number_format(d, 8, '.', '\x00');
263  
-				dbl_len = strlen(dbl_str);
  262
+				REDIS_DOUBLE_TO_STRING(dbl_str, dbl_len, d)
264 263
 				smart_str_append_long(&buf, dbl_len);
265 264
 				smart_str_appendl(&buf, _NL, sizeof(_NL) - 1);
266 265
 				smart_str_appendl(&buf, dbl_str, dbl_len);
@@ -328,8 +327,7 @@ redis_cmd_format(char **ret, char *format, ...) {
328 327
 				case 'F':
329 328
 				case 'f': {
330 329
 					double d = va_arg(ap, double);
331  
-					dbl_str = _php_math_number_format(d, 8, '.', '\x00');
332  
-					dbl_len = strlen(dbl_str);
  330
+					REDIS_DOUBLE_TO_STRING(dbl_str, dbl_len, d)
333 331
 					smart_str_append_long(&buf, dbl_len);
334 332
 					smart_str_appendl(&buf, _NL, sizeof(_NL) - 1);
335 333
 					smart_str_appendl(&buf, dbl_str, dbl_len);
12  library.h
@@ -42,3 +42,15 @@ redis_key_prefix(RedisSock *redis_sock, char **key, int *key_len TSRMLS_DC);
42 42
 PHPAPI int
43 43
 redis_unserialize(RedisSock *redis_sock, const char *val, int val_len, zval **return_value TSRMLS_DC);
44 44
 
  45
+#if ZEND_MODULE_API_NO >= 20100000
  46
+#define REDIS_DOUBLE_TO_STRING(dbl_str, dbl_len, dbl) \
  47
+	char dbl_decsep; \
  48
+	dbl_decsep = '.'; \
  49
+	dbl_str = _php_math_number_format_ex(dbl, 8, &dbl_decsep, 1, NULL, 0); \
  50
+	dbl_len = strlen(dbl_str);
  51
+#else
  52
+#define REDIS_DOUBLE_TO_STRING(dbl_str, dbl_len, dbl) \
  53
+	dbl_str = _php_math_number_format(dbl, 8, '.', '\x00'); \
  54
+	dbl_len = strlen(dbl_str);
  55
+#endif
  56
+
3  redis.c
@@ -3662,8 +3662,7 @@ PHP_METHOD(Redis, zAdd) {
3662 3662
 
3663 3663
 		/* add score */
3664 3664
 		score = Z_DVAL_P(z_args[i]);
3665  
-		dbl_str = _php_math_number_format(score, 8, '.', '\x00');
3666  
-		dbl_len = strlen(dbl_str);
  3665
+		REDIS_DOUBLE_TO_STRING(dbl_str, dbl_len, score)
3667 3666
 		smart_str_appendc(&buf, '$');
3668 3667
 		smart_str_append_long(&buf, dbl_len);
3669 3668
 		smart_str_appendl(&buf, _NL, sizeof(_NL) - 1);
16  tests/TestRedis.php
@@ -1794,6 +1794,22 @@ public function testZX() {
1794 1794
 	$this->redis->delete('key2');
1795 1795
 	$this->redis->delete('key3');
1796 1796
 
  1797
+	$this->redis->zadd('key1', 2000.1, 'one');
  1798
+	$this->redis->zadd('key1', 3000.1, 'two');
  1799
+	$this->redis->zadd('key1', 4000.1, 'three');
  1800
+
  1801
+	$ret = $this->redis->zRange('key1', 0, -1, TRUE);
  1802
+	$this->assertTrue(count($ret) === 3);
  1803
+	$retValues = array_keys($ret);
  1804
+
  1805
+	$this->assertTrue(array('one', 'two', 'three') === $retValues);
  1806
+
  1807
+	// + 0 converts from string to float OR integer
  1808
+	$this->assertTrue(is_float($ret['one'] + 0));
  1809
+	$this->assertTrue(is_float($ret['two'] + 0));
  1810
+	$this->assertTrue(is_float($ret['three'] + 0));
  1811
+
  1812
+	$this->redis->delete('key1');
1797 1813
 
1798 1814
 	// ZREMRANGEBYRANK
1799 1815
 	$this->redis->zAdd('key1', 1, 'one');
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.