Conversation
|
|
||
| PHP_INSTALL_HEADERS([ext/uuid], m4_normalize([ | ||
| php_uuid.h | ||
| uuidv7-h/php_uuid.h |
There was a problem hiding this comment.
This should be uuidv7.h right?
| uuidv7-h/php_uuid.h | |
| uuidv7-h/uuidv7.h |
|
|
||
| #else | ||
|
|
||
| ts->tv_sec = zend_time_real_get(); |
There was a problem hiding this comment.
| ts->tv_sec = zend_time_real_get(); | |
| ts->tv_sec = zend_time_real_sec(); |
Is this a typo? We don't get a definition of zend_time_real_get yet(?)
|
|
||
| ZEND_ATTRIBUTE_NONNULL_ARGS(1) PHPAPI zend_result php_uuid_v7_parse(const zend_string *uuid_str, php_uuid_v7 uuid) | ||
| { | ||
| int result = uuidv7_from_string(ZSTR_VAL(uuid_str), uuid); |
There was a problem hiding this comment.
since uuidv7_from_string only check if the input is a valid uuid but not a valid uuidv7. I suggest we can have a function like
static zend_always_inline bool php_uuid_v7_is_valid(const php_uuid_v7 uuid)
{
return (uuid[6] & 0xf0) == 0x70 && (uuid[8] & 0xc0) == 0x80;
}To specifically check if the input is uuidv7
| case UUIDV7_STATUS_CLOCK_ROLLBACK: | ||
| ZEND_FALLTHROUGH; | ||
| case UUIDV7_STATUS_ERR_TIMESTAMP: | ||
| ZEND_FALLTHROUGH; |
There was a problem hiding this comment.
Would this be clearer?
| ZEND_FALLTHROUGH; | |
| zend_throw_error(NULL, "The generated UUID v7 timestamp is out of range"); | |
| RETURN_THROWS(); |
| case UUIDV7_STATUS_ERR_TIMESTAMP: | ||
| ZEND_FALLTHROUGH; | ||
| case UUIDV7_STATUS_ERR_TIMESTAMP_OVERFLOW: | ||
| break; |
There was a problem hiding this comment.
| break; | |
| zend_throw_error(NULL, "The generated UUID v7 timestamp overflowed"); | |
| RETURN_THROWS(); |
| uuid_object->is_initialized = false; | ||
| } | ||
|
|
||
| PHP_METHOD(Uuid_UuidV7, generate) |
There was a problem hiding this comment.
This uses a monotonic clock when no DateTimeImmutable is passed. UUID v7 is supposed to encode Unix wall-clock time, so on platforms where zend_hrtime() is available this will produce timestamps based on uptime rather than the Unix epoch. So I'd suggest to implement a php_uuid_current_unix_time_ms() here, it could also be used in other time-based uuids like uuidv1
| } | ||
|
|
||
| uint8_t random_bytes[10]; | ||
| for (int i = 0; i < 10; i++) { |
There was a problem hiding this comment.
The default Random\Engine is MT19937 which is PRNG. In the RFC of uuidv7 the random bytes should be generated in CSPRNG, and therefore I suggest using Random\CryptoSafeEngine instead here.
There was a problem hiding this comment.
therefore I suggest using
Random\CryptoSafeEngineinstead here.
Not necessary to restrict this to CryptSafeEngines, but the default should indeed be Random\Engine\Secure.
| object_init_ex(return_value, Z_CE_P(ZEND_THIS)); | ||
| php_uuid_v7_object *uuid_object = Z_UUID_V7_OBJECT_P(return_value); | ||
|
|
||
| if (uuidv7_from_string(ZSTR_VAL(uuid_str), uuid_object->uuid) == FAILURE) { | ||
| zend_throw_exception(NULL, "The specified UUID v7 is malformed", 0); | ||
| RETURN_THROWS(); |
There was a problem hiding this comment.
If I am understanding this code correctly, this could be clearer:
| object_init_ex(return_value, Z_CE_P(ZEND_THIS)); | |
| php_uuid_v7_object *uuid_object = Z_UUID_V7_OBJECT_P(return_value); | |
| if (uuidv7_from_string(ZSTR_VAL(uuid_str), uuid_object->uuid) == FAILURE) { | |
| zend_throw_exception(NULL, "The specified UUID v7 is malformed", 0); | |
| RETURN_THROWS(); | |
| if (php_uuid_v7_parse(uuid_str, uuid) == FAILURE) { | |
| zend_throw_exception(NULL, "The specified UUID is not a valid UUID v7", 0); | |
| RETURN_THROWS(); |
| if (zend_hash_num_elements(Z_ARRVAL_P(arr)) > 0) { | ||
| zend_throw_exception_ex(NULL, 0, "Invalid serialization data for %s object", ZSTR_VAL(uuid_object->std.ce->name)); | ||
| RETURN_THROWS(); | ||
| } |
There was a problem hiding this comment.
To make it readonly.
| } | |
| } | |
| uuid_object->is_initialized = true; |
| memcpy(new_uuid_object->uuid, uuid_object->uuid, sizeof(php_uuid_v7)); | ||
| zend_objects_clone_members(&new_uuid_object->std, &uuid_object->std); | ||
|
|
There was a problem hiding this comment.
Mark this as is_initialized after we clone it.
| memcpy(new_uuid_object->uuid, uuid_object->uuid, sizeof(php_uuid_v7)); | |
| zend_objects_clone_members(&new_uuid_object->std, &uuid_object->std); | |
| memcpy(new_uuid_object->uuid, uuid_object->uuid, sizeof(php_uuid_v7)); | |
| new_uuid_object->is_initialized = true; | |
| zend_objects_clone_members(&new_uuid_object->std, &uuid_object->std); | |
|
|
||
| uint64_t unix_time_ms; | ||
| if (datetime_object == NULL) { | ||
| unix_time_ms = zend_time_mono_fallback_nsec() / 1000000; |
There was a problem hiding this comment.
mono is not the correct block to use, it can only be used to measure durations within a single execution.
| } | ||
|
|
||
| uint8_t random_bytes[10]; | ||
| for (int i = 0; i < 10; i++) { |
There was a problem hiding this comment.
therefore I suggest using
Random\CryptoSafeEngineinstead here.
Not necessary to restrict this to CryptSafeEngines, but the default should indeed be Random\Engine\Secure.
| uint8_t random_bytes[10]; | ||
| for (int i = 0; i < 10; i++) { | ||
| random_bytes[i] = php_random_range(random_algo, 0, 127); | ||
| } |
Native support for UUIDs is clearly missing from PHP, so the newly added UUID extension would fill this void. For now, only v7 is implemented, just for demonstration purposes.
I wanted to benchmark how a native implementation compares against
symfony/uidandramsey/uuidperformance-wise. Here are the initial results (with the caveat that I have not verified yet the correctness of the native version, and monotonic ordering is not yet guaranteed):PHP UUID - 50 iterations, 20 warmups, 10 requests (sec)
Symfony UUID - 50 iterations, 20 warmups, 10 requests (sec)
Ramsey UUID - 50 iterations, 20 warmups, 10 requests (sec)