Skip to content

PHPC-1849: Handle deprecation of Serializable in PHP 8.1 #1246

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.

Sign up for GitHub

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

Merged
merged 23 commits into from
Sep 1, 2021
Merged

PHPC-1849: Handle deprecation of Serializable in PHP 8.1 #1246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
b71a162
Add __serialize and __unserialize methods to Serializable classes
jmikola Aug 9, 2021
b98b7f3
Binary tests
jmikola Aug 10, 2021
a3691fa
DBPointer tests
jmikola Aug 10, 2021
204a079
Decimal128 tests
jmikola Aug 10, 2021
5955d46
Int64 tests
jmikola Aug 16, 2021
e7a69c1
Javascript tests
jmikola Aug 16, 2021
dccb51f
MaxKey and MinKey tests
jmikola Aug 16, 2021
d73a847
ObjectId tests
jmikola Aug 16, 2021
6f1e573
Regex tests
jmikola Aug 17, 2021
2371be8
Symbol tests
jmikola Aug 17, 2021
1dbff4d
Timestamp tests
jmikola Aug 17, 2021
e61b29b
Undefined tests
jmikola Aug 17, 2021
29f6983
UTCDateTime tests
jmikola Aug 17, 2021
940ce50
CursorId tests
jmikola Aug 17, 2021
d9009c0
ServerApi tests
jmikola Aug 17, 2021
598615f
ReadConcern tests
jmikola Aug 17, 2021
5dae132
ReadPreference tests
jmikola Aug 17, 2021
c1ee5d7
WriteConcern tests
jmikola Aug 17, 2021
aff6c8c
Explicitly handle serialize output in php_phongo_cursorid_get_propert…
jmikola Aug 17, 2021
08ba376
Explicitly handle serialize output in php_phongo_writeconcern_get_pro…
jmikola Aug 17, 2021
f59b619
Note maxStalenessSeconds handling in php_phongo_readpreference_get_pr…
jmikola Aug 17, 2021
e0de6c9
Fix __unserialize protos
jmikola Aug 18, 2021
6a4f475
CursorId should always serialize id as string
jmikola Aug 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 18 additions & 18 deletions php_phongo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,26 +182,26 @@ bool php_phongo_manager_unregister(php_phongo_manager_t* manager);
} while (0)
#endif

#define PHONGO_GET_PROPERTY_HASH_INIT_PROPS(is_debug, intern, props, size) \
do { \
if (is_debug) { \
ALLOC_HASHTABLE(props); \
zend_hash_init((props), (size), NULL, ZVAL_PTR_DTOR, 0); \
} else if ((intern)->properties) { \
(props) = (intern)->properties; \
} else { \
ALLOC_HASHTABLE(props); \
zend_hash_init((props), (size), NULL, ZVAL_PTR_DTOR, 0); \
(intern)->properties = (props); \
} \
#define PHONGO_GET_PROPERTY_HASH_INIT_PROPS(is_temp, intern, props, size) \
do { \
if (is_temp) { \
ALLOC_HASHTABLE(props); \
zend_hash_init((props), (size), NULL, ZVAL_PTR_DTOR, 0); \
} else if ((intern)->properties) { \
(props) = (intern)->properties; \
} else { \
ALLOC_HASHTABLE(props); \
zend_hash_init((props), (size), NULL, ZVAL_PTR_DTOR, 0); \
(intern)->properties = (props); \
} \
} while (0)

#define PHONGO_GET_PROPERTY_HASH_FREE_PROPS(is_debug, props) \
do { \
if (is_debug) { \
zend_hash_destroy((props)); \
FREE_HASHTABLE(props); \
} \
#define PHONGO_GET_PROPERTY_HASH_FREE_PROPS(is_temp, props) \
do { \
if (is_temp) { \
zend_hash_destroy((props)); \
FREE_HASHTABLE(props); \
} \
} while (0)

#define PHONGO_ZVAL_CLASS_OR_TYPE_NAME(zv) (Z_TYPE(zv) == IS_OBJECT ? ZSTR_VAL(Z_OBJCE(zv)->name) : zend_get_type_by_const(Z_TYPE(zv)))
Expand Down
80 changes: 54 additions & 26 deletions src/BSON/Binary.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,32 @@ static bool php_phongo_binary_init_from_hash(php_phongo_binary_t* intern, HashTa
return false;
} /* }}} */

static HashTable* php_phongo_binary_get_properties_hash(phongo_compat_object_handler_type* object, bool is_temp) /* {{{ */
{
php_phongo_binary_t* intern;
HashTable* props;

intern = Z_OBJ_BINARY(PHONGO_COMPAT_GET_OBJ(object));

PHONGO_GET_PROPERTY_HASH_INIT_PROPS(is_temp, intern, props, 2);

if (!intern->data) {
return props;
}

{
zval data, type;

ZVAL_STRINGL(&data, intern->data, intern->data_len);
zend_hash_str_update(props, "data", sizeof("data") - 1, &data);

ZVAL_LONG(&type, intern->type);
zend_hash_str_update(props, "type", sizeof("type") - 1, &type);
}
Comment on lines +85 to +93
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've wondered about this before. What's the practical reason for nesting? Is it to only conditionally allocate the two zval structs on the stack and avoid doing so if there's no data to handle?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These aren't even conditionally allocated. We're just using braces to create a scope and allow us to declare variables inline. Our C compiler options would otherwise flag this as a warning and require that we declare these at the top of the function scope.


return props;
} /* }}} */

/* {{{ proto void MongoDB\BSON\Binary::__construct(string $data, int $type)
Construct a new BSON binary type */
static PHP_METHOD(Binary, __construct)
Expand Down Expand Up @@ -270,6 +296,28 @@ static PHP_METHOD(Binary, unserialize)
zval_ptr_dtor(&props);
} /* }}} */

/* {{{ proto array MongoDB\Driver\Binary::__serialize()
*/
static PHP_METHOD(Binary, __serialize)
{
PHONGO_PARSE_PARAMETERS_NONE();

RETURN_ARR(php_phongo_binary_get_properties_hash(PHONGO_COMPAT_OBJ_P(getThis()), true));
} /* }}} */

/* {{{ proto void MongoDB\Driver\Binary::__unserialize(array $data)
*/
static PHP_METHOD(Binary, __unserialize)
{
zval* data;

PHONGO_PARSE_PARAMETERS_START(1, 1)
Z_PARAM_ARRAY(data)
PHONGO_PARSE_PARAMETERS_END();

php_phongo_binary_init_from_hash(Z_BINARY_OBJ_P(getThis()), Z_ARRVAL_P(data));
} /* }}} */

/* {{{ MongoDB\BSON\Binary function entries */
/* clang-format off */
ZEND_BEGIN_ARG_INFO_EX(ai_Binary___construct, 0, 0, 2)
Expand All @@ -281,6 +329,10 @@ ZEND_BEGIN_ARG_INFO_EX(ai_Binary___set_state, 0, 0, 1)
ZEND_ARG_ARRAY_INFO(0, properties, 0)
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_INFO_EX(ai_Binary___unserialize, 0, 0, 1)
ZEND_ARG_ARRAY_INFO(0, data, 0)
ZEND_END_ARG_INFO()
Comment on lines +332 to +334
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been wondering about this for a while as well: shouldn't this be in PHP's realm to provide arginfo structs for its own interface? We're duplicating this in a lot of our classes, where using ai_Serializable___unserialize would work just fine everywhere else 🤔

// cc @nikic

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting point, although I could see this being problematic if PHP happened to change its arginfo between versions. I think this may relate to why interfaces in 8.1 started using tentative return types, to defer the actual changes to PHP 9.0.

Allowing extensions to define their own arginfo presumably lets us use a narrower API if we want, which would still conform with the interface in core.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nowadays we auto-generate all arginfo (as well as class declarations in general) anyway, so we don't have any particular interest in making reuse simpler.


ZEND_BEGIN_ARG_WITH_TENTATIVE_RETURN_TYPE_INFO_EX(ai_Binary_jsonSerialize, 0, 0, IS_ARRAY, 0)
ZEND_END_ARG_INFO()

Expand All @@ -293,8 +345,10 @@ ZEND_END_ARG_INFO()

static zend_function_entry php_phongo_binary_me[] = {
PHP_ME(Binary, __construct, ai_Binary___construct, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
PHP_ME(Binary, __serialize, ai_Binary_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
PHP_ME(Binary, __set_state, ai_Binary___set_state, ZEND_ACC_PUBLIC | ZEND_ACC_STATIC)
PHP_ME(Binary, __toString, ai_Binary_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
PHP_ME(Binary, __unserialize, ai_Binary___unserialize, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
PHP_ME(Binary, jsonSerialize, ai_Binary_jsonSerialize, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
PHP_ME(Binary, serialize, ai_Binary_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
PHP_ME(Binary, unserialize, ai_Binary_unserialize, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
Expand Down Expand Up @@ -377,32 +431,6 @@ static int php_phongo_binary_compare_objects(zval* o1, zval* o2) /* {{{ */
return zend_binary_strcmp(intern1->data, intern1->data_len, intern2->data, intern2->data_len);
} /* }}} */

static HashTable* php_phongo_binary_get_properties_hash(phongo_compat_object_handler_type* object, bool is_debug) /* {{{ */
{
php_phongo_binary_t* intern;
HashTable* props;

intern = Z_OBJ_BINARY(PHONGO_COMPAT_GET_OBJ(object));

PHONGO_GET_PROPERTY_HASH_INIT_PROPS(is_debug, intern, props, 2);

if (!intern->data) {
return props;
}

{
zval data, type;

ZVAL_STRINGL(&data, intern->data, intern->data_len);
zend_hash_str_update(props, "data", sizeof("data") - 1, &data);

ZVAL_LONG(&type, intern->type);
zend_hash_str_update(props, "type", sizeof("type") - 1, &type);
}

return props;
} /* }}} */

static HashTable* php_phongo_binary_get_debug_info(phongo_compat_object_handler_type* object, int* is_temp) /* {{{ */
{
*is_temp = 1;
Expand Down
78 changes: 53 additions & 25 deletions src/BSON/DBPointer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,31 @@ static bool php_phongo_dbpointer_init_from_hash(php_phongo_dbpointer_t* intern,
return false;
} /* }}} */

HashTable* php_phongo_dbpointer_get_properties_hash(phongo_compat_object_handler_type* object, bool is_temp) /* {{{ */
{
php_phongo_dbpointer_t* intern;
HashTable* props;

intern = Z_OBJ_DBPOINTER(PHONGO_COMPAT_GET_OBJ(object));

PHONGO_GET_PROPERTY_HASH_INIT_PROPS(is_temp, intern, props, 2);

if (!intern->ref) {
return props;
}

{
zval ref, id;

ZVAL_STRING(&ref, intern->ref);
ZVAL_STRING(&id, intern->id);
zend_hash_str_update(props, "ref", sizeof("ref") - 1, &ref);
zend_hash_str_update(props, "id", sizeof("id") - 1, &id);
Comment on lines +87 to +90
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to change anything here, but getting to the second class in the code review I thought "I suppose this is now 20 times the same change", and I immediately catch the different code organisation between this and the Binary class up there. No need to change since I'd like to refactor this code in general: macros that call the combination of ZVAL_* and zend_hash_str_update would help since they could handle the scoping (if it's necessary, see earlier comment).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created PHPC-1962 to track this.

}

return props;
} /* }}} */

/* {{{ proto string MongoDB\BSON\DBPointer::__toString()
Return the DBPointer's namespace string and ObjectId. */
static PHP_METHOD(DBPointer, __toString)
Expand Down Expand Up @@ -187,8 +212,34 @@ static PHP_METHOD(DBPointer, unserialize)
zval_ptr_dtor(&props);
} /* }}} */

/* {{{ proto array MongoDB\Driver\DBPointer::__serialize()
*/
static PHP_METHOD(DBPointer, __serialize)
{
PHONGO_PARSE_PARAMETERS_NONE();

RETURN_ARR(php_phongo_dbpointer_get_properties_hash(PHONGO_COMPAT_OBJ_P(getThis()), true));
} /* }}} */

/* {{{ proto void MongoDB\Driver\DBPointer::__unserialize(array $data)
*/
static PHP_METHOD(DBPointer, __unserialize)
{
zval* data;

PHONGO_PARSE_PARAMETERS_START(1, 1)
Z_PARAM_ARRAY(data)
PHONGO_PARSE_PARAMETERS_END();

php_phongo_dbpointer_init_from_hash(Z_DBPOINTER_OBJ_P(getThis()), Z_ARRVAL_P(data));
} /* }}} */
Comment on lines +224 to +235
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this is another one where I feel PHP could offer a macro:

/* {{{ proto void MongoDB\Driver\DBPointer::__unserialize(array $data)
*/
IMPL_SERIALIZABLE___UNSERIALIZE_START()
	php_phongo_dbpointer_init_from_hash(Z_DBPOINTER_OBJ_P(getThis()), Z_ARRVAL_P(data));
IMPL_SERIALIZABLE___UNSERIALIZE_END() /* }}} */

Since the other logic is always the same, this may be an improvement to help reducing the menial task of adding this to multiple classes...


/* {{{ MongoDB\BSON\DBPointer function entries */
/* clang-format off */
ZEND_BEGIN_ARG_INFO_EX(ai_DBPointer___unserialize, 0, 0, 1)
ZEND_ARG_ARRAY_INFO(0, data, 0)
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_WITH_TENTATIVE_RETURN_TYPE_INFO_EX(ai_DBPointer_jsonSerialize, 0, 0, IS_ARRAY, 0)
ZEND_END_ARG_INFO()

Expand All @@ -201,7 +252,9 @@ ZEND_END_ARG_INFO()

static zend_function_entry php_phongo_dbpointer_me[] = {
/* __set_state intentionally missing */
PHP_ME(DBPointer, __serialize, ai_DBPointer_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
PHP_ME(DBPointer, __toString, ai_DBPointer_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
PHP_ME(DBPointer, __unserialize, ai_DBPointer___unserialize, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
PHP_ME(DBPointer, jsonSerialize, ai_DBPointer_jsonSerialize, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
PHP_ME(DBPointer, serialize, ai_DBPointer_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
PHP_ME(DBPointer, unserialize, ai_DBPointer_unserialize, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
Expand Down Expand Up @@ -279,31 +332,6 @@ static int php_phongo_dbpointer_compare_objects(zval* o1, zval* o2) /* {{{ */
return strcmp(intern1->id, intern2->id);
} /* }}} */

HashTable* php_phongo_dbpointer_get_properties_hash(phongo_compat_object_handler_type* object, bool is_debug) /* {{{ */
{
php_phongo_dbpointer_t* intern;
HashTable* props;

intern = Z_OBJ_DBPOINTER(PHONGO_COMPAT_GET_OBJ(object));

PHONGO_GET_PROPERTY_HASH_INIT_PROPS(is_debug, intern, props, 2);

if (!intern->ref) {
return props;
}

{
zval ref, id;

ZVAL_STRING(&ref, intern->ref);
ZVAL_STRING(&id, intern->id);
zend_hash_str_update(props, "ref", sizeof("ref") - 1, &ref);
zend_hash_str_update(props, "id", sizeof("id") - 1, &id);
}

return props;
} /* }}} */

static HashTable* php_phongo_dbpointer_get_debug_info(phongo_compat_object_handler_type* object, int* is_temp) /* {{{ */
{
*is_temp = 1;
Expand Down
80 changes: 54 additions & 26 deletions src/BSON/Decimal128.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,32 @@ static bool php_phongo_decimal128_init_from_hash(php_phongo_decimal128_t* intern
return false;
} /* }}} */

static HashTable* php_phongo_decimal128_get_properties_hash(phongo_compat_object_handler_type* object, bool is_temp) /* {{{ */
{
php_phongo_decimal128_t* intern;
HashTable* props;
char outbuf[BSON_DECIMAL128_STRING] = "";

intern = Z_OBJ_DECIMAL128(PHONGO_COMPAT_GET_OBJ(object));

PHONGO_GET_PROPERTY_HASH_INIT_PROPS(is_temp, intern, props, 1);

if (!intern->initialized) {
return props;
}

bson_decimal128_to_string(&intern->decimal, outbuf);

{
zval dec;

ZVAL_STRING(&dec, outbuf);
zend_hash_str_update(props, "dec", sizeof("dec") - 1, &dec);
}

return props;
} /* }}} */

/* {{{ proto void MongoDB\BSON\Decimal128::__construct(string $value)
Construct a new BSON Decimal128 type */
static PHP_METHOD(Decimal128, __construct)
Expand Down Expand Up @@ -214,6 +240,28 @@ static PHP_METHOD(Decimal128, unserialize)
zval_ptr_dtor(&props);
} /* }}} */

/* {{{ proto array MongoDB\Driver\Decimal128::__serialize()
*/
static PHP_METHOD(Decimal128, __serialize)
{
PHONGO_PARSE_PARAMETERS_NONE();

RETURN_ARR(php_phongo_decimal128_get_properties_hash(PHONGO_COMPAT_OBJ_P(getThis()), true));
} /* }}} */

/* {{{ proto void MongoDB\Driver\Decimal128::__unserialize(array $data)
*/
static PHP_METHOD(Decimal128, __unserialize)
{
zval* data;

PHONGO_PARSE_PARAMETERS_START(1, 1)
Z_PARAM_ARRAY(data)
PHONGO_PARSE_PARAMETERS_END();

php_phongo_decimal128_init_from_hash(Z_DECIMAL128_OBJ_P(getThis()), Z_ARRVAL_P(data));
} /* }}} */

/* {{{ MongoDB\BSON\Decimal128 function entries */
/* clang-format off */
ZEND_BEGIN_ARG_INFO_EX(ai_Decimal128___construct, 0, 0, 1)
Expand All @@ -224,6 +272,10 @@ ZEND_BEGIN_ARG_INFO_EX(ai_Decimal128___set_state, 0, 0, 1)
ZEND_ARG_ARRAY_INFO(0, properties, 0)
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_INFO_EX(ai_Decimal128___unserialize, 0, 0, 1)
ZEND_ARG_ARRAY_INFO(0, data, 0)
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_WITH_TENTATIVE_RETURN_TYPE_INFO_EX(ai_Decimal128_jsonSerialize, 0, 0, IS_ARRAY, 0)
ZEND_END_ARG_INFO()

Expand All @@ -236,8 +288,10 @@ ZEND_END_ARG_INFO()

static zend_function_entry php_phongo_decimal128_me[] = {
PHP_ME(Decimal128, __construct, ai_Decimal128___construct, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
PHP_ME(Decimal128, __serialize, ai_Decimal128_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
PHP_ME(Decimal128, __set_state, ai_Decimal128___set_state, ZEND_ACC_PUBLIC | ZEND_ACC_STATIC)
PHP_ME(Decimal128, __toString, ai_Decimal128_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
PHP_ME(Decimal128, __unserialize, ai_Decimal128___unserialize, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
PHP_ME(Decimal128, jsonSerialize, ai_Decimal128_jsonSerialize, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
PHP_ME(Decimal128, serialize, ai_Decimal128_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
PHP_ME(Decimal128, unserialize, ai_Decimal128_unserialize, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
Expand Down Expand Up @@ -294,32 +348,6 @@ static zend_object* php_phongo_decimal128_clone_object(phongo_compat_object_hand
return new_object;
} /* }}} */

static HashTable* php_phongo_decimal128_get_properties_hash(phongo_compat_object_handler_type* object, bool is_debug) /* {{{ */
{
php_phongo_decimal128_t* intern;
HashTable* props;
char outbuf[BSON_DECIMAL128_STRING] = "";

intern = Z_OBJ_DECIMAL128(PHONGO_COMPAT_GET_OBJ(object));

PHONGO_GET_PROPERTY_HASH_INIT_PROPS(is_debug, intern, props, 1);

if (!intern->initialized) {
return props;
}

bson_decimal128_to_string(&intern->decimal, outbuf);

{
zval dec;

ZVAL_STRING(&dec, outbuf);
zend_hash_str_update(props, "dec", sizeof("dec") - 1, &dec);
}

return props;
} /* }}} */

static HashTable* php_phongo_decimal128_get_debug_info(phongo_compat_object_handler_type* object, int* is_temp) /* {{{ */
{
*is_temp = 1;
Expand Down
Loading