->[to|from]Array() -> [to|from]PHP(). "root" typemapping. Default "root" to stdclass, not array #56

jmikola · 2015-07-06T06:12:22Z

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

Rename BSON\to|from]Array() to BSON[to|from]PHP()

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

Support setting the "root" (container) value to explicit type.
->setTypemap(["root" => "array"]);

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

Use stdClass, not array, for top-level containers by default

jmikola · 2015-07-08T00:30:13Z

tests/manager/manager-executeQuery-001.phpt

@@ -73,7 +73,7 @@ object(MongoDB\Driver\Cursor)#%d (%d) {
      ["mode"]=>
      int(1)
      ["tags"]=>
-      array(0) {
+      object(stdClass)#%d (0) {


@bjori: When mongoc_read_prefs_t is initialized, its tags are also initialized to an empty BSON document. In turn, php_phongo_read_preference_to_zval() will do:

if (read_prefs->tags.len) { php_phongo_bson_state state = PHONGO_BSON_STATE_INITIALIZER; MAKE_STD_ZVAL(state.zchild); bson_to_zval(bson_get_data(&read_prefs->tags), read_prefs->tags.len, &state); add_assoc_zval_ex(retval, ZEND_STRS("tags"), state.zchild); } else { add_assoc_null_ex(retval, ZEND_STRS("tags")); }

That else condition will really never be reached. Would you rather I change the condition to read_prefs->tags.len > 5 and add a comment that we only dump it if it's a non-empty document? Otherwise, I can leave this as-is.

I think all the else append null checks were all added at the same time (dozens of them) after I realized not everything was initialized everytime, so var_dump()ing had the tendency to crash.

I don't think I ever checked individual cases, but instead always added a fallback path.

I'd say in this particular case, tags = null would look weird. tags = array() looks best.

Why is this stdClass now? that looks weird?

Why is this stdClass now? that looks weird?

I'm changing php_phongo_read_preference_to_zval(), Server::getTags(), Server::getInfo(), and php_phongo_server_to_zval() to use native array decoding, as we discussed earlier.

bjori · 2015-07-08T20:06:58Z

src/bson.c

+	 *
+	 * TODO: decide if native array/object should be allowed to override this,
+	 * in which case we only supersede PHONGO_TYPEMAP_NONE.
+	 */


no code todo please. They are destined to rot.
If its realistic, add a ticket. If not. swallow it :]

Created PHPC-329, so I'll remove this.

bjori · 2015-07-08T22:21:00Z

I've fixed the failing test on travis and am kicking the builds now

Update tests to expect stdClass for documents. Debug handlers and the Server getter methods will still decode documents as native arrays.

jmikola · 2015-07-11T04:39:47Z

@bjori: I cleaned up the cases where I replaced (%d) expectations with hard-coded numbers, but I wasn't about to do a global search-and-replace in this PR. Feel free to create a ticket to do this later. I'm not sure if you just wanted stdClass and array dumps covered, or also things like string lengths.

jmikola · 2015-07-11T05:44:40Z

A few issues still remaining:

PHPC-329: Only allow ODM behavior to supersede unspecified type map (not array/stdClass/custom mode)
PHPC-330: Include __pclass in bsonUnserialize() when possible
PHPC-331: Throw UnexpectedValueExceptions for unsupported (de)serialization cases

bjori · 2015-07-13T20:16:07Z

src/bson.c

