PHPLIB-516: Append PHPLIB version information to handshake data #731
Conversation
There was a problem hiding this comment.
A short note on testing: the only way to test this would be to enable the MongoDB debug log and check the handshake data. Even then, we can only check if ext-mongodb gets the additional data for the handshake, as it can't be changed after the first connection due to a limitation in libmongoc. If there are any suggestions on how to efficiently test this, please let me know.
| @@ -13,7 +13,8 @@ | |||
| "php": "^7.0", | |||
| "ext-hash": "*", | |||
| "ext-json": "*", | |||
| "ext-mongodb": "^1.8" | |||
| "ext-mongodb": "^1.8", | |||
| "jean85/pretty-package-versions": "^1.2" | |||
There was a problem hiding this comment.
This provides a better interface around version strings returned by ocramius/package-versions. It is used to provide the current version of this library without having to maintain it in a version constant.
| { | ||
| if (self::$version === null) { | ||
| try { | ||
| self::$version = PrettyVersions::getVersion('mongodb/mongodb')->getPrettyVersion(); |
There was a problem hiding this comment.
While the package-versions package caches package information when they are installed, I thought it would be best to still cache this data to ensure we're not unnecessarily causing delays on connecting.
| } | ||
|
|
||
| foreach ($this->getInvalidStringValues() as $value) { | ||
| $options[][] = ['driver' => ['version' => $value]]; |
There was a problem hiding this comment.
Should we also test platform here?
There was a problem hiding this comment.
Added, but requires a little extra work since we only pass platform information through to the driver.
There was a problem hiding this comment.
Oh, I didn't realize that when I left my first comment. I see now that PHPLIB is only throwing its own InvalidArgumentException for the two options it merges. Thanks for adding this just the same.
| try { | ||
| self::$version = PrettyVersions::getVersion('mongodb/mongodb')->getPrettyVersion(); | ||
| } catch (OutOfBoundsException $e) { | ||
| return 'unknown'; |
There was a problem hiding this comment.
I took a look through PrettyVersions and it seems that it doesn't throw anything on its own, so I assume this comes from Ocramius' library. If so, I think there's also a case where UnexpectedValueException could be thrown. Would it be safer to just catch any RuntimeException?
Also, I'm totally on board with not exposing the original exception. I don't think "fix your shit" would go over well with users, especially when it might not be clear which library is generating that message 😆
There was a problem hiding this comment.
Changed to catch any throwable. That exception message (along with not wanting to break a connection to MongoDB because we couldn't determine the version) was the main reason for adding a try-catch block.
| 'version' => '1.2.3-dev', | ||
| 'platform' => 'some-platform', | ||
| ] | ||
| .. versionadded:: 1.7 |
There was a problem hiding this comment.
Should we add a note that this is best used for things like wrapping libraries and ODMs rather than applications?
| } | ||
|
|
||
| foreach ($this->getInvalidStringValues() as $value) { | ||
| $options[][] = ['driver' => ['version' => $value]]; |
There was a problem hiding this comment.
Oh, I didn't realize that when I left my first comment. I see now that PHPLIB is only throwing its own InvalidArgumentException for the two options it merges. Thanks for adding this just the same.
https://jira.mongodb.org/browse/PHPLIB-516