PHPC-1026: Log warnings instead of throwing for unsupported BSON types #665
Conversation
tests/bson/bson-toPHP_error-006.phpt
Outdated
| @@ -18,45 +18,66 @@ $tests = [ | |||
| pack('VCa*xVCa*xxCa*xVCa*xxx', 31, 0x03, 'e1', 9, 0x06, 'u1', 0x03, 'e2', 9, 0x06, 'u2'), | |||
| ]; | |||
|
|
|||
| ini_set("mongodb.debug", "stdout"); | |||
There was a problem hiding this comment.
The test filename is bson-toPHP_error-006.phpt and the title still says "BSON decoding throws multiple exceptions". I think we should at least rename the title, but I don't feel strongly about the filename (technically, toPHP() is no longer erroring, just silently logging a warning).
Also, this test doesn't do much more than create an array of bytes and call toPHP(). Would it make sense to move mongodb.debug to an --INI-- block instead of using ini_set()? We don't do anything with the driver by including tools.php, so that shouldn't produce any clutter in the logs.
There was a problem hiding this comment.
You still get lots of libmongoc initialisation messages, which we don't need. So I'm keeping that.
I've changed the titles, but not the file names.
tests/bson/bson-toPHP_error-005.phpt
Outdated
| @@ -11,20 +11,29 @@ $tests = [ | |||
| pack('VCa*xVa*xx', 18, 0x0E, 'foo', 4, 'bar'), // symbol | |||
| ]; | |||
|
|
|||
| ini_set("mongodb.debug", "stdout"); | |||
There was a problem hiding this comment.
Same comment as the test below. At least the title should be changed and the ini_set() can possibly be replaced with --INI--.
There was a problem hiding this comment.
It can, but then there as so many other messages that we don't care about. This is better.
| @@ -47,7 +47,7 @@ static bool php_phongo_bson_visit_array(const bson_iter_t *iter ARG_UNUSED, cons | |||
|
|
|||
| static void php_phongo_bson_visit_corrupt(const bson_iter_t *iter ARG_UNUSED, void *data ARG_UNUSED) /* {{{ */ | |||
| { | |||
| mongoc_log(MONGOC_LOG_LEVEL_TRACE, MONGOC_LOG_DOMAIN, "Corrupt BSON data detected!"); | |||
| mongoc_log(MONGOC_LOG_LEVEL_WARNING, MONGOC_LOG_DOMAIN, "Corrupt BSON data detected!"); | |||
There was a problem hiding this comment.
I was going to ask why we don't throw an exception here, but I realized that libbson already reports an error. This callback just exists as a courtesy in case the integrating library wants to do something extra.
src/bson.c
Outdated
| if (((php_phongo_bson_state *)data)->is_visiting_array) { | ||
| add_next_index_null(retval); | ||
| } else { | ||
| add_assoc_null(retval, key); |
There was a problem hiding this comment.
In the interest of restoring 1.2's behavior, do you think it'd be better to leave the fields unset? That's less chance that the user mixes one of these up with a real BSON null type.
When we actually introduce the classes for these deprecated types, then we can start adding them to the PHP HashTable.
derickr
force-pushed
the
PHPC-1026-bson-classes
branch
from
c3f2513 to
20bbaf3
Compare
No description provided.