Skip to content

PHPC-2083: BSON handling for enum classes #1378

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 3 commits into from
Nov 10, 2022
Merged

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented Oct 28, 2022

https://jira.mongodb.org/browse/PHPC-2083
https://jira.mongodb.org/browse/PHPC-2159

Proposed Behavior

Prohibit encoding of pure enums.

Encode BackedEnum instances as their backing value.

Ignore enums indicated in a “__pclass” field during BSON encoding (see PHPC-2159). While enums were previously never instantiatable from decoding BSON, a previous driver version might have been used to encode a Persistable enum to BSON. That is not a use case we want to support, but it would be inconsistent to raise an exception in this case.

Prohibit enums from implementing Persistable and Unserializable using the “interface_gets_implemented” object handler. We will remove previously introduced code in 1.15-dev that instantiated enums from case strings, and will not add behavior to instantiate BackedEnums from case values. Additionally, the bsonUnserialize() method was never relevant for enums, since they have no state.

Allow pure and backed enums to implement Serializable. Since enums are objects, they are permitted to implement Serializeable and that implementation will take precedence over standard BSON encoding rules (as it does with BSON type classes). Two thoughts on this:

  • The previous behavior (i.e. encoding as a BSON document with a “name” and optional “value” fields) can be achieved by implementing Serializable and returning “(array) $this” from bsonSerialize().
  • I certainly don’t want to encourage Serializable implementations for enums, but I also don't see a reason we need to prohibit it. It could technically provide a back door for users to round-trip non-backed enums. For example, the bsonUnserialize() method of an Unserializable class containing an enum could convert the pure/backed enum’s bsonSerialize() document representation back into an enum.

Note that I removed the following:

Allow a BackedEnum class to be specified in a type map field path without requiring it to implement Unserializable. Modify the int32, int64, and string visitor functions to consult the type map and conditionally instantiate a BackedEnum using the same logic as from() (i.e. error if the value is not an enum case). We can utilize the zend_enum_get_case_by_value() API in PHP 8.2+ for this (and backport it for PHP 8.1).

@jmikola jmikola marked this pull request as ready for review October 28, 2022 09:16
@jmikola jmikola force-pushed the phpc-2083 branch 4 times, most recently from 818e0e5 to 41578d2 Compare October 30, 2022 14:28
@jmikola jmikola requested a review from alcaeus November 8, 2022 07:52
Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

LGTM. We might circle back to add decoding to an enum case, but for now this is significantly more robust than the previous iteration 👍

…checks

Prohibit uninstantiatable classes in type maps, and ignore them when processing __pclass fields.

Utilize zend_get_object_type_case from PHP 8.2 for more helpful error messages in php_phongo_bson_state_fetch_class.
Revert previous enum instantiation behavior and PersistableEnum trait from de5f1e5

Backed enums will be encoded as their case value. Non-backed cannot be encoded and no enums can be encoded at the root level.

Prohibit enums from implementing Persistable and Unserializable.
@jmikola jmikola merged commit 594cad9 into mongodb:master Nov 10, 2022
@jmikola jmikola deleted the phpc-2083 branch November 10, 2022 12:23
@jmikola jmikola mentioned this pull request Nov 10, 2022
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