PHPC-1027: Introduce classes for deprecated BSON types #700

derickr · 2017-12-11T17:06:11Z

No description provided.

jmikola · 2017-12-12T05:50:24Z

php_phongo.c

@@ -2377,6 +2377,33 @@ void php_phongo_new_regex_from_regex_and_options(zval *object, const char *patte
 	qsort((void *) intern->flags, intern->flags_len, 1, php_phongo_regex_compare_flags);
 } /* }}} */

+void php_phongo_new_symbol_from_symbol(int init, zval *object, const char *symbol, size_t symbol_len TSRMLS_DC) /* {{{ */


Two questions about the init argument. It is apparently treated as a boolean, so bool would be more appropriate than int; however, I only see this function called when init is 1, so does it and the conditional below really serve a purpose? The other php_phongo_new_X functions don't take such an argument an always call object_init_ex().

One of them must have done, as I've copied it :-)

I'll change it.

jmikola · 2017-12-12T05:52:14Z

php_phongo.c

+	intern->symbol_len = symbol_len;
+} /* }}} */
+
+void php_phongo_new_dbpointer_from_dbpointer(int init, zval *object, const char *namespace, size_t namespace_len, const bson_oid_t *oid TSRMLS_DC) /* {{{ */


If there is only going to be one php_phongo_new_X function per deprecated type, I think we can omit the "from_X" suffix for conciseness. It appears to be a historical relic, as newer functions such as php_phongo_new_decimal128() do not have a "from" component in the name.

OK, will fix that too.

jmikola · 2017-12-12T05:58:54Z

src/BSON/DBPointer.c

+	zval *namespace, *oid;
+
+	if ((namespace = zend_hash_str_find(props, "namespace", sizeof("namespace")-1)) && Z_TYPE_P(namespace) == IS_STRING &&
+		(oid = zend_hash_str_find(props, "oid", sizeof("oid")-1)) && Z_TYPE_P(oid) == IS_OBJECT && instanceof_function(Z_OBJCE_P(oid), php_phongo_objectid_interface_ce TSRMLS_CC)) {


Objections to mixing tabs and spaces not withstanding, current code format uses tabs for block indents and spaces to align multi-line parens (e.g. if conditions). This is using only tabs.

I had missed that. Will fix.

jmikola · 2017-12-12T06:02:35Z

src/BSON/DBPointer.c

+static PHP_METHOD(DBPointer, __toString)
+{
+	php_phongo_dbpointer_t *intern;
+	char                    *retval;


Alignment is off by a space here.

jmikola · 2017-12-12T06:02:50Z

src/BSON/DBPointer.c

+} /* }}} */
+
+/* {{{ proto string MongoDB\BSON\DBPointer::__toString()
+   Return the DBPointer's namespace string. */


"namespace string and ObjectId"

jmikola · 2017-12-12T06:13:24Z

tests/bson/bson-dbpointer-jsonserialize-001.phpt

@@ -0,0 +1,17 @@
+--TEST--
+MongoDB\BSON\Javascript::jsonSerialize() return value (without scope)


I assume this was copy/pasted prematurely, since DBPointer::jsonSerialize() isn't implemented yet (although the other jsonSerialize() methods appear to be).

Yes, and this is also what causes my current crash. It's required as part of one of the interfaces.

jmikola · 2017-12-12T06:14:09Z

tests/bson/bson-dbpointerinterface-001.phpt

@@ -0,0 +1,14 @@
+--TEST--
+MongoDB\BSON\JavascriptInterface is implemented by MongoDB\BSON\Javascript


I don't think we'll need interfaces for any of these deprecated classes.

I know. I haven't removed this test yet, as I had done for Symbol and Undefined.

jmikola · 2017-12-12T06:15:18Z

tests/bson/bson-symbol-compare-001.phpt

+--FILE--
+<?php
+
+var_dump(MongoDB\BSON\toPHP(MongoDB\BSON\fromJSON('{ "undefined": {"$symbol": "val1"} }')) == MongoDB\BSON\toPHP(MongoDB\BSON\fromJSON('{ "undefined": {"$symbol": "val1"} }')));


"undefined" is a curious choice here. Something like "x" or "foo" might be more concise, and not cause any confusion with the actual Undefined type.

Edit: I noticed you used "symbol" in a later test. That certainly works.

Right, I've missed this one. Will fix it.

jmikola · 2017-12-12T06:16:41Z

tests/bson/bson-symbol-set_state-001.phpt

+<?php
+
+var_export(MongoDB\BSON\Symbol::__set_state([
+	'symbol' => "symbolValue",


@derickr: I'll see your 👎 and raise you a:

jmikola · 2017-12-12T06:17:34Z

tests/bson/bson-toPHP_error-005.phpt

@@ -3,9 +3,7 @@ MongoDB\BSON\toPHP(): BSON decoding ignores unsupported BSON types
 --FILE--
 <?php
 $tests = [
-    pack('VCa*xx', 10, 0x06, 'foo'), // undefined
    pack('VCa*xVa*xx12x', 37, 0x0C, 'foo', 11, 'collection'), // DBPointer


I assume this entire test will ultimately need to be deleted.

derickr

Updated for all the comments, and finished DBPointer.

Only questions I have is whether you think the serialisation formats are good.

derickr · 2017-12-12T10:28:31Z

php_phongo.c

@@ -2377,6 +2377,33 @@ void php_phongo_new_regex_from_regex_and_options(zval *object, const char *patte
 	qsort((void *) intern->flags, intern->flags_len, 1, php_phongo_regex_compare_flags);
 } /* }}} */

+void php_phongo_new_symbol_from_symbol(int init, zval *object, const char *symbol, size_t symbol_len TSRMLS_DC) /* {{{ */


One of them must have done, as I've copied it :-)

I'll change it.

derickr · 2017-12-12T10:28:53Z

php_phongo.c

+	intern->symbol_len = symbol_len;
+} /* }}} */
+
+void php_phongo_new_dbpointer_from_dbpointer(int init, zval *object, const char *namespace, size_t namespace_len, const bson_oid_t *oid TSRMLS_DC) /* {{{ */


OK, will fix that too.

derickr · 2017-12-12T10:29:38Z

src/BSON/DBPointer.c

+	zval *namespace, *oid;
+
+	if ((namespace = zend_hash_str_find(props, "namespace", sizeof("namespace")-1)) && Z_TYPE_P(namespace) == IS_STRING &&
+		(oid = zend_hash_str_find(props, "oid", sizeof("oid")-1)) && Z_TYPE_P(oid) == IS_OBJECT && instanceof_function(Z_OBJCE_P(oid), php_phongo_objectid_interface_ce TSRMLS_CC)) {


I had missed that. Will fix.

derickr · 2017-12-12T10:29:50Z

src/BSON/DBPointer.c

+} /* }}} */
+
+/* {{{ proto string MongoDB\BSON\DBPointer::__toString()
+   Return the DBPointer's namespace string. */


derickr · 2017-12-12T10:30:50Z

src/BSON/DBPointer.c

+*/
+static PHP_METHOD(DBPointer, serialize)
+{
+	php_phongo_dbpointer_t  *intern;


It was off in other BSON class files too, I think. Didn't want to change them though just for the sake of it, but in new code it should done good.

derickr · 2017-12-12T10:32:00Z

tests/bson/bson-dbpointer-jsonserialize-001.phpt

@@ -0,0 +1,17 @@
+--TEST--
+MongoDB\BSON\Javascript::jsonSerialize() return value (without scope)


Yes, and this is also what causes my current crash. It's required as part of one of the interfaces.

derickr · 2017-12-12T10:32:41Z

tests/bson/bson-dbpointerinterface-001.phpt

@@ -0,0 +1,14 @@
+--TEST--
+MongoDB\BSON\JavascriptInterface is implemented by MongoDB\BSON\Javascript


I know. I haven't removed this test yet, as I had done for Symbol and Undefined.

derickr · 2017-12-12T10:33:10Z

tests/bson/bson-symbol-compare-001.phpt

+--FILE--
+<?php
+
+var_dump(MongoDB\BSON\toPHP(MongoDB\BSON\fromJSON('{ "undefined": {"$symbol": "val1"} }')) == MongoDB\BSON\toPHP(MongoDB\BSON\fromJSON('{ "undefined": {"$symbol": "val1"} }')));


Right, I've missed this one. Will fix it.

derickr · 2017-12-12T10:33:31Z

tests/bson/bson-toPHP_error-005.phpt

@@ -3,9 +3,7 @@ MongoDB\BSON\toPHP(): BSON decoding ignores unsupported BSON types
 --FILE--
 <?php
 $tests = [
-    pack('VCa*xx', 10, 0x06, 'foo'), // undefined
    pack('VCa*xVa*xx12x', 37, 0x0C, 'foo', 11, 'collection'), // DBPointer


jmikola · 2017-12-12T14:34:11Z

src/BSON/Symbol.c

+ZEND_END_ARG_INFO()
+
+static zend_function_entry php_phongo_symbol_me[] = {
+	PHP_ME(Symbol, __set_state, ai_Symbol___set_state, ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)


Does it make sense to implement this method instead of disabling it as we do the constructor?

With this gone, that would leave unserialize() as the only way for users to manually create these deprecated objects. I'm not suggesting we remove unserialize(), as folks should be able to round-trip these classes through PHP serialization :)

Removed the __set_state implementation all together.

jmikola

I believe DBPointer::__toString() is missing, but other things look good.

Now that these types exist, I think we can enable some of the deprecated tests in the BSON corpus. I created PHPC-1065 to track that.

jmikola · 2017-12-12T16:59:39Z

src/BSON/DBPointer.c

+
+	array_init_size(return_value, 2);
+	ADD_ASSOC_STRINGL(return_value, "$ref", intern->ref, intern->ref_len);
+	ADD_ASSOC_STRING(return_value, "$id", intern->id);


I just realized that the extended JSON spec calls for: {"$dbPointer": {"$ref": <namespace as a string>, "$id": ObjectId}} to avoid confusion with DBRef notation. Can we wrap this in an outer array with a $dbPointer key?

The spec also shows { "$oid": "57e193d7a9cc81b4027498b1" } for the value of the $id field, so I think that string should be wrapped as well.

derickr requested a review from jmikola December 11, 2017 17:06

jmikola reviewed Dec 12, 2017

View reviewed changes

derickr force-pushed the PHPC-1027-add-classes-for-deprecated-bson-types branch from 257342d to ff7b852 Compare December 12, 2017 12:55

derickr changed the title ~~[WIP] PHPC-1027: Introduce classes for deprecated BSON types~~ PHPC-1027: Introduce classes for deprecated BSON types Dec 12, 2017

derickr commented Dec 12, 2017

View reviewed changes

jmikola reviewed Dec 12, 2017

View reviewed changes

derickr force-pushed the PHPC-1027-add-classes-for-deprecated-bson-types branch from ff7b852 to f259e19 Compare December 12, 2017 15:08

jmikola reviewed Dec 12, 2017

View reviewed changes

PHPC-1027: Introduce classes for deprecated BSON types (Undefined)

c780a27

derickr force-pushed the PHPC-1027-add-classes-for-deprecated-bson-types branch 2 times, most recently from 64a82bc to 8eae9ce Compare December 13, 2017 17:07

derickr added 2 commits December 14, 2017 10:49

PHPC-1027: Introduce classes for deprecated BSON types (Symbol)

a9f6e3b

PHPC-1027: Introduce classes for deprecated BSON types (DBPointer)

d3953f6

derickr force-pushed the PHPC-1027-add-classes-for-deprecated-bson-types branch from 8eae9ce to d3953f6 Compare December 14, 2017 10:49

jmikola approved these changes Dec 14, 2017

View reviewed changes

derickr merged commit d3953f6 into mongodb:master Dec 14, 2017

derickr added a commit that referenced this pull request Dec 14, 2017

Merged pull request #700

2be8af5

		@@ -0,0 +1,17 @@
		--TEST--
		MongoDB\BSON\Javascript::jsonSerialize() return value (without scope)

		@@ -0,0 +1,14 @@
		--TEST--
		MongoDB\BSON\JavascriptInterface is implemented by MongoDB\BSON\Javascript

PHPC-1027: Introduce classes for deprecated BSON types #700

PHPC-1027: Introduce classes for deprecated BSON types #700

Uh oh!

Conversation

derickr commented Dec 11, 2017

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!

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!

jmikola Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

derickr left a comment

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!

Choose a reason for hiding this comment

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

jmikola Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmikola Dec 12, 2017 •

edited

Loading

jmikola Dec 12, 2017 •

edited

Loading