Skip to content
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

Union support #5

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Union support #5

wants to merge 1 commit into from

Conversation

MidnightDesign
Copy link
Contributor

@MidnightDesign MidnightDesign commented Jun 19, 2024

Previously, it was not possible to instantiate objects that have properties with a union type:

class Bar { public function __construct(public string $a) { } }
class Baz { public function __construct(public int $b) { } }
class Foo { public function __construct(public Bar|Baz $value) { } }

Json::decode('{ value: { b: 42 } }', Foo::class); // Error: Union types are not supported

This pull request adds a $converter to the Field attribute, which enables unions:

class Bar { public function __construct(public string $a) { } }
class Baz { public function __construct(public int $b) { } }
class BarBazUnion implements Converter {
    public static function fromJson(mixed $value): Bar|Baz
    {
        assert(is_array($value));
        return Json::instantiateClass(array_key_exists('a', $value) ? Bar::class : Baz::class, $value);
    }
}
class Foo {
    public function __construct(
        #[Field(converter: BarBazUnion::class)] public Bar|Baz $value,
    ) { }
}

$foo = Json::decode('{ value: { b: 42 } }', Foo::class);
$baz = $foo->value;
$b = $baz->b;
assert($b === 42)

Unfortunately, I had to increase the surface area of the library's API by making Json::instantiateClass() public. I'm still thinking about alternatives. Like instead of returning the final value, the converter (which would be a misnomer at this point) could just return the type as a string.

Alternatives considered

  • Separate attribute instead of adding the converter to Field: #[Converter(BarBazUnion::class)] public Bar|Baz $value
    It felt more readable adding it to the existing attribute, and I don't see any downsides.
  • Trying to instantiate each member until one succeeds
    This could be very wasteful, especially for large unions. Also, what if multiple members can be instantiated? Now the order of the members matters - which shouldn't matter.
  • Returning a type string from the converter and let the Json instantiate the object. This would avoid making Json::instantiateClass() public. Works for strings, integers, floats, null, bools and classes, but not (as easily) for arrays (lists and maps). We would have to either use strings like list<Foo> and array<string, Foo>, but they would have to be parsed. Or we could use something like List::of(Foo::class) and Map::of('string', Foo::class).
    Another option would be a hybrid: #[Field(typeProvider: BarBazUnion::class, converter: MyTypeConverter::class)]. But these kind of conflict with each other.

Future scope

  • Right now, any converter has to be implemented from scratch. Simple converters like a discriminator-based one could be built in.

@MidnightDesign MidnightDesign changed the title Support object unions Union support Jun 19, 2024
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.

1 participant