Skip to content

PHPC-554: Rephrase unsupported/corrupt BSON messages #306

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 1 commit into from
Apr 25, 2016

Conversation

derickr
Copy link
Contributor

@derickr derickr commented Apr 22, 2016

* been thrown already (due to an unsupported BSON type for example,
* don't overwrite with a generic exception message. */
if (!EG(exception)) {
phongo_throw_exception(PHONGO_ERROR_UNEXPECTED_VALUE TSRMLS_CC, "Detected corrupt BSON data.");
Copy link
Member

Choose a reason for hiding this comment

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

I realize you used the same message in mongodb/mongo-hhvm-driver@1d51fe9, but should there really be a period after this message? We typically don't use that, and only did so in the corruption message since it consists of two sentences.

@jmikola
Copy link
Member

jmikola commented Apr 22, 2016

LGTM apart from the question on the exception message format.

@derickr derickr force-pushed the PHPC-554-unsupported-corrupt-bson branch from 324e242 to a04008e Compare April 25, 2016 09:35
@derickr derickr merged commit a04008e into mongodb:master Apr 25, 2016
derickr added a commit that referenced this pull request Apr 25, 2016
@@ -35,13 +35,13 @@ foreach ($tests as $bson) {
<?php exit(0); ?>
--EXPECTF--
OK: Got MongoDB\Driver\Exception\UnexpectedValueException
Could not convert BSON document to a PHP variable
Detected corrupt BSON data.
Copy link
Member

Choose a reason for hiding this comment

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

I believe you forgot to remove the trailing period in these expected exception messages. Fixed in e6deaeb.

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