Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:112442
User-Agent: Cyrus-JMAP/3.3.0-622-g4a97c0b-fm-20201115.001-g4a97c0b3
Mime-Version: 1.0
Message-ID: <cb3d061a-2424-424d-b1e6-1fe03fadc672@www.fastmail.com>
In-Reply-To: 
 <CAG9XoMS=7BWpVpbrq2hgtr4owaU5j0pmE9yNhVWX7gpVui2HzA@mail.gmail.com>
References: <d1c4689e-9120-4e90-837c-a1b3eac63cd6@www.fastmail.com>
 <79044037-4cd4-4065-a3c1-39a4deea3e09@www.fastmail.com>
 <CAG9XoMS=7BWpVpbrq2hgtr4owaU5j0pmE9yNhVWX7gpVui2HzA@mail.gmail.com>
Date: Sun, 06 Dec 2020 09:32:43 -0600
To: "php internals" <internals@lists.php.net>
Content-Type: text/plain
Subject: Re: [PHP-DEV] [RFC] Enumerations
From: larry@garfieldtech.com ("Larry Garfield")

On Sat, Dec 5, 2020, at 5:23 PM, Benjamin Morel wrote:
> Thanks a lot for this RFC, Larry and Iliya! I can't imagine the amount of
> thought and work put into this.
> Enums are definitely a most-wanted PHP feature.
> 
> I played a bit with the early implementation, and love it so far. Here are
> my thoughts on the RFC and the current implementation:

Yay!

> *Serialization*
> 
> I guess what it comes down to is whether / how easily a class can return
> > an existing instance when asked to unserialize, rather than setting
> > properties on an existing instance. That is, given the string
> > "C:4:Suit:6:{Spades}" can the class definition return the appropriate
> > singleton for Suits::Spades rather than a newly constructed object?
> >
> > If this proves tricky to implement, it would probably be better to
> > forbid serialization than using the default object format and breaking
> > the singleton-ness of the case objects.
> 
> 
> +1, totally agree with this statement. That's the first thing I tried when
> playing with the implementation, and noticed that serialization is not
> supported:
> 
>     echo unserialize(serialize(Status::Active));
>     Notice: unserialize(): Error at offset 0 of 42 bytes
> 
> I would definitely expect strict equality to be maintained on enum cases
> even after unserialization!

I've opened a task to just block serialization entirely for now. It's probably best to not support it at all than to have half-arsed buggy support, at least for now: https://github.com/Crell/enum-comparison/issues/46

> *var_export()*
> 
> Currently, var_export() returns a __set_state() syntax:
> 
>     var_export(Status::Active);
>     Foo\Bar\Status::Active::__set_state(array(
>     ))
> 
> I guess this should just return Foo\Bar\Status::Active.

Good point.  I've opened a task for that: https://github.com/Crell/enum-comparison/issues/47

> *Scalar Enums, ::cases()*
> 
> The implementation does not support these yet, so I haven't had a chance to
> play with them.
> I share Pierre R.'s concerns, though:
> 
> Does this mean that an enum can't have two cases with the same primitive
> > value ? I would very much being able to do so, for example, when you
> > change a name and want to keep the legacy for backward compatibility.
> 
> 
> But I'd understand if you just disallow duplicate scalar values altogether,
> this is probably the most sensible solution here.
> Another idea that comes to mind is that cases() could return an iterator
> instead of an array, having the cases as keys and the scalars as values,
> but this would probably come as a surprise and be bad for DX.

Off hand, I'm not sure if any languages support duplicate enums.  I can see the use case for renaming, but I suspect it's going to just be too complicated to support in practice.  I'll make a note in the RFC that they must be unique.

> *Enum & UnitEnum interfaces*
> 
> The implementation does not seem to support these yet. Taking the examples
> from the RFC:
> 
> Suit::Hearts instanceof Enum; // true => Parse error: syntax error,
> unexpected token "enum"
> Suit::Hearts instanceof UnitEnum; // true => FALSE

Yeah, we only added that to the RFC text an hour or three before you posted; Ilija has to catch up with the new text yet. :-)  Please give him some time.

--Larry Garfield