Stabilize enum interface error handling and tests#1954
Conversation
GromNaN
requested review from
alcaeus and
Copilot
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
Stabilizes the fatal error message emitted when PHP enums attempt to implement MongoDB\BSON\Persistable, making it consistent across PHP versions despite changes in interface_gets_implemented handler invocation order.
Changes:
- Adjust
Unserializable’sinterface_gets_implementedhandler to prefer reportingPersistablein the fatal error when applicable. - Remove
Persistable’s custominterface_gets_implementedhandler (enum rejection is centralized inUnserializable). - Update PHPT expectations to assert a stable
Persistable-specific fatal error message.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/bson/bson-enum_error-003.phpt | Updates expected fatal error output to consistently reference MongoDB\BSON\Persistable. |
| tests/bson/bson-enum_error-004.phpt | Same as above, for backed enums. |
| src/BSON/Unserializable.c | Special-cases the enum-implementation fatal to prefer Persistable when the enum implements it. |
| src/BSON/Persistable.c | Removes redundant enum-implementation handler; relies on Unserializable handler. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| /* Persistable extends Unserializable, so this handler may be called for | ||
| * Persistable enums; prefer Persistable in the fatal message when applicable. */ | ||
| if (interface == php_phongo_unserializable_ce && php_phongo_persistable_ce && instanceof_function(class_type, php_phongo_persistable_ce)) { | ||
| error_interface = php_phongo_persistable_ce; | ||
| } |
There was a problem hiding this comment.
Still not convinced to keep this part. That's the Unserializable behavior that is not supported.
There was a problem hiding this comment.
If Unserializable::bsonUnserialize() was static, then it could return an enum case and this would not be an issue.
alcaeus
left a comment
alcaeus
left a comment
There was a problem hiding this comment.
LGTM. We can revisit the discussion around Unserializable, although I think that codecs are the better variant to deal with BSON data than the Persistable solution.
Follow-up #1953 (comment)