Conversation
It was only used in `bulkwrite_get_debug_info`, where we could really just instantiate the `WriteConcern` instance and add that to the debug info, which would result in the `WriteConcern` object being var_dumped and it more in line with what you'd expect as a PHP user anyway.
paulinevos
requested review from
GromNaN and
Copilot
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
This PR removes the internal helper phongo_write_concern_to_zval() and updates MongoDB\Driver\BulkWrite debug output to expose the write concern as a MongoDB\Driver\WriteConcern object (instead of an array), aligning var_dump() output with user expectations.
Changes:
- Removed
phongo_write_concern_to_zval()from the WriteConcern header and implementation. - Updated
BulkWritedebug info generation to instantiate aWriteConcernobject viaphongo_writeconcern_init(). - Updated the PHPT expectation to match the new
var_dump()structure.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/bulk/write-0002.phpt | Updates expected var_dump() output for BulkWrite to show a WriteConcern object. |
| src/MongoDB/WriteConcern.h | Removes the declaration of the now-unused helper function. |
| src/MongoDB/WriteConcern.c | Removes the implementation of phongo_write_concern_to_zval(). |
| src/MongoDB/BulkWrite.c | Switches debug info write concern from array serialization to a WriteConcern object instance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| NULL | ||
| ["write_concern"]=> | ||
| array(%d) { | ||
| object(MongoDB\Driver\WriteConcern)#%d (%d) { |
There was a problem hiding this comment.
To make our intent explicit, we can add an assertion stating that this property is an instance of WriteConcern.
There was a problem hiding this comment.
I don't understand this one. Is this not already clear from the debug output?
There was a problem hiding this comment.
Yes, it's already clear. It’s just that I find it easier to modify an output snapshot by telling myself it’s a minor change. Whereas if it’s a specific assertion, we know that the result was intended.
| NULL | ||
| ["write_concern"]=> | ||
| array(%d) { | ||
| object(MongoDB\Driver\WriteConcern)#%d (%d) { |
There was a problem hiding this comment.
The WriteConcern object always has a j property (set to NULL when journal is not configured by phongo_writeconcern_update_properties), unlike the old array produced by phongo_write_concern_to_zval which only included j when mongoc_write_concern_journal_is_set() was true. The expected output is missing:
["j"]=>
NULL
between ["w"] and ["wtimeout"]. This is why the test fails in CI.
There was a problem hiding this comment.
How is it possible that it passes locally? e: and it fails locally when I do add the j prop 🤔
There was a problem hiding this comment.
Did you rebase after I merged the big properties PR?
There was a problem hiding this comment.
Ahh nope, that'll do it lol
It was only used in
bulkwrite_get_debug_info, where we could really just instantiate theWriteConcerninstance and add that to the debug info, which would result in theWriteConcernobject being var_dumped and it more in line with what you'd expect as a PHP user anyway.