Skip to content

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented Sep 9, 2015

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really wasn't intentional.

I would remove this check from the test, and make a totally new testcase for the exact ticket.

@bjori
Copy link
Contributor

bjori commented Sep 9, 2015

I don't think this is a good idea. Silently serializing objects as empty?
Its a clear data loss at least without any indication of the error.

I would rather see an exception thrown.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't make a whole lot of sense to me.

php -r '$d = new DateTime("now"); var_dump($d);'                                                                                                                                                                     <<<
object(DateTime)#1 (3) {
  ["date"]=>
  string(26) "2015-09-09 16:13:49.000000"
  ["timezone_type"]=>
  int(3)
  ["timezone"]=>
  string(19) "America/Los_Angeles"
}

Why wouldn't at least the public properties be serialized?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The output is in fact right. It serializes the object's real properties, of which DateTime has none. This is also the output that matches what Hippo does. DateTime only creates those when you make a var_dump() or use the debug handler.... (yes, that is odd).

I don't think however think it's nice to serialize as some random object... hence PHPC-410 makes sense (and #103)

@jmikola
Copy link
Member Author

jmikola commented Sep 10, 2015

I don't think this is a good idea. Silently serializing objects as empty? Its a clear data loss at least without any indication of the error. I would rather see an exception thrown.

As @derickr mentioned, #103 is my proposed change for that. I just broke these into separate PRs to address the issue Derick raised in PHPC-408 independently.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought you were going to throw an exception in this case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/mongodb-labs/mongo-php-driver-prototype/pull/103/files#diff-f7d29a680148f52d6601f59ed787f577R712 throws an exception for an unsupported BSON Type, but any other object will ideally get its public properties serialized.

@jmikola jmikola merged commit 16e28a9 into mongodb:master Sep 14, 2015
jmikola added a commit that referenced this pull request Sep 14, 2015
@jmikola jmikola deleted the phpc-408 branch September 14, 2015 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants