Everything

php

Home » Project » Newsgroups » php.internals » Inconsistency in enumerations RFC »

Inconsistency in enumerations RFC

php.internals

Marc

5 years ago

Hi I'm trying out the new enumeration support for PHP 8.1 using
https://github.com/php/php-src/pull/6489 thought to implement a polyfill
based on class constants and reflection. Doing so I come across the
newly defined interfaces:
``` interface  UnitEnum{
public  string$name;
public  staticfunction  cases():  array  <http://www.php.net/array>;
}

interface  BackedEnumextends  UnitEnum{
public  string$value;
public  staticfunction  from(int|string$scalar):  static;
public  staticfunction  tryFrom(int|string$scalar):  ?static;
}
``` My interest here is on BackedEnum which (currently) supports
int|string. First of all the interface defines a "public string $value"
but in fact the type needs to be "int|string". As properties are not
supported in interfaces this is a pure documentation issue in the RFC
itself. Secondly the arguments name $scalar is not really a good name -
now as we have named arguments - $value would be much more meaningful.
But most interestingly I get the feeling that this should be split into
several typed interfaces because this makes wrong impression especially
that "tryFrom" would except int|string but actually it doesn't.
Additionally, if we want to support other types later on this would blow
up the types even more. Example: ``` enum Test:string { case TEST = '1';
} Test::tryFrom('unknown'); // NULL Test::tryFrom(1);// TypeError ```
The TypeError doesn't make sense here as it matches the type and it even
matches the string representation of an existing type. I think it would
be much clearer if there would be two different interfaces like:
``` interface IntEnum extends UnitEnum { public int $value; public
static function from(int$value): static; public static function
tryFrom(int$value): ?static; }
interface StringEnum extends UnitEnum { public string $value; public
static function from(string $value): static; public static function
tryFrom(string $value): ?static; } ``` or else the above example should
return a NULL or even case matching the string representation instead of
TypeError as it matches the defined argument types. Thoughts? Thanks, Marc

Ilija Tovilo

5 years ago

Hi Marc

Thanks for testing the enum feature.

> Hi I'm trying out the new enumeration support for PHP 8.1 using
> https://github.com/php/php-src/pull/6489 thought to implement a polyfill
> based on class constants and reflection. Doing so I come across the
> newly defined interfaces:
> ``` interface  UnitEnum{
>    public  string$name;
>    public  staticfunction  cases():  array  <http://www.php.net/array>;
> }
>
> interface  BackedEnumextends  UnitEnum{
>    public  string$value;
>    public  staticfunction  from(int|string$scalar):  static;
>    public  staticfunction  tryFrom(int|string$scalar):  ?static;
> }
> ``` My interest here is on BackedEnum which (currently) supports
> int|string. First of all the interface defines a "public string $value"
> but in fact the type needs to be "int|string". As properties are not
> supported in interfaces this is a pure documentation issue in the RFC
> itself.

We intentionally put it in the interface for clarity, even though it's
not technically possible yet.

> Secondly the arguments name $scalar is not really a good name -
> now as we have named arguments - $value would be much more meaningful.

I think you might be looking at an older implementation. I only fixed
the naming recently (~2 weeks ago). The parameter is called $value, as
you suggested.

> But most interestingly I get the feeling that this should be split into
> several typed interfaces because this makes wrong impression especially
> that "tryFrom" would except int|string but actually it doesn't.
> Additionally, if we want to support other types later on this would blow
> up the types even more. Example: ``` enum Test:string { case TEST = '1';
> } Test::tryFrom('unknown'); // NULL Test::tryFrom(1);// TypeError ```
> The TypeError doesn't make sense here as it matches the type and it even
> matches the string representation of an existing type.

try/tryFrom will coerce the given value under non-strict_types. With
strict_types enabled it will type error. I think this behavior is
consistent with the rest of PHP.

> I think it would
> be much clearer if there would be two different interfaces like:
> ``` interface IntEnum extends UnitEnum { public int $value; public
> static function from(int$value): static; public static function
> tryFrom(int$value): ?static; }
> interface StringEnum extends UnitEnum { public string $value; public
> static function from(string $value): static; public static function
> tryFrom(string $value): ?static; } ``` or else the above example should
> return a NULL or even case matching the string representation instead of
> TypeError as it matches the defined argument types.

I guess the interface would be more accurate. I'm not sure if that's a
huge win or just unnecessarily bloats the API. I think using
BackedEnum as a type hint should be relatively rare in practice.
Optimally we could add an associated type or generic type to
BackedEnum but that's currently not possible.

Ilija

Marc

5 years ago

Hi Ilija - thanks for your answers

On 14.03.21 23:18, Ilija Tovilo wrote:

> Hi Marc
>
> Thanks for testing the enum feature.
>
>> Hi I'm trying out the new enumeration support for PHP 8.1 using
>> https://github.com/php/php-src/pull/6489 thought to implement a polyfill
>> based on class constants and reflection. Doing so I come across the
>> newly defined interfaces:
>> ``` interface  UnitEnum{
>>     public  string$name;
>>     public  staticfunction  cases():  array  <http://www.php.net/array>;
>> }
>>
>> interface  BackedEnumextends  UnitEnum{
>>     public  string$value;
>>     public  staticfunction  from(int|string$scalar):  static;
>>     public  staticfunction  tryFrom(int|string$scalar):  ?static;
>> }
>> ``` My interest here is on BackedEnum which (currently) supports
>> int|string. First of all the interface defines a "public string $value"
>> but in fact the type needs to be "int|string". As properties are not
>> supported in interfaces this is a pure documentation issue in the RFC
>> itself.
> We intentionally put it in the interface for clarity, even though it's
> not technically possible yet.
>
>> Secondly the arguments name $scalar is not really a good name -
>> now as we have named arguments - $value would be much more meaningful.
> I think you might be looking at an older implementation. I only fixed
> the naming recently (~2 weeks ago). The parameter is called $value, as
> you suggested.

nice +1

>
>> But most interestingly I get the feeling that this should be split into
>> several typed interfaces because this makes wrong impression especially
>> that "tryFrom" would except int|string but actually it doesn't.
>> Additionally, if we want to support other types later on this would blow
>> up the types even more. Example: ``` enum Test:string { case TEST = '1';
>> } Test::tryFrom('unknown'); // NULL Test::tryFrom(1);// TypeError ```
>> The TypeError doesn't make sense here as it matches the type and it even
>> matches the string representation of an existing type.
> try/tryFrom will coerce the given value under non-strict_types. With
> strict_types enabled it will type error. I think this behavior is
> consistent with the rest of PHP.
>
>> I think it would
>> be much clearer if there would be two different interfaces like:
>> ``` interface IntEnum extends UnitEnum { public int $value; public
>> static function from(int$value): static; public static function
>> tryFrom(int$value): ?static; }
>> interface StringEnum extends UnitEnum { public string $value; public
>> static function from(string $value): static; public static function
>> tryFrom(string $value): ?static; } ``` or else the above example should
>> return a NULL or even case matching the string representation instead of
>> TypeError as it matches the defined argument types.
> I guess the interface would be more accurate. I'm not sure if that's a
> huge win or just unnecessarily bloats the API. I think using
> BackedEnum as a type hint should be relatively rare in practice.
> Optimally we could add an associated type or generic type to
> BackedEnum but that's currently not possible.

I think it would be at least be helpful for example on mapping an
enumeration from database to PHP as it's possible with doctrine types
and probably also for static analyzers without the need to special case
enumeration objects.

I also don't think one additional interface bloats up the API or waiting
for generics helps as nobody knows if this will ever be a thing in PHP.

Would this actually require a follow-up RFC or could this be done as a
small implementation detail change ?

Ilija Tovilo

5 years ago

Hi Marc

> >> I think it would
> >> be much clearer if there would be two different interfaces like:
> >> ``` interface IntEnum extends UnitEnum { public int $value; public
> >> static function from(int$value): static; public static function
> >> tryFrom(int$value): ?static; }
> >> interface StringEnum extends UnitEnum { public string $value; public
> >> static function from(string $value): static; public static function
> >> tryFrom(string $value): ?static; } ``` or else the above example should
> >> return a NULL or even case matching the string representation instead of
> >> TypeError as it matches the defined argument types.
> > I guess the interface would be more accurate. I'm not sure if that's a
> > huge win or just unnecessarily bloats the API. I think using
> > BackedEnum as a type hint should be relatively rare in practice.
> > Optimally we could add an associated type or generic type to
> > BackedEnum but that's currently not possible.
>
>
> I think it would be at least be helpful for example on mapping an
> enumeration from database to PHP as it's possible with doctrine types
> and probably also for static analyzers without the need to special case
> enumeration objects.

