Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:118058
Feedback-ID: i8414410d:Fastmail
User-Agent: Cyrus-JMAP/3.7.0-alpha0-713-g1f035dc716-fm-20220617.001-g1f035dc7
Mime-Version: 1.0
Message-ID: <2cee17c0-7b77-4709-94b9-598a4c3d1f49@www.fastmail.com>
In-Reply-To: <9C11261B-B9D0-4342-81EB-60276B3036E5@php.net>
References: 
 <CAOWwgpmG33Ybp6KS4ePrMgMJdW5xX+M2mos2cpgoVNUy8dVMjQ@mail.gmail.com>
 <9C11261B-B9D0-4342-81EB-60276B3036E5@php.net>
Date: Wed, 22 Jun 2022 12:26:56 -0500
To: "php internals" <internals@lists.php.net>
Content-Type: text/plain
Subject: Re: [PHP-DEV] [RFC] [Under Discussion] Auto-implement Stringable for string
 backed enums
From: larry@garfieldtech.com ("Larry Garfield")

On Wed, Jun 22, 2022, at 8:05 AM, Derick Rethans wrote:
> On 21 June 2022 23:47:15 BST, Nicolas Grekas 
> <nicolas.grekas+php@gmail.com> wrote:
>>Hi everyone!
>>
>>I'd like to open a discussion on this RFC, to auto-implement Stringable for
>>string-backed enums:
>>https://wiki.php.net/rfc/auto-implement_stringable_for_string_backed_enums
>>
>>I'm looking forward to your feedback,
>
> I am not in favour of this RFC, especially because it is not opt-in. 
>
> 1. Enums were introduced to add extra type safety. Allowing them to 
> degrade to simple strings reduces that type safety.
> 2. It is not implemented for all enum types, only for string backed 
> ones. This change would now make different classes of enums, without 
> them being differently inherited classes. 
> 3. The main use case seems to be to prevent having to type ->value, 
> which would otherwise indicate reliably whether an enum is used as 
> string argument. 
>
> I won't be voting in favour of this RFC as it currently stands. 
>
> cheers 
> Derick 

I am also still opposed to this proposal, and the more I think on it the more opposed I get.

Here's the basic problem.  There are two use cases (broadly speaking) where this would be applicable.  In one, a parameter is string typed today but there is a finite number of legal values, logically.  Think $order = ASC/DESC.  In the other, the parameter is string typed today and there is a technically infinite number of legal values, from the point of view of that function.  In a given use case there may be a finite set we want to use, but from the function's point of view, it's nominally infinite.  That's the "roles" example that Nicolas cites.

In case 1, I'd argue that the function should be switching to an Enum long term *and dropping the string*.  For that, a union type is the optimal solution.  Does that have BC implications for sub-classes?  Well, yes, but so does any type improvement.  This is a known problem space, with known solutions and migration strategies.  It's conceptually no different from migrating from "this takes a string" to "this takes an array of strings": Widen the type, have transitional code, retighten the type.  The time frame for that could be weeks, months, or years depending on the situation, but it's not a novel concept.

In case two, an enum is simply the wrong tool.  Every time I say this someone whines that I'm being elitist or judgemental or holier than thou or whatever, but... they're different types.  If someone proposed "I want to be able to pass a single-element array to a string parameter and have it automatically unwrap the array", it wouldn't be taken seriously.  That's what's being proposed here, though.

If an access control system works on roles as strings, then... it expects a string.  It doesn't expect "one of these and only these values, defined at code time."  The *application* may be written for that, but the access check function does not.  It works on strings.  Passing it a non-string *should* be an error, just as much as passing it an array should be.

The argument presented is that it's easier to type `AppRoles::Admin` than `"admin"`, because the former provides you with an error if you typo something.  That's a valid argument, but... not for using enums.  It's an argument for using constants.

class AppRoles
{
  public const Admin = 'admin';
  public const Editor = 'editor';
  public const User = 'user';
}

enum AppRoles: string
{
  case Admin = 'admin';
  case Editor = 'editor';
  case User = 'user';
}

It's basically the same work to setup, but one does exactly what it says: It provides syntax-checked shortcuts for strings, which is what the API wants.  Using an enum here instead of constants provides exactly zero additional value, because the limited-set-ness of the enum won't be checked in the first place.

Just because someone wants to use the claw-side of a hammer as a screwdriver doesn't mean we should design it to be a screwdriver.  They should just use a screwdriver.

And for those arguing that we shouldn't be "protecting users from themselves"... that's exactly what types are.  That's exactly what they're for.  The whole reason to have typed parameters is to stop people from doing things that make no sense.  That's the point.

So I am firmly against making it easier to (mis)use enums in a situation where constants are already the superior solution by every metric.  The only argument I see is making case 1, transitioning from a string to an enum for a genuinely limited-case, easier.  But in that case, the transition is going to have to happen eventually anyway, and that means the type is going to change at some point, and the same BC issue will appear, just at a different time.  Unless the intent is to then never change the type and keep the function incorrectly typed (from the POV that it's logically an enum, even though string typed was the best/correct type for years) forever, in which case... use a set of constants.  

--Larry Garfield