PHPC-1000: Allow bsonSerialize to return BSON structure #1449
Conversation
The previously indicated return type was incorrect, as returning an object other than stdClass would result in an error.
src/phongo_bson_encode.c
Outdated
| PHONGO_ZVAL_CLASS_OR_TYPE_NAME(obj_data)); | ||
| zval_ptr_dtor(&obj_data); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| is_array = php_phongo_is_array_or_document(&obj_data) == IS_ARRAY || (Z_TYPE(obj_data) == IS_OBJECT && instanceof_function(Z_OBJCE(obj_data), php_phongo_packedarray_ce)); |
There was a problem hiding this comment.
The doc block for php_phongo_is_array_or_document() states:
Determines whether the argument should be serialized as a BSON array or document.
Would it make more sense to modify that function to return IS_ARRAY for PackedArray?
Prior to this PR I see two usages of the function, one which handles IS_OBJECT and one for IS_ARRAY. You're modifying the IS_OBJECT case in this PR, so that would leave the second usage in php_phongo_bson_append for IS_ARRAY to be updated (PackedArray is currently handled in the IS_OBJECT case of the surrounding switch).
There was a problem hiding this comment.
I've changed the function to return IS_ARRAY for PackedArray instances and updated the case in php_phongo_bson_append to only handle zvals of type IS_ARRAY if they also fulfil the php_phongo_is_array_or_document check.
alcaeus
force-pushed
the
phpc-1000-serializable-yield-bson
branch
from
68e12f3 to
b4fb869
Compare
| @@ -141,7 +164,7 @@ static void php_phongo_bson_append_object(bson_t* bson, php_phongo_field_path* f | |||
|
|
|||
| /* Persistable objects must always be serialized as BSON documents; | |||
| * otherwise, infer based on bsonSerialize()'s return value. */ | |||
| if (instanceof_function(Z_OBJCE_P(object), php_phongo_persistable_ce) || php_phongo_is_array_or_document(&obj_data) == IS_OBJECT) { | |||
| if (instanceof_function(Z_OBJCE_P(object), php_phongo_persistable_ce) || php_phongo_is_array_or_document(&obj_data) != IS_ARRAY) { | |||
There was a problem hiding this comment.
I think it'd also help for php_phongo_is_array_or_document to assert that only an object or array is passed in. We don't really have to worry about this thanks to the explicit checks on obj_data above, but it'd be good for peace of mind. I'll make a note of this in PHPC-2217 as well.
alcaeus
force-pushed
the
phpc-1000-serializable-yield-bson
branch
from
b4fb869 to
3c01dd2
Compare
| BSON_SERIALIZE_FUNC_NAME, | ||
| PHONGO_ZVAL_CLASS_OR_TYPE_NAME(obj_data)); | ||
| if (!phongo_check_bson_serialize_return_type(&obj_data, Z_OBJCE_P(object))) { | ||
| // Exception already thrown |
There was a problem hiding this comment.
@alcaeus: I see this was already merged, but note that phongo_check_bson_serialize_return_type() can technically return false without throwing an exception if the object is neither Serializable nor Persistable. Perhaps impossible in practice, but I wanted to point it out since this seems like undefined behavior.
There was a problem hiding this comment.
Good point - taken care of in #1453
| return true; | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
Quoting #1449 (comment):
Consider throwing PHONGO_ERROR_UNEXPECTED_VALUE since we shouldn't get here, but if you want to just return true here you can remove the true returns above and use if/else instead.
If we return false, we should absolutely throw to follow conventions. Otherwise, just return true and NOP.
PHPC-1000
This PR allows implementations of
MongoDB\BSON\Serializableto return instances ofMongoDB\BSON\DocumentandMongoDB\BSON\PackedArray. This allows for an easier implementation of theSerializablein PHPLIB classes.The first commit fixes the tentative return type for
Serializable::bsonSerialize: while the original type wasarray|object, the code actually rejected any object that was not an instance ofstdClass. I've also tested the impact on this change: on PHP < 8.1 there is no change as tentative return types are only reported in PHP 8.1+. In those versions, users that have a wider return type (e.g. the originalarray|objecttype orobjectin general), will see a deprecation message that the return types should be compatible. There is no hard break until we make the type non-tentative in PHPC 2.0, so I thought I'd preempt this and fix the return type. This then allows us to specify a return type ofarray|stdClass|Document|PackedArrayin the last commit, which once again is consistent with the values the serializer accepts as return value.