Skip to content

PHPC-640: Create interfaces for BSON types classes #621

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 4, 2017

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented Jul 31, 2017

https://jira.mongodb.org/browse/PHPC-640
https://jira.mongodb.org/browse/PHPC-988

These interfaces are implemented by the corresponding BSON classes and intended to be used by TypeWrapper implementations.

TODO:

  • Tests to ensure that each BSON type class implements the appropriate interface
  • Tests to demonstrate how we expect TypeWrapper implementations to use these interfaces

@jmikola jmikola requested a review from derickr July 31, 2017 21:36
Copy link
Contributor

@derickr derickr left a comment

Choose a reason for hiding this comment

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

Looks good so far.

@jmikola jmikola force-pushed the phpc-640 branch 4 times, most recently from 6803c0e to f6650bb Compare August 2, 2017 21:19
$myDecimal = new MyDecimal128('1234.5678');

echo "\nTesting MyDecimal128 and Decimal128 are equivalent\n";
var_dump((string) $decimal === (string) $myDecimal);
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: __toString() is the only way to obtain the value of Decimal128 and ObjectID objects. The method is merely a convenience for Binary, Javascript, Regex, Timestamp, and UTCDateTime (note: it is the easiest way to obtain a UTCDateTime's milliseconds). It does not apply at all for MaxKey and MinKey.

I originally decided not to add it to the type-specific interfaces, as I didn't like the idea of incorporating a magic method into an interface (one could say the same about __debugInfo() if PHPC-470 was already merged). However, I'm not wondering if it makes sense to add it. I also purposefully omitted JsonSerializable and Serializable, am plan to simply suggest that users also implement those in their custom wrapper classes if desired.

Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed this while doing the HHVM versions, and this is what the conclusion was I believe: https://github.com/mongodb/mongo-hhvm-driver/blob/master/ext_mongodb.php#L1182

Copy link
Member Author

Choose a reason for hiding this comment

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

Added __toString() to all interfaces except MinKey and MaxKey.

throw new InvalidArgumentException('Cannot create MyUTCDateTime from ' . get_class($type));
}

return new self((string) $type);
Copy link
Member Author

Choose a reason for hiding this comment

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

A string cast is necessary to avoid UTCDateTime::__construct() throwing an exception because its object argument is not a DateTime instance. This issue didn't come up in the Decimal128 and ObjectID tests, as we only parse the argument as a string and PHP happily calls __toString() on the object argument for us.

I don't think this is anything we need to fix, as this is all consistent with UTCDateTime's documentation and the Decimal128 and ObjectID behavior is just a happy coincidence.

@jmikola jmikola changed the title [WIP] PHPC-640: Create interfaces for BSON types classes PHPC-640: Create interfaces for BSON types classes Aug 2, 2017
@jmikola
Copy link
Member Author

jmikola commented Aug 2, 2017

/ping @alcaeus, @malarzm: Let me know if this seems useful for ODM 2.0.

These interfaces are implemented by the corresponding BSON classes and intended to be used by TypeWrapper implementations.
If the userland function throws, we should return early and avoid assigning the uninitialized return value (zwrapper) to zchild. On PHP 5.x, such an assignment could cause a segfault.
Copy link
Contributor

@derickr derickr left a comment

Choose a reason for hiding this comment

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

LGTM

@jmikola jmikola merged commit ef4241c into mongodb:master Aug 4, 2017
jmikola added a commit that referenced this pull request Aug 4, 2017
@jmikola jmikola deleted the phpc-640 branch August 4, 2017 14:04
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.

2 participants