Can you provide a concrete example where the current interface is
inadequate? I can't imagine a situation where you'd want to accept all
integer BackedEnums but string BackedEnums. If we ever expand the
types that backed enums accept the number of interfaces will grow
linearly.

> I also don't think one additional interface bloats up the API or waiting
> for generics helps as nobody knows if this will ever be a thing in PHP.
>
> Would this actually require a follow-up RFC or could this be done as a
> small implementation detail change ?

Historically, things like this have been done before if nobody
objects. But that seems unjustified at this point given the lack of
concrete reasoning.

Ilija

Marc

5 years ago

On 17.03.21 09:36, Ilija Tovilo wrote:

> Hi Marc
>
>>>> I think it would
>>>> be much clearer if there would be two different interfaces like:
>>>> ``` interface IntEnum extends UnitEnum { public int $value; public
>>>> static function from(int$value): static; public static function
>>>> tryFrom(int$value): ?static; }
>>>> interface StringEnum extends UnitEnum { public string $value; public
>>>> static function from(string $value): static; public static function
>>>> tryFrom(string $value): ?static; } ``` or else the above example should
>>>> return a NULL or even case matching the string representation instead of
>>>> TypeError as it matches the defined argument types.
>>> I guess the interface would be more accurate. I'm not sure if that's a
>>> huge win or just unnecessarily bloats the API. I think using
>>> BackedEnum as a type hint should be relatively rare in practice.
>>> Optimally we could add an associated type or generic type to
>>> BackedEnum but that's currently not possible.
>>
>> I think it would be at least be helpful for example on mapping an
>> enumeration from database to PHP as it's possible with doctrine types
>> and probably also for static analyzers without the need to special case
>> enumeration objects.
> Can you provide a concrete example where the current interface is
> inadequate? I can't imagine a situation where you'd want to accept all
> integer BackedEnums but string BackedEnums. If we ever expand the
> types that backed enums accept the number of interfaces will grow
> linearly.

One example that comes to my mind would be enumeration support as
doctrine type.

This is a modified and simplified example of a real world doctrine type
using marc-mabe/php-enum

abstract class AbstractEnumType extends Type {
    /** @return class-string<BackedEnum> */
    abstract public function getEnum(): string;

    public function convertToDatabaseValue($phpValue, AbstractPlatform
$platform): ?string {
        $enum = $this->getEnum();
        if (!$phpValue instanceof $enum) {
            throw new RuntimeException('Unexpected $phpValue');
        }

        return (string)$phpValue->value;
    }

    // This currently requires reflection
    public function convertToPHPValue($dbValue, AbstractPlatform
$platform): BackedEnum {
        $enum = $this->getEnum();

        $refl = new ReflectionEnum($enum);
        if ($refl->getBackingType()->__toString() === 'int') {
            return $enum::from((int)$dbValue);
        }

        return $enum::from($dbValue);
    }

    // This would be cleaner and faster
    public function convertToPHPValue($dbValue, AbstractPlatform
$platform): IntEnum|StringEnum {
        $enum = $this->getEnum();

        if ($enum instanceof IntEnum) {
            return $enum::from((int)$dbValue);
        }

        return $enum::from((string)$dbValue);
    }
}


##########################

Another example would be an input filter

class EnumFilter extends Laminas\Filter\AbstractFilter {
    /** @var class-string<BackedEnum> */
    private string $enum;

    public function filter($value) {
        $enum = $this->enum;
        $refl = new ReflectionEnum($enum);
        if ($refl->getBackingType()->__toString() === 'int') {
            return $enum::tryFrom((int)$value);
        }

        return $enum::tryFrom((string)$value);
    }

    // vs

    public function filter($value) {
        $enum = $this->enum;

        if ($enum instanceof IntEnum) {
            return $enum::tryFrom((int)$value);
        }

        return $enum::tryFrom((string)$value);
    }
}

########################################

I can also think of static analyzers to detect type of from(), tryFrom()
and $enum->value based on the implemented interface instead of requiring
them to special case integer vs string backed enums.

>> I also don't think one additional interface bloats up the API or waiting
>> for generics helps as nobody knows if this will ever be a thing in PHP.
>>
>> Would this actually require a follow-up RFC or could this be done as a
>> small implementation detail change ?
> Historically, things like this have been done before if nobody
> objects. But that seems unjustified at this point given the lack of
> concrete reasoning.

Sure, this needs a concrete and approved reason. I try my best ;)

> Ilija

Thanks,
Marc