BSON serialization spec compliance #64

jmikola · 2015-07-27T20:34:59Z

jmikola · 2015-07-27T20:50:20Z

src/bson.c

+				int classname_free = 0;
+
+				if (Z_TYPE_P(obj_data) == IS_OBJECT) {
+					classname_free = !zend_get_object_classname(obj_data, &classname, &classname_len TSRMLS_CC);


@bjori: I noticed you were Z_OBJCE_P(object)->name directly when creating the __pclass value. I found zend_get_object_classname() used in core in several places where a class name needed to be reported, so I assume it's fine to use for this exception message.

I can't imagine why folks would want to report a different class name, but I expect the ODM behavior is fine sticking to the name value from the struct.

zend_get_object_classname has been removed in PHP-7.
Please continue to use Z_OBJCE_P(object)->name

jmikola · 2015-07-28T01:02:37Z

@derickr: Feel free to take a look at this and my test changes. I was able to get the 5.3 build working (although I still seem some local failures about missing OpenSSL types), so I'll plan to merge this in tomorrow.

derickr · 2015-07-28T08:59:51Z

config.m4

+
+    if test "$PHP_MONGODB_FOUND_VERNUM" -ge "50400"; then
+      PHP_CHECK_GCC_ARG(-Wno-cast-qual,                 _MAINTAINER_CFLAGS="$_MAINTAINER_CFLAGS -Wno-cast-qual")
+    fi


You shouldn't do this. Instead, fix the consts.

Just for 5.3?

5.4+ rightfully lists the args as const char *, so there's no need there.

I'm fine with this.
Just for reference, which exact case prompted this change?
Isn't it also valid for the arginfos?

arginfos appear to be declared as static const in PHP 5.3. I only saw these warnings with zend_parse_parameters() and possibly one other macro (not arginfo).

For reference, here are a few of these errors from 5.3:

/home/jmikola/workspace/mongodb/phpc/src/bson.c: In function ‘zif_fromPHP’: /home/jmikola/workspace/mongodb/phpc/src/bson.c:967:55: warning: passing argument 3 of ‘zend_parse_parameters’ discards ‘const’ qualifier from pointer target type if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "A", &data) == FAILURE) { ^ In file included from /home/jmikola/bin/php-bin/php-5.3/include/php/main/php.h:38:0, from /home/jmikola/workspace/mongodb/phpc/src/bson.c:29: /home/jmikola/bin/php-bin/php-5.3/include/php/Zend/zend_API.h:245:14: note: expected ‘char *’ but argument is of type ‘const char *’ ZEND_API int zend_parse_parameters(int num_args TSRMLS_DC, char *type_spec, ...); ^ /home/jmikola/workspace/mongodb/phpc/src/bson.c:974:1: warning: passing argument 3 of ‘_estrndup’ discards ‘const’ qualifier from pointer target type RETVAL_STRINGL((const char *) bson_get_data(bson), bson->len, 1); ^

A full log of the stderr output from a 5.3 build is here: https://gist.github.com/jmikola/e3a3f7da387333187864

We don't care any more, as we can drop 5.3 :-D

bjori · 2015-07-28T17:18:12Z

src/bson.c

+					classname_free = !zend_get_object_classname(obj_data, &classname, &classname_len TSRMLS_CC);
+				}
+
+				phongo_throw_exception(PHONGO_ERROR_UNEXPECTED_VALUE TSRMLS_CC, "Expected %s() to return an array or stdClass, %s given", BSON_SERIALIZE_FUNC_NAME, (classname_len ? classname : zend_get_type_by_const(Z_TYPE_P(obj_data))));


You should probably include the classname too.
There are plenty matches for the function name so it doesn't help mutch having that in the error message without the classname

Per the serialization spec, we should check that the class (1) exists, (2), is instantiatable, and (3) implements Unserializable. Additionally, this refactors some code duplication out of apply_classname_to_state(). We should also always check to free classname, since an empty string might have been allocated (i.e. classname_len is zero).

ODM behavior should override only the default type map and named class. Array and stdClass object modes still take precedence. The PHONGO_TYPEMAP_CLASS switch cases were simplified, since we can trust that the root/document or ODM class implements Unserializable (thanks to php_phongo_bson_typemap_to_state() and php_phongo_bson_visit_binary(), respectively).

This revises the UnexpectedValueException message for an invalid return value (PHPC-331). Additionally, we consolidated the previous BSON-encoding error tests.

bjori · 2015-07-28T21:10:24Z

src/bson.c

@@ -31,6 +31,10 @@
 #include <Zend/zend_hash.h>
 #include <Zend/zend_interfaces.h>

+#if PHP_VERSION_ID >= 50400
+# include <Zend/zend_string.h>
+#endif


I feel this should really be in the compat header as a #else condition for the str_efree() ?
Or is that pushing it a bit far?

I think that'd be neater too.

bjori · 2015-07-28T21:16:04Z

I'm not sure why so many unrelated things are in the same PR!

I haven't read the latest and greatest "BSON serialization spec" so I don't know if this does indeed confirm to it, or makes actual sense.

The code itself looks good to me though.

jmikola · 2015-07-29T15:18:48Z

I'm not sure why so many unrelated things are in the same PR!

I created half of the tickets after starting PHPC-329 and most of the code was inter-dependent.

jmikola force-pushed the phpc-329 branch from 3198091 to 2e6eb07 Compare July 27, 2015 20:46

jmikola reviewed Jul 27, 2015
View reviewed changes

jmikola force-pushed the phpc-329 branch 3 times, most recently from 0cde2cf to 2fcae11 Compare July 28, 2015 00:44

jmikola mentioned this pull request Jul 28, 2015

Invalid type map class throws InvalidArgumentException mongodb/mongo-hhvm-driver#25

Merged

derickr reviewed Jul 28, 2015
View reviewed changes

This was referenced Jul 28, 2015

Fixed syntax for PHP 5.3 #65

Closed

Fixes due to some inconsistencies in HHVM vs PHP #66

Merged

bjori reviewed Jul 28, 2015
View reviewed changes

jmikola mentioned this pull request Jul 28, 2015

PHPC-334: Split hex_dump() test case #67

Merged

jmikola added 8 commits July 28, 2015 14:50

PHPC-336: BSON\Binary debug handler

802e2ae

PHPC-329: __pclass must be both instantiatable and Persistable

2551570

PHPC-331: Tests for BSON deserialization errors

2824214

PHPC-343: Allow bsonSerialize() to return a stdClass

dea4ff9

This revises the UnexpectedValueException message for an invalid return value (PHPC-331). Additionally, we consolidated the previous BSON-encoding error tests.

PHPC-344: Allow non-root Serializable to encode as BSON array

c5eb198

Compile with no-cast-qual for PHP 5.3

80b9f3e

jmikola force-pushed the phpc-329 branch from 2fcae11 to ea4836b Compare July 28, 2015 19:38

bjori reviewed Jul 28, 2015
View reviewed changes

Define str_efree() macro for PHP 5.3 compatibility

bfb1b86

PHP 5.3 compatibility for tools.php

888b5dc

jmikola force-pushed the phpc-329 branch from ea4836b to 888b5dc Compare July 29, 2015 15:24

jmikola merged commit 888b5dc into mongodb:master Jul 29, 2015

jmikola added a commit that referenced this pull request Jul 29, 2015

Merge pull request #64

9d7bb9a

jmikola deleted the phpc-329 branch July 29, 2015 15:59

BSON serialization spec compliance #64

BSON serialization spec compliance #64

Uh oh!

Conversation

jmikola commented Jul 27, 2015

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!

jmikola commented Jul 28, 2015

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!

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!

bjori commented Jul 28, 2015

Uh oh!

jmikola commented Jul 29, 2015

Uh oh!

Uh oh!