@@ -957,7 +979,7 @@ void php_phongo_bson_typemap_to_state(zval *typemap, php_phongo_bson_typemap *ma
 				map->array_type = PHONGO_TYPEMAP_NATIVE_OBJECT;
 			} else {
 				map->array_type = PHONGO_TYPEMAP_CLASS;
-				array_ce = zend_fetch_class(classname, classname_len, ZEND_FETCH_CLASS_AUTO TSRMLS_CC);
+				zend_class_entry *array_ce = zend_fetch_class(classname, classname_len, ZEND_FETCH_CLASS_AUTO TSRMLS_CC);


Variable declarations need to be done first.
This won't compile on VS.. And I thought this was part of our --enable-developer-flags (-Wdeclaration-after-statement).

https://travis-ci.org/10gen-labs/mongo-php-driver-prototype/jobs/70487040#L878

Good catch. I surely must have missed this stderr output when building locally.

bjori · 2015-07-13T20:37:50Z

LGTM

Fix the build
Confirm no new build warnings
Confirm tests/bson/bson-encode-002.phpt

jmikola · 2015-07-14T23:59:36Z

Rebased the PHPC-315 commit to fix the build warnings, and also adjusted my build scripts to I should catch stderr output more easily in the future.

I left an explanation of why I changed the bson-encode-002.phpt test, too. Hopefully that's acceptable.

bjori · 2015-07-15T01:40:58Z

oklgtm

jmikola force-pushed the phpc-319 branch 6 times, most recently from e250088 to 2ad506f Compare July 8, 2015 00:12

jmikola reviewed Jul 8, 2015
View reviewed changes

jmikola force-pushed the phpc-319 branch 2 times, most recently from 4dd8f9e to 3d04657 Compare July 8, 2015 18:41

jmikola changed the title ~~[WIP] PHPC-319: Convert top-level documents to stdClass by default~~ PHPC-319: Convert top-level documents to stdClass by default Jul 8, 2015

bjori reviewed Jul 8, 2015
View reviewed changes

jmikola force-pushed the phpc-319 branch from 3d04657 to 2f2c865 Compare July 8, 2015 20:48

jmikola mentioned this pull request Jul 9, 2015

Added draft spec on how to serialize and deserialize mongodb/mongo-hhvm-driver#6

Merged

jmikola force-pushed the phpc-319 branch from 2f2c865 to 339bf31 Compare July 11, 2015 03:41

jmikola added 3 commits July 10, 2015 23:52

Fix WriteResult::getWriteConcernError() proto

e40e31a

Use plural property name to store list of Address objects

d34a09e

Relax expected class instance in Manager: getServers() test

9448dd8

jmikola force-pushed the phpc-319 branch 2 times, most recently from 06ca6a8 to 7df7934 Compare July 11, 2015 04:36

PHPC-319: Convert top-level documents to stdClass by default

6eb0944

Update tests to expect stdClass for documents. Debug handlers and the Server getter methods will still decode documents as native arrays.

jmikola force-pushed the phpc-319 branch from 7df7934 to ef03e90 Compare July 11, 2015 04:38

bjori reviewed Jul 13, 2015
View reviewed changes

bjori changed the title ~~PHPC-319: Convert top-level documents to stdClass by default~~ ->[to|from]Array() -> [to|from]PHP(). "root" typemapping. Default "root" to stdclass, not array Jul 13, 2015

jmikola added 4 commits July 14, 2015 19:52

PHPC-315: Support "root" type mapping for top-level documents

965f8c3

PHPC-311: Rename from/toArray() functions to from/toPHP()

194bbbd

Fix from/toJSON() protos

04cd180

Use namespace-agnostic BSON (de)serialization functions

1edcd14

jmikola force-pushed the phpc-319 branch from b8d0494 to 1edcd14 Compare July 14, 2015 23:52

jmikola merged commit 1edcd14 into mongodb:master Jul 15, 2015

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

Merge pull request #56

c2b2dcd

jmikola deleted the phpc-319 branch July 15, 2015 19:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

->[to|from]Array() -> [to|from]PHP(). "root" typemapping. Default "root" to stdclass, not array #56

->[to|from]Array() -> [to|from]PHP(). "root" typemapping. Default "root" to stdclass, not array #56

Uh oh!

jmikola commented Jul 6, 2015

Uh oh!

jmikola Jul 8, 2015

Uh oh!

bjori Jul 8, 2015

Uh oh!

jmikola Jul 8, 2015

Uh oh!

bjori Jul 8, 2015

Uh oh!

jmikola Jul 8, 2015

Uh oh!

bjori commented Jul 8, 2015

Uh oh!

jmikola commented Jul 11, 2015

Uh oh!

jmikola commented Jul 11, 2015

Uh oh!

bjori Jul 13, 2015

Uh oh!

bjori Jul 13, 2015

Uh oh!

jmikola Jul 14, 2015

Uh oh!

bjori commented Jul 13, 2015

Uh oh!

jmikola commented Jul 14, 2015

Uh oh!

bjori commented Jul 15, 2015

Uh oh!

Uh oh!

->[to|from]Array() -> [to|from]PHP(). "root" typemapping. Default "root" to stdclass, not array #56

->[to|from]Array() -> [to|from]PHP(). "root" typemapping. Default "root" to stdclass, not array #56

Uh oh!

Conversation

jmikola commented Jul 6, 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!

bjori commented Jul 8, 2015

Uh oh!

jmikola commented Jul 11, 2015

Uh oh!

jmikola commented Jul 11, 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!

bjori commented Jul 13, 2015

Uh oh!

jmikola commented Jul 14, 2015

Uh oh!

bjori commented Jul 15, 2015

Uh oh!

Uh oh!