PHPC-2083: Allow enums to be instantiated during BSON decoding

[jmikola]

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

[jmikola]

When executed with TEST_PHP_ARGS=-m, both bson-enum-002.phpt and bson-enum-003.phpt fail the following assertion:

php: /tmp/build_php-8.1.8.FWj/php-8.1.8/Zend/zend_objects_API.c:195: zend_objects_store_del: Assertion `(!(((zend_uintptr_t)((executor_globals.objects_store).object_buckets[handle])) & (1<<0)))' failed.
Aborted

Termsig=6

The corresponding .mem file reports many leaks (none include phongo_bson.c in the stack trace), but I believe those are just because of the abort failure immediately terminating the process without allowing PHP to clean up internally allocated memory.

The assertion failure corresponds to this line in LXR.

[jmikola]

I spent some time investigating this and realized it was caused by using ZVAL_COPY_VALUE instead of ZVAL_COPY on the resolved enum case. These are discussed in more detail in Copying Zvals in the internals book.

ZVAL_COPY_VALUE was previously used on the return value from bsonSerialize(), which made sense there since we didn't need to increment the ref count (the value was being moved). But for enums, this left the original enum case in a bad state, which triggered the assertion failure in zend_objects_store_del(). ZVAL_COPY correctly increments the case's refcount, which allows all tests to pass w/o leaks.

[alcaeus]

Logic looks good, I do have a couple of questions around making this easier to use. Requiring Unserializable isn't really necessary IMO, and a trait to ease implementing enums that implement Persistable would reduce the number of foot-guns we provide to users.

[alcaeus]

We might want to provide a PersistableEnum trait to reduce copy-pasta and make this less error-prone for users:

enum MyBackedEnum: string implements Persistable
{
    use PersistableEnum;

    case foo = 'bar';
}

[jmikola]

I see a few options, which may not be mutually exclusive:

• Allow an enum to be specified in a typeMap without requiring it to implement Unserializable (since we know the method is irrelevant). In this case, I suppose we'd still invoke bsonUnserialize() if it is available on the enum, just to preserve expected behavior for BSON decoding. Since enforcement of Unserializable happens elsewhere, this will require some additional code changes.
• Create an UnserializableEnum trait
• Create a PersistableEnum trait

Presumably, the traits would exist in PHPC.

[jmikola]

Added a PersistableEnum trait, which includes a bsonSerialize() method that returns the enum object cast as an array and a bsonUnserialize() method that does nothing. Presumably, this can be used just fine with an enum implementing any combination of Serializable, Unserializable, and/or Persistable.

If users implement Persistable, the encoded BSON is going to include __pclass; otherwise, they'll need to use a typemap and implement Unserializable (as demonstrated in the tests). We'd want to document both approaches, and I suppose the PersistableEnum docs would be a good place to do that.

I don't think there's any reason to create UnserializableEnum, as it'd just be a subset of PersistableEnum with no bsonSerialize() method. And if users actually want a different implementation for bsonSerialize() (we're well in edge case territory here), they can avoid the trait and just roll their own implementations.

Lastly, I looked into whether we could restrict use of this trait to enum objects (e.g. throw an exception if a non-enum imported the trait) but found no easy way to do so after digging through zend_compile_use_trait() in PHP core. I expect this would be possible with some compiler hacking, but I have no idea how to do that and it'd probably be a waste of @sgolemon's time for relatively little benefit.

[alcaeus]

I agree with only providing PersistableEnum - also agree with not restricting the use to traits. No matter how much effort you'll put in it, people will always find a way to shoot themselves in the foot.

[sgolemon]

I'm wondering what you expect to happen in the bsonUnserialize call for a typical unserializable enum case. Since these are essentially singleton instances any side effects will be essentially global.

[jmikola]

I mentioned this in one of the test file comments, but there's really no practical purpose for bsonUnserialize. Since enums have no state, there's nothing that would be initialized. We're just invoking it because of the existing contract. The example implementation I used in the test file was to log a message that bsonUnserialize was called.

I think it sucks that we need to require enums to implement an interface, but I'd be more concerned about implicitly encoding their class name (i.e. pretending they're Persistable w/o the explicit implementation). The trait that @alcaeus suggested is probably the best compromise.

Beyond that, I don't think an ODM like Doctrine would even care about this. Enums would never be used as root documents and if they appeared as a field within a modeled class, that class itself could just re-instantiate the enum (e.g. calling BackedEnum::tryFrom() with the value property, or reading name and fetching the case constant directly).

[jmikola]

FYI: a PersistableEnum trait with a no-op bsonUnserialize() method has been created.

[alcaeus]

LGTM!

[alcaeus]

Seeing these changes makes me appreciate the work people put into gen_stub.php for PHP core - it's just so much easier to define classes now than it was before when everything had to be done manually...

[jmikola]