On Tue, Jan 5, 2021 at 6:56 PM Larry Garfield <la...@garfieldtech.com>
wrote:
> On Tue, Jan 5, 2021, at 8:26 AM, Nikita Popov wrote:
>
> > Nice work, I like the updated proposal. Some notes:
> >
> > > Similarly, enum names and case names are both case insensitive.
> >
> > I agree that enum names should be case insensitive (like class names),
> but
> > why should case names be case insensitive? The closest analogon to a case
> > would be a class constant, and those are case sensitive.
>
> This is a left over from the earlier draft when Cases were also classes.
> I've corrected it. Thanks for the catch.
>
> > > All Cases have a read-only property, case, that is the case-sensitive
> > name of the case itself.
> >
> > I can see how this makes sense, but it wouldn't be my first guess as to
> how
> > you access the case name. I'm wondering if using $enumValue->name or
> > $enumValue->caseName might be preferable.
>
> We'll change it to ->name.
>
> > > Scalar equivalent values must be literals. Constants and constant
> > expressions are not supported.
> >
> > Why? This seems inconsistent with the overall language. If I can use a
> > constant expression as a class constant value, why can't I use it as an
> > enum value?
>
> Mostly because it was more challenging to do. We're fine with including
> it if Ilija can make it work. (Suggestions for him on how to do so most
> efficiently are welcome.)
>
It may be okay to retain the limitation, but in that case the RFC should
include some technical justification for that. It would also be good to
explicitly state whether something like "1 + 1" is allowed, i.e. whether
constant expressions are forbidden entirely, or only those that cannot be
evaluated at compile-time.
> > > ScalarEnum exposes an additional static method from() that is
> > automatically generated.
> >
> > I think it would be good to be slightly more explicit here and call it
> > fromValue(). (We could have fromName() to construct it from a case name,
> > and any number of custom from* named constructors.)
> >
> > > The from() method will up-cast from a scalar to its corresponding
> > Enumerated Case. Invalid scalars with no matching Case will throw a
> > ValueError. There is also a has() method, which will return boolean true
> if
> > a case with that value exists and false otherwise
> >
> > Just a thought, but rather than having has() and from(), it may make
> sense
> > to have from() (throws if invalid) and tryFrom() (returns null if
> invalid).
> > I think a has() method is pretty much useless in isolation, it will
> always
> > be used in a combination of has() and from(), in which case it is better
> to
> > combine them rather than have an implicit contract between them.
>
> Hm, maybe. My concern is that "try" to me implies "exceptions are
> involved somewhere", since PHP doesn't have the "TryX means nullable
> return" convention that Rust or C# have.
>
> Anyone else want to weigh in?
>
> > > Manually defining a static from() or has() method on a Scalar Enum will
> > result in a fatal error.
> >
> > Or a non-static one :) You can't define a static and a non-static method
> > with the same name in PHP.
>
> Fixed.
>
> > > Enums and cases may have attributes attached to them, like any other
> > language construct. The TARGET_CLASS target filter will include Enums
> > themselves. The TARGET_CONST target filter will include Enum Cases
> >
> > TARGET_CONST should presumably be TARGET_CLASS_CONST. More generally
> > though, I wonder if there should be a separate TARGET_CASE...
>
> We had that in the spec until like 40 hours ago. :-) We removed it mainly
> because beberlei was pushing back on it. (And the corresponding reflection
> classes, which are still under discussion.)
>
> Personally I'd rather have the flexibility of targeting enums and cases
> specifically. Ilija and Benjamin feel it's better to "embrace the
> object-y-ness" of enums and just use the existing targets. It's not a hill
> I'm going to die on either way, so if you can convince them I'm happy to
> add them back in.
>
Can't say I have a particularly strong argument either way here :)
> > > Returns the scalar equivalent type of the Enum, if any. If it doesn't
> > have one, it returns a ReflectionType on null.
> >
> > Possibly I'm misunderstanding the sentence, but why does it return a
> > "ReflectionType on null" rather than just "null"? It's not like ::$value
> > will have a null value in this case, it will not exist at all.
>
> More likely I'm misunderstanding the nuances of the reflection API. The
> ideal here is to avoid a nullable return if at all possible. As noted
> above reflection is the shakiest part left right now, so if you have
> specific suggestions for how it would work better, please share.
>
Well, why do you want to avoid a nullable return here? We have many other
getType/getReturnType style APIs that all return null if no type is
specified, so I would expect this one to work the same way. Returning
ReflectionType("null") is disingenuous, as the enum was neither declared as
"enum Foo: null {}", nor does it behave as if it were declared as such.
> > > Additionally, a new function is_enum(mixed): bool returns true if the
> > value passed is an enum or case object.
> >
> > Could you please clarify what "an enum or case object" means? Is this
> > function both for checking whether an object is a case object and a
> > (string) class an enum class?
>
> It would return true if given an enum case, a variable that is assigned to
> an enum case, or a class name string that refers to a class that is an
> enum. Other than replicating the test for it I;m not sure how better to
> describe it. (Or, just use that sentence?)
>
Including an example would be good. I just checked the implementation, and
apparently the actual signature is "is_enum(mixed $value, bool $autoload =
true): bool", which brings me to the concern I have about this API: Mixing
checking for classes, which requires autoloading, and for other values is
generally a bad idea -- been there, done that.
The problem is that if used to check whether a value is an enum instance,
then just naively writing is_enum($value) may trigger autoloading when
$value happens to be a string. This is not just inefficient, but also a
possible security issue. The $autoload flag can mitigate that, but that's
just a workaround for bad function design.
This could be addressed in different ways, e.g. one could have separate
is_enum_class() and is_enum_value() functions, though the first one would
probably more typically be called enum_exists(). I'm not sure if that is
the right way to address it though, as the intended use case for the
is_enum() function is not entirely clear to me.
Regards,
Nikita
