-
Notifications
You must be signed in to change notification settings - Fork 208
PHPC-790: Accept integer and floats in Timestamp and UTCDateTime constructors #467
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
1f785d2
8d7e358
6fd69af
a916713
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ | |
/* External libs */ | ||
#include <bson.h> | ||
#include <mongoc.h> | ||
#include <math.h> | ||
|
||
/* PHP Core stuff */ | ||
#include <php.h> | ||
|
@@ -77,11 +78,7 @@ static bool php_phongo_utcdatetime_init_from_string(php_phongo_utcdatetime_t *in | |
|
||
errno = 0; | ||
|
||
#if defined(PHP_WIN32) | ||
milliseconds = _atoi64(s_milliseconds); | ||
#else | ||
milliseconds = bson_ascii_strtoll(s_milliseconds, &endptr, 10); | ||
#endif | ||
|
||
/* errno will set errno if conversion fails; however, we do not need to | ||
* specify the type of error. | ||
|
@@ -161,49 +158,64 @@ static bool php_phongo_utcdatetime_init_from_date(php_phongo_utcdatetime_t *inte | |
return true; | ||
} | ||
|
||
/* {{{ proto void UTCDateTime::__construct([string|DateTimeInterface $milliseconds = null]) | ||
/* {{{ proto void UTCDateTime::__construct([int|float|string|DateTimeInterface $milliseconds = null]) | ||
Construct a new BSON UTCDateTime type from either the current time, | ||
milliseconds since the epoch, or a DateTimeInterface object. */ | ||
milliseconds since the epoch, or a DateTimeInterface object. Defaults to the | ||
current time. */ | ||
PHP_METHOD(UTCDateTime, __construct) | ||
{ | ||
php_phongo_utcdatetime_t *intern; | ||
zend_error_handling error_handling; | ||
zval *datetime = NULL; | ||
zval *milliseconds = NULL; | ||
|
||
zend_replace_error_handling(EH_THROW, phongo_exception_from_phongo_domain(PHONGO_ERROR_INVALID_ARGUMENT), &error_handling TSRMLS_CC); | ||
intern = Z_UTCDATETIME_OBJ_P(getThis()); | ||
|
||
if (zend_parse_parameters_ex(ZEND_PARSE_PARAMS_QUIET, ZEND_NUM_ARGS() TSRMLS_CC, "|o!", &datetime) == SUCCESS) { | ||
if (datetime == NULL) { | ||
php_phongo_utcdatetime_init_from_current_time(intern); | ||
} else if (instanceof_function(Z_OBJCE_P(datetime), php_date_get_date_ce() TSRMLS_CC)) { | ||
php_phongo_utcdatetime_init_from_date(intern, Z_PHPDATE_P(datetime)); | ||
if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "|z!", &milliseconds) == FAILURE) { | ||
zend_restore_error_handling(&error_handling TSRMLS_CC); | ||
return; | ||
} | ||
zend_restore_error_handling(&error_handling TSRMLS_CC); | ||
|
||
if (milliseconds == NULL) { | ||
php_phongo_utcdatetime_init_from_current_time(intern); | ||
return; | ||
} | ||
|
||
if (Z_TYPE_P(milliseconds) == IS_OBJECT) { | ||
if (instanceof_function(Z_OBJCE_P(milliseconds), php_date_get_date_ce() TSRMLS_CC)) { | ||
php_phongo_utcdatetime_init_from_date(intern, Z_PHPDATE_P(milliseconds)); | ||
#if PHP_VERSION_ID >= 50500 | ||
} else if (instanceof_function(Z_OBJCE_P(datetime), php_date_get_immutable_ce() TSRMLS_CC)) { | ||
php_phongo_utcdatetime_init_from_date(intern, Z_PHPDATE_P(datetime)); | ||
} else if (instanceof_function(Z_OBJCE_P(milliseconds), php_date_get_immutable_ce() TSRMLS_CC)) { | ||
php_phongo_utcdatetime_init_from_date(intern, Z_PHPDATE_P(milliseconds)); | ||
#endif | ||
} else { | ||
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected instance of DateTimeInterface, %s given", ZSTR_VAL(Z_OBJCE_P(datetime)->name)); | ||
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected instance of DateTimeInterface, %s given", ZSTR_VAL(Z_OBJCE_P(milliseconds)->name)); | ||
} | ||
return; | ||
} | ||
|
||
zend_restore_error_handling(&error_handling TSRMLS_CC); | ||
if (Z_TYPE_P(milliseconds) == IS_LONG) { | ||
php_phongo_utcdatetime_init(intern, Z_LVAL_P(milliseconds)); | ||
return; | ||
} | ||
|
||
{ | ||
char *s_milliseconds; | ||
phongo_zpp_char_len s_milliseconds_len; | ||
if (Z_TYPE_P(milliseconds) == IS_DOUBLE) { | ||
char tmp[24]; | ||
int tmp_len; | ||
|
||
if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", &s_milliseconds, &s_milliseconds_len) == FAILURE) { | ||
zend_restore_error_handling(&error_handling TSRMLS_CC); | ||
return; | ||
} | ||
tmp_len = snprintf(tmp, sizeof(tmp), "%.0f", Z_DVAL_P(milliseconds) > 0 ? floor(Z_DVAL_P(milliseconds)) : ceil(Z_DVAL_P(milliseconds))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't it make more sense to convert to a long straight away here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I originally used |
||
|
||
php_phongo_utcdatetime_init_from_string(intern, s_milliseconds, s_milliseconds_len TSRMLS_CC); | ||
php_phongo_utcdatetime_init_from_string(intern, tmp, tmp_len TSRMLS_CC); | ||
return; | ||
} | ||
|
||
zend_restore_error_handling(&error_handling TSRMLS_CC); | ||
if (Z_TYPE_P(milliseconds) != IS_STRING) { | ||
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected integer or string, %s given", zend_get_type_by_const(Z_TYPE_P(milliseconds))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. HHVM only throws "Expected instance of DateTimeInterface" and "Error parsing %s as 64-bit integer" exceptions, so it may need a PR for this (see here). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the HHVM message is much clearer as an API point of view. The "error parsing x as 64-bit integer" is what people are interested in. Whether it's an "integer", "float", or "string" is irrelevant. The current message in your code also misses "float". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I omitted "float" from the message since the cast to a long is just for the user's convenience. I wasn't planning on documenting that we accept a float, since it is being truncated. This is mainly to skirt around scalar type hint errors. In HHVM's case, the invalid argument is cast to a string value for the exception message. That leads to some odd output for null (empty string), boolean ("1" and "0"), and would likely emit some notice on an array. The type error I was using here was consistent with what we already did for ReadPreference, WriteConcern, etc. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK - add some tests for these additional types, so I can test HHVM against it as well? (If you haven't done so) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests are in |
||
return; | ||
} | ||
|
||
php_phongo_utcdatetime_init_from_string(intern, Z_STRVAL_P(milliseconds), Z_STRLEN_P(milliseconds) TSRMLS_CC); | ||
} | ||
/* }}} */ | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
--TEST-- | ||
MongoDB\BSON\Timestamp unserialization requires strings to parse as 64-bit integers | ||
--FILE-- | ||
<?php | ||
|
||
require_once __DIR__ . '/../utils/tools.php'; | ||
|
||
echo throws(function() { | ||
unserialize('C:22:"MongoDB\BSON\Timestamp":60:{a:2:{s:9:"increment";s:4:"1.23";s:9:"timestamp";s:4:"5678";}}'); | ||
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n"; | ||
|
||
echo throws(function() { | ||
unserialize('C:22:"MongoDB\BSON\Timestamp":60:{a:2:{s:9:"increment";s:4:"1234";s:9:"timestamp";s:4:"5.67";}}'); | ||
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n"; | ||
|
||
?> | ||
===DONE=== | ||
<?php exit(0); ?> | ||
--EXPECT-- | ||
OK: Got MongoDB\Driver\Exception\InvalidArgumentException | ||
Error parsing "1.23" as 64-bit integer increment for MongoDB\BSON\Timestamp initialization | ||
OK: Got MongoDB\Driver\Exception\InvalidArgumentException | ||
Error parsing "5.67" as 64-bit integer timestamp for MongoDB\BSON\Timestamp initialization | ||
===DONE=== |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,26 +1,24 @@ | ||
--TEST-- | ||
MongoDB\BSON\Timestamp::__set_state() with broken numeric strings | ||
--SKIPIF-- | ||
<?php if (8 !== PHP_INT_SIZE) { die('skip Only for 64-bit platform'); } ?> | ||
MongoDB\BSON\Timestamp::__set_state() requires strings to parse as 64-bit integers | ||
--FILE-- | ||
<?php | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this was erroneously copy-pasted when the test was created in d25e565. |
||
|
||
require_once __DIR__ . '/../utils/tools.php'; | ||
|
||
echo throws(function() { | ||
MongoDB\BSON\Timestamp::__set_state(['increment' => 'broken', 'timestamp' => '5678']); | ||
MongoDB\BSON\Timestamp::__set_state(['increment' => '1.23', 'timestamp' => '5678']); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test would have been problematic for Windows since |
||
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n"; | ||
|
||
echo throws(function() { | ||
MongoDB\BSON\Timestamp::__set_state(['increment' => '1234', 'timestamp' => 'broken']); | ||
MongoDB\BSON\Timestamp::__set_state(['increment' => '1234', 'timestamp' => '5.67']); | ||
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n"; | ||
|
||
?> | ||
===DONE=== | ||
<?php exit(0); ?> | ||
--EXPECT-- | ||
OK: Got MongoDB\Driver\Exception\InvalidArgumentException | ||
Error parsing "broken" as 64-bit integer increment for MongoDB\BSON\Timestamp initialization | ||
Error parsing "1.23" as 64-bit integer increment for MongoDB\BSON\Timestamp initialization | ||
OK: Got MongoDB\Driver\Exception\InvalidArgumentException | ||
Error parsing "broken" as 64-bit integer timestamp for MongoDB\BSON\Timestamp initialization | ||
Error parsing "5.67" as 64-bit integer timestamp for MongoDB\BSON\Timestamp initialization | ||
===DONE=== |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
--TEST-- | ||
MongoDB\BSON\Timestamp constructor requires strings to parse as 64-bit integers | ||
--FILE-- | ||
<?php | ||
|
||
require_once __DIR__ . '/../utils/tools.php'; | ||
|
||
echo throws(function() { | ||
new MongoDB\BSON\Timestamp('1.23', '5678'); | ||
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n"; | ||
|
||
echo throws(function() { | ||
new MongoDB\BSON\Timestamp('1234', '5.67'); | ||
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n"; | ||
|
||
?> | ||
===DONE=== | ||
<?php exit(0); ?> | ||
--EXPECT-- | ||
OK: Got MongoDB\Driver\Exception\InvalidArgumentException | ||
Error parsing "1.23" as 64-bit integer increment for MongoDB\BSON\Timestamp initialization | ||
OK: Got MongoDB\Driver\Exception\InvalidArgumentException | ||
Error parsing "5.67" as 64-bit integer timestamp for MongoDB\BSON\Timestamp initialization | ||
===DONE=== |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
--TEST-- | ||
MongoDB\BSON\Timestamp constructor requires integer or string arguments | ||
--FILE-- | ||
<?php | ||
|
||
require_once __DIR__ . '/../utils/tools.php'; | ||
|
||
$invalidValues = [null, 1234.5, true, [], new stdClass]; | ||
|
||
foreach ($invalidValues as $invalidValue) { | ||
echo throws(function() use ($invalidValue) { | ||
new MongoDB\BSON\Timestamp($invalidValue, 0); | ||
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n"; | ||
} | ||
|
||
foreach ($invalidValues as $invalidValue) { | ||
echo throws(function() use ($invalidValue) { | ||
new MongoDB\BSON\Timestamp(0, $invalidValue); | ||
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n"; | ||
} | ||
|
||
?> | ||
===DONE=== | ||
<?php exit(0); ?> | ||
--EXPECTF-- | ||
OK: Got MongoDB\Driver\Exception\InvalidArgumentException | ||
Expected increment to be an unsigned 32-bit integer or string, %r(null|NULL)%r given | ||
OK: Got MongoDB\Driver\Exception\InvalidArgumentException | ||
Expected increment to be an unsigned 32-bit integer or string, %r(double|float)%r given | ||
OK: Got MongoDB\Driver\Exception\InvalidArgumentException | ||
Expected increment to be an unsigned 32-bit integer or string, boolean given | ||
OK: Got MongoDB\Driver\Exception\InvalidArgumentException | ||
Expected increment to be an unsigned 32-bit integer or string, array given | ||
OK: Got MongoDB\Driver\Exception\InvalidArgumentException | ||
Expected increment to be an unsigned 32-bit integer or string, object given | ||
OK: Got MongoDB\Driver\Exception\InvalidArgumentException | ||
Expected timestamp to be an unsigned 32-bit integer or string, %r(null|NULL)%r given | ||
OK: Got MongoDB\Driver\Exception\InvalidArgumentException | ||
Expected timestamp to be an unsigned 32-bit integer or string, %r(double|float)%r given | ||
OK: Got MongoDB\Driver\Exception\InvalidArgumentException | ||
Expected timestamp to be an unsigned 32-bit integer or string, boolean given | ||
OK: Got MongoDB\Driver\Exception\InvalidArgumentException | ||
Expected timestamp to be an unsigned 32-bit integer or string, array given | ||
OK: Got MongoDB\Driver\Exception\InvalidArgumentException | ||
Expected timestamp to be an unsigned 32-bit integer or string, object given | ||
===DONE=== |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
--TEST-- | ||
MongoDB\BSON\UTCDateTime constructor truncates floating point values | ||
--FILE-- | ||
<?php | ||
|
||
$tests = [ | ||
new MongoDB\BSON\UTCDateTime(1416445411987.0), | ||
new MongoDB\BSON\UTCDateTime(2147483647.0), | ||
new MongoDB\BSON\UTCDateTime(1234.5678), | ||
]; | ||
|
||
foreach ($tests as $test) { | ||
var_dump($test); | ||
} | ||
|
||
?> | ||
===DONE=== | ||
<?php exit(0); ?> | ||
--EXPECTF-- | ||
object(MongoDB\BSON\UTCDateTime)#%d (%d) { | ||
["milliseconds"]=> | ||
string(13) "1416445411987" | ||
} | ||
object(MongoDB\BSON\UTCDateTime)#%d (%d) { | ||
["milliseconds"]=> | ||
string(10) "2147483647" | ||
} | ||
object(MongoDB\BSON\UTCDateTime)#%d (%d) { | ||
["milliseconds"]=> | ||
string(4) "1234" | ||
} | ||
===DONE=== |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ MongoDB\BSON\UTCDateTime unserialization requires "milliseconds" string to parse | |
require_once __DIR__ . '/../utils/tools.php'; | ||
|
||
echo throws(function() { | ||
unserialize('C:24:"MongoDB\BSON\UTCDateTime":40:{a:1:{s:12:"milliseconds";s:7:"INVALID";}}'); | ||
unserialize('C:24:"MongoDB\BSON\UTCDateTime":42:{a:1:{s:12:"milliseconds";s:9:"1234.5678";}}'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would have kept the length of the argument the same, as to not have to mess with lengths in hardcoded serialized strings. But that's done now. |
||
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n"; | ||
|
||
/* TODO: Add tests for out-of-range values once CDRIVER-1377 is resolved */ | ||
|
@@ -16,5 +16,5 @@ echo throws(function() { | |
<?php exit(0); ?> | ||
--EXPECT-- | ||
OK: Got MongoDB\Driver\Exception\InvalidArgumentException | ||
Error parsing "INVALID" as 64-bit integer for MongoDB\BSON\UTCDateTime initialization | ||
Error parsing "1234.5678" as 64-bit integer for MongoDB\BSON\UTCDateTime initialization | ||
===DONE=== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_atoi64()
would parse "1234.5" as 1234 and raise no error, whilebson_ascii_strtoll()
is more strict. Also,bson_ascii_strtoll()
now supports out-of-range error reporting in libbson 1.5.0 (CDRIVER-1337). That was the only benefit of using_atoi64()
for Windows before, so I see no reason to use consistent parsing for all platforms.