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

jmikola · 2021-08-10T17:01:24Z

https://jira.mongodb.org/browse/PHPC-1849

These methods will be used on PHP 7.4+ when available. Implementing them is required to suppress a deprecation notice on PHP 8.1+. This commit also renames is_debug parameter for get_properties_hash utility functions (and related macros) to more accurately reflect its purpose of allocating a new HashTable instead of using an internal props table.

…ies_hash

…perties_hash

…operties_hash

php_phongo.h

src/BSON/Binary.c

src/MongoDB/CursorId.c

src/MongoDB/WriteConcern.c

tests/bson/bson-utcdatetime-serialization_error-004.phpt

alcaeus

Changes look good, just a couple of questions on the code to judge potential refactoring effort.

php_phongo.h

alcaeus · 2021-08-31T10:29:47Z

src/BSON/Binary.c

+	{
+		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);
+	}


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?

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.

alcaeus · 2021-08-31T10:32:09Z

src/BSON/Binary.c

+ZEND_BEGIN_ARG_INFO_EX(ai_Binary___unserialize, 0, 0, 1)
+	ZEND_ARG_ARRAY_INFO(0, data, 0)
+ZEND_END_ARG_INFO()


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

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.

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.

alcaeus · 2021-08-31T10:34:48Z

src/BSON/DBPointer.c

+		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);


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).

Created PHPC-1962 to track this.

alcaeus · 2021-08-31T10:37:49Z

src/BSON/DBPointer.c

+/* {{{ 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));
+} /* }}} */


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...

src/MongoDB/WriteConcern.c

alcaeus · 2021-08-31T10:47:23Z

tests/bson/bson-binary-serialization_error-004.phpt

+
+echo throws(function() {
+    unserialize('O:19:"MongoDB\BSON\Binary":1:{s:4:"data";s:6:"foobar";}');
+}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";


I know many of these files don't do this yet, but we should be using the ::class constant. No need to change this in these files as they're most likely copypasta from the originals, so I'll see if I can do a nice find/replace to take care of these.

It would likely require a sprintf since we'd also need to capture the string length in the preceding integer. I'll leave this to you to make a ticket if you'd like to track the work.

jmikola · 2021-08-31T17:34:45Z

Noted one test failure on AppVeyor for 32-bit CursorId var_dump() output. Fixed in the most recent push and will wait for green CI before merging.

jmikola force-pushed the phpc-1849 branch 2 times, most recently from 86ae7c3 to b3d1920 Compare August 17, 2021 16:58

jmikola added 13 commits August 17, 2021 16:48

Binary tests

b98b7f3

DBPointer tests

a3691fa

Decimal128 tests

204a079

Int64 tests

5955d46

Javascript tests

e7a69c1

MaxKey and MinKey tests

dccb51f

ObjectId tests

d73a847

Regex tests

6f1e573

Symbol tests

2371be8

Timestamp tests

1dbff4d

Undefined tests

e61b29b

UTCDateTime tests

29f6983

jmikola force-pushed the phpc-1849 branch from b3d1920 to 29f6983 Compare August 17, 2021 20:48

jmikola added 7 commits August 17, 2021 18:00

CursorId tests

940ce50

ServerApi tests

d9009c0

ReadConcern tests

598615f

ReadPreference tests

5dae132

WriteConcern tests

c1ee5d7

Explicitly handle serialize output in php_phongo_cursorid_get_propert…

aff6c8c

…ies_hash

Explicitly handle serialize output in php_phongo_writeconcern_get_pro…

08ba376

…perties_hash

jmikola force-pushed the phpc-1849 branch from a4b058c to 08ba376 Compare August 17, 2021 22:23

Note maxStalenessSeconds handling in php_phongo_readpreference_get_pr…

f59b619

…operties_hash

jmikola marked this pull request as ready for review August 18, 2021 13:35

jmikola requested review from alcaeus and tanlisu August 18, 2021 13:35

jmikola commented Aug 18, 2021

View reviewed changes

Fix __unserialize protos

e0de6c9

jmikola commented Aug 18, 2021

View reviewed changes

tests/bson/bson-utcdatetime-serialization_error-004.phpt Show resolved Hide resolved

alcaeus approved these changes Aug 31, 2021

View reviewed changes

jmikola force-pushed the phpc-1849 branch from e0a61b2 to a8533e5 Compare August 31, 2021 17:34

CursorId should always serialize id as string

6a4f475

jmikola force-pushed the phpc-1849 branch from a8533e5 to 6a4f475 Compare August 31, 2021 22:35

jmikola merged commit bd10948 into mongodb:master Sep 1, 2021

jmikola deleted the phpc-1849 branch September 1, 2021 02:13

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

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

Uh oh!

Conversation

jmikola commented Aug 10, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alcaeus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola commented Aug 31, 2021

Uh oh!

Uh oh!