Allow using enums directly in queries

[IonBazan]

Q	A
Type	bug/improvement
BC Break	no

Summary

This change allows using BackedEnums directly in the query builder instead of using ->value:

// Before:
$this->createQueryBuilder()->field('status')->equals(Status::APPROVED->value)->getQuery();

// Now
$this->createQueryBuilder()->field('status')->equals(Status::APPROVED)->getQuery();

This supports arrays too. Please do note that if enumType is not set, the value is passed directly to the Type without any changes to allow developers to customize the behaviour in their custom types if needed (see testQueryWithMappedNonEnumFieldIsPassedToTypeDirectly() where value is passed directly to StringType and producing an error).

I've added the tests in EnumTest instead of DocumentPersisterTest to avoid conditional mapping loading there (this feature requires PHP 8.1+).

Not sure if this should be treated as bugfix or improvement so feel free to assign to correct milestone.

[malarzm]

Please do note that if enumType is not set, the value is passed directly to the Type without any changes

Thanks to this I'm inclined to release this PR as a bugfix :instead of hacing people wait until next minor 🎉 If anybody asks let's say we thought it'll work out of the box 🙈

[malarzm]

Given $mapping['enumType'] can be either null or a backed enum's FQCN (guaranteed by metadata validation) we should be OK to do following check (maybe backed by an \assert call to satisfy SA) which should be cheaper

Suggested change

	if ($value instanceof BackedEnum && $mapping['enumType']) {
	if (isset($mapping['enumType'])) {

[malarzm]

Also I'm not sure about the call being here instead of after next if statement. Actually I started wondering if we shouldn't ensure that enumType can be combined with type=int|string (or not set which allows autoconfiguration to kick in). Right now

/** @ODM\Field(type="collection", enumType=Suit::class) */
private array $enums = [];

looks OK from user's perspective but I believe it'll blow up?

[IonBazan]

This $value instanceof BackedEnum is necessary because the value might be the enum's backed value too:

$this->dm->createQueryBuilder(Card::class)
            ->field('suit')->equals(Suit::Spades->value)
            ->field('nullableSuit')->equals('C');

I'll add a test for that.

Regarding mixing enumType with arrays, it will probably blow up if you map it this way. Not sure how could we solve it here though.

[malarzm]

Not sure how could we solve it here though.

Not necessarily here, I'll hack a PR later today to throw a mapping exception if enumType is combined with something else than int or string types. Then we'll have a clear "it's not working message" for userland instead of weird blow ups under the hood.

[jmikola]

Is this PR in any way related to the driver? We recently had a bug report come in about using enums in conjunction with the driver's Persistable interface. While that was mainly an issue with unserializing BSON into an enum, I wonder if there's also another problem with how enums are serialized to BSON (ignoring anything to do with Persistable).

I'm tracking this enum/Persistable issue in PHPC-2083 and have started working on a PR.

I haven't actually tested standard BSON serialization for enums, but based on this issue it seems like they get encoded as BSON documents with name field. And for BackedEnum instances, there's would be an additional value field. According to the docs for BackedEnum, that type is intended for cases where an enum does need to get stored in a database (since a pure enum wouldn't have a scalar value).

Would it be preferable for the driver to raise an exception when the user attempts to serialize a pure enum (without Persistable) and encode a BackedEnum as its value (i.e. BSON integer or string)?

[malarzm]

Is this PR in any way related to the driver?

No, we ended with our own (ok ok, ORM's 🙄) way of handling enums (#2412): user must either specify an enumClass property in the mapping or use an enum as property's type and have ODM figure out correct mapping (#2411). Under the hood we use a special reflection with which we get enum's backing value and then treat it as a normal string/int. That's also what is stored in the database

Would it be preferable for the driver to raise an exception when the user attempts to serialize a pure enum (without Persistable)

With ODM we've decided to deal only with BackedEnums, using pure one ends with a mapping exception

and encode a BackedEnum as its value (i.e. BSON integer or string)?

Encoding any backed enum as its backing value would save us some headaches that we are yet to discover, maybe they'd render this PR not needed as well. But is the behaviour safe from driver's perspective? I mean we're cool saving just the value because we have a mapping to know what enum instatiate later. I guess same could be done with an explicit typeMap

[malarzm]

@IonBazan please squash commits ;)

[malarzm]

Thanks @IonBazan!

[jmikola]

Encoding any backed enum as its backing value would save us some headaches that we are yet to discover, maybe they'd render this PR not needed as well. But is the behaviour safe from driver's perspective?

@IonBazan: I haven't worked enough with enums to say with any certainty, but the documentation does say that backed enums exist to allow for database round-tripping. I assume that is alluding to ORM logic that would re-construct an enum from mapping information given a value (using the from() or tryFrom() methods).

IMO, allowing a pure or backed enum to be serialized to BSON is preferable from a driver perspective (assuming users opt in via the Persistable interface), since it's less for the application or an ORM to worry about; however, that does invite possible issues converting BSON to PHP if the document data references a class that no longer exists in the application. Enums just introduce some additional ways that code changes could break if a name or value is no longer defined.