Skip to content

Auto-implement Stringable for string backed enums #8825

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

Closed
wants to merge 3 commits into from

Conversation

iluuu1994
Copy link
Member

@@ -20,6 +20,7 @@
#include "zend_API.h"
#include "zend_compile.h"
#include "zend_enum_arginfo.h"
#include "zend_interfaces_arginfo.h"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kocsismate I'm importing this to have access to arginfo_class_Stringable___toString but this generates a bunch of "defined but not used" warnings. How would you fix those? Split the stubs?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, arginfo.h files should only be imported once, so splitting the required parts would make sense.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kocsismate Note: The stringable stubs would still be imported twice, but there wouldn't be any other symbols included twice and not used in this case. Is that ok? The problem is, I can't declare the function in an enum interface since __toString is only generated for string enums. And I want to avoid creating another interface.

@iluuu1994
Copy link
Member Author

/cc @Crell Also FYI. I'm mostly apathetic on this change. The community has repeatedly asked for it and I don't see a big technical reason not to add it (other than having 2 ways to do the same thing). Are you ok with this too?

@iluuu1994 iluuu1994 changed the title Auto-implement Stringable for string backend enums Auto-implement Stringable for string backed enums Jun 19, 2022
@Furgas
Copy link
Contributor

Furgas commented Jun 20, 2022

Great, it's a start. Of course I would rather allow to redeclare __toString in string backed enum with own implementation and ultimately allow it for all enums, backed or not.

@iluuu1994
Copy link
Member Author

I'm against allowing redeclaring __toString(). If you're reusing __toString() for a different form of string representation you should use a proper method to give that representation a name. Allowing to repurpose __toString() will just result in a mess. Similarly, it doesn't make sense to allow __toString() in non-string enums but then disallow them in string enums. I'd prefer to keep the logic here simple and consistent.

@Crell
Copy link
Contributor

Crell commented Jun 20, 2022

I'm very hesitant here, frankly. I think a lot of the request for it has come from people trying to use Enums as fancy constants and pass values to legacy code that types for string, rather than refactoring their code to use Enums consistently.

For example, should __toString return the name, or the value? Or vary depending on if it's backed? What happens when we add ADTs?

Has there been feedback specifically from ORM and other automation folks, or just the Reddit crowd? In my serialization library, I had to do a little dancing to handle serializing non-backed enums but the lack of __toString was never an issue.

@iluuu1994
Copy link
Member Author

@Crell Generally I agree. (string) seems no easier than ->value, and implicit coercion to string is... sub-optimal. Furthermore enum to string coercion will work but not string to enum coercion.

Has there been feedback specifically from ORM and other automation folks, or just the Reddit crowd?

No Reedit, this feedback comes mostly from the Symfony community, and I think many people expect this to kinda just work. It was also suggested as one approach to solve the "value can't be accessed in constant expressions" problem.

@nicolas-grekas
Copy link
Contributor

For example, should __toString return the name, or the value?

The value obviously, let's no pretend the name is something that should be used for anything else than an identifier in the code.

I think a lot of the request for it has come from people trying to use Enums as fancy constants

Yes, and we cannot just ignore those use cases; that would be pedantic. We cannot control how tools provided by the engine are going to be used.

pass values to legacy code that types for string, rather than refactoring their code to use Enums consistently.

Where does this "legacy" adjective come from? There is a lot of code that is just fine with accepting strings and that won't make sense accepting enums. Yes, ppl want to pass them strings defined by backed enums and that makes sense also.

Has there been feedback specifically from ORM and other automation folks, or just the Reddit crowd?

See this discussion for example: symfony/symfony#46363

I can tell that for Symfony, enums are hell. They require many meaningless changes just to pass the type system. And the reason is mostly that the current casting rules for enums make using them painful.

I definitely support this PR.

@iluuu1994
Copy link
Member Author

iluuu1994 commented Jun 20, 2022

@nicolas-grekas

I can tell that for Symfony, enums are hell. They require many meaningless changes just to pass the type system. And the reason is mostly that the current casting rules for enums make using them painful.

Can you elaborate on that? You should only need to do this conversion when working with external systems (which I don't deny will be quite a few for Symfony), but that's only a handful of explicit conversions per such component. All Symfony components should pass the enum objects directly. Also, as mentioned, this PR would only solve one half of this problem, the enum to string coversion, but not vice versa.

@javiereguiluz
Copy link
Contributor

I'd like this feature to be implemented too.

I haven't used lots of enums in PHP apps yet, but so far my experience is very good ... except for the fact that you need to do Foo::Bar->value instead of just Foo::Bar and that felt a bit cumbersome to me. Thanks.

@iluuu1994
Copy link
Member Author

iluuu1994 commented Jun 20, 2022

@nicolas-grekas Maybe it would be good if you gathered some pain points that you've experienced specifically in Symfony so we can also use good argumentation in the RFC. Would it be possible to create such a short list? Doesn't have to be more than a few link/snippets.

And maybe we should start a discussion on the mailing list to get an initial feel of peoples opinions.

@nicolas-grekas
Copy link
Contributor

Sure, let me chat about this with a few other contributors to sum up our experience and get back here.

@Crell
Copy link
Contributor

Crell commented Jun 21, 2022

@nicolas-grekas "Legacy" in the sense of "existing code", not a pejorative in this case. If a parameter takes a string now, and it works, OK cool, keep doing that. If someone wants shorthands for known strings to help turn typos into syntax errors, constants already do that job adequately. Switching those to an enum is a misuse of enums, IMO.

If the use case truly is a limited enum, then the correct solution would be to widen the type to take string|TheEnum, and then as the first line of the method do:

if (is_string($param)) {
  $param = TheEnum::from($param);
}

And then use the enum thereafter. That's fully BC (the string option can be dropped at some point in the future), type safe, and lets people "just pass an enum". Similar patterns already exist for things like "pass a URL object or a URL string", so it should be quite familiar to PHP devs already.

Enums are not primitives. That's not the feature PHP has. I'm very skeptical about making it easier to use enums as primitives, because they're not, and that leads people to poor designs. The only place it would really make sense would be at the boundary, where values get dumped into other systems; Template engines, ORMs, serialization libraries, etc. Most of those are write once, use many times, so adding the if-statement above in a few key places should insulate most developers from the conversion entirely.

I too would be interested to see specific examples where it's problematic, because at the moment, I'm not convinced it's that much of an issue.

@stof
Copy link
Contributor

stof commented Jun 21, 2022

An alternative idea might be to allow implementing __toString in an enum without making it auto-implemented, allowing devs to implement it when they need their enum to be easier to integrate with other libraries expecting a string, but without forcing it all the time.

@derrabus
Copy link
Contributor

derrabus commented Jun 21, 2022

The only place it would really make sense would be at the boundary, where values get dumped into other systems; Template engines, ORMs, serialization libraries, etc.

And here we are in Symfony's problemspace, I suppose.

Most of those are write once, use many times, so adding the if-statement above in a few key places should insulate most developers from the conversion entirely.

That's kind of our problem. Those key places are not just a few, enums are a permanent edge case.

Before we've had real enums, people used classes with only constants on them as a poor man's implementation. Now that we have native enums, people expect to migrate those classes to enums and everything magically continues to work, but it doesn't. We could simply say that those people just haven't read the RFC or documentation properly and that they're misusing enums, but those people open PRs and issues at symfony/symfony and we have to tell them, well that's not how enums are meant to work in PHP.

I'm not sure if this is an RTFM problem or if people are coming from other languages (like Typescript) where backed enums are a true subtype of a primitive type and thus expect PHP to somewhat behave in a similar way.

I too would be interested to see specific examples where it's problematic, because at the moment, I'm not convinced it's that much of an issue.

For us, the sanes way currently is to tell people that they need to cast their enum cases to string before they pass them to framework methods. And I can understand that they want to do that. I understand that people want to model all the permissions a user could possibly have as an enum. I understand that people want to model all states of a workflow as an enum.

But for us, migrating all affected method parameters on public interfaces and non-final classes from string to string|BackedEnum is a long road full of breaking changes. So again the sanest way for us is to ask people to cast to a string.

And that brings us to attributes. People cannot perform a string cast there because all ways to do that are forbidden by the engine:

#[IsGranted(Permission::AccessSensitiveData->value)]

The -> operator is not allowed in constant expressions. But as I understand, this is being worked on.

#[IsGranted((string) Permission::AccessSensitiveData)]

This does not work because casting enum cases to strings is not allowed (yet, see this PR) and again, explicit type casts are not allowed in constant expressions.

#[IsGranted(Permission::AccessSensitiveData)]

If this PR passes, the enum will implicitly be cast to string but unfortunately only if strict types are disabled.

So, to sum it up, even if people use enum for their application logic in valid ways, they face limitations if they want to use external libraries with their enums. It would be great if we could find ways to make it easier for general-purpose libraries to support enums without cluttering libraries with if/else blocks and without changing existing interfaces in a backwards-incompatible way.

@nicolas-grekas
Copy link
Contributor

nicolas-grekas commented Jun 21, 2022

What @derrabus wrote.

For reference, I've back-linked issues/PRs in Symfony+Twig that relate to this issue so that the sources of our early experience with enums can be easily reviewed.

If the use case truly is a limited enum, then the correct solution would be to widen the type to take string|TheEnum, and then as the first line of the method do [...]

That's what I fear: enums as implemented right now are really an edge case (to borrow from @derrabus) in the sense that they require ALL the existing code in place to deal with them.

I'm looking for providing a smoother way to use enums for the community, because requiring every libs to upgrade potentially any string type-hint to string|*Enum is not reasonable. At least for Symfony we're not gonna do it. But this already creates a lot of friction for us maintainers, since ppl legitimally want to use enums in their code, and seamlessly pass them to their deps, where a string is enough for the generic case managed by eg a Symfony component.

And then use the enum thereafter. That's fully BC

Widening a type is not BC at all. It is on constructors, but it's not on any other (non final) method. This won't ever happen at the scale of the PHP community.

Enums are not primitives. That's not the feature PHP has.

@derrabus spotted TypeScript, where string enums extend the string type. This makes so much sense, this is so intuitive. That's also why I think ppl are expecting this to just work. In PHP, string is not a type be can extended from. The closest we have is Stringable, and it would be very much expected to see it implemented on string enums.

The only place it would really make sense would be at the boundary, where values get dumped into other systems

THAT! Every Symfony component / every library plays at this boundary level. And every app uses libraries. These boundaries are VERY common, that's why the engine needs to help get through them.

From @stof:

An alternative idea might be to allow implementing __toString in an enum without making it auto-implemented

Since there is only one sensible implementation for the method, i think it's fine for the engine to provide one.
I would also prefer allowing the implementation of toString on other kind of enums, but that's for another discussion :)

There is work happening, by @iluuu1994 also, to allow writing FooEnum::TheCase->value in eg attributes. This is an alternative way to solve the same issue in the specific context of attribute declarations. Its advantage is that it is compatible with strict types. One major drawback is that it's verbose. Ppl that didn't opt-in for strict types shouldn't be forced to write that boilerplate.
Don't get me wrong, I think it would be nice for the engine to support this other RFC about ->value. But this very PR is still required for ppl that want to use non-strict mode seamlessly. Let's not make non-strict mode the looser mode of PHP. It's not.

Note that for the non-strict camp, this PR covers more use cases than the ->value RFC, eg when using enums in the code itself, or when imploding arrays of string|BackedEnum, etc.

I hope this helps clarify why this PR is highly desired.

@SpacePossum
Copy link

An alternative idea might be to allow implementing __toString

👍
I know I don't represent the community here as all others do, but wanted to say that not allowing implementing Stringable, yet allowing implementing something like JsonSerializable is very confusing for a bunch of PHP endusers.

legacy code, or just the Reddit crowd, has come from people trying to use Enums as fancy constants

You know, great that In my serialization library it is not issue for you, but maybe try to reason without dismissing or talking down a part of the endusers, some just want to share ideas and have a different POV, they not all need education on "proper" use of enums.

@Crell
Copy link
Contributor

Crell commented Jun 21, 2022

Widening a type is not BC at all. It is on constructors, but it's not on any other (non final) method. This won't ever happen at the scale of the PHP community.

Uh? https://3v4l.org/GnBJr

By "boundary" I don't mean library boundary; I mean PHP boundary. Databases, HTML, etc. Places where objects (which is what Enums are) have no natural representation the way a string or number does.

@wouterj
Copy link

wouterj commented Jun 21, 2022

Uh? https://3v4l.org/GnBJr

If we need to widen the argument in Symfony, it's the other way around (A extending B). That is not BC (and rightfully so).

@wouterj
Copy link

wouterj commented Jun 21, 2022

My 2 cents (not doing any PHP internal work and not having done much enum work on Symfony either): I see a lot of noise in our issues and PRs about users wanting to pass string-backed enums into methods using a string (especially in cases that were using constants before, like security voters). This creates lots of overhead for us, having to explain how enums in PHP are meant to be used and why their use-case doesn't fit this over and over again.

To me, this is a clear sign that the current use case is not intuitive for many PHP developers. We've even had issue owners saying "From my understanding, enums were invented for cases like this; while having a class that contains just consts feels like misusing the class idea".
At the same time, other languages do seem to allow backed enums to act as their backed primitive type (e.g. Typescript). So it seems like the feature proposed in this PR isn't incompatible with the concept of backend enums that PHP has.
Combining these two observations, I think it is worth to consider how PHP can adapt this aspect in the current enum feature and I'm happy that @iluuu1994 and @nicolas-grekas have started this discussion.

In the end, it sounds like this change slightly widens the enums use-case and makes it better fit the expectation of PHP developers, without adding much maintenance efforts on the PHP internals side (correct me if I'm wrong on that). Speaking for my own time, a change like this will at least remove quite a bit of maintenance overhead on Symfony's side (to keep our issue list somewhat clean).

@nicolas-grekas
Copy link
Contributor

For the record I opened an RFC at https://wiki.php.net/rfc/auto-implement_stringable_for_string_backed_enums

@stof
Copy link
Contributor

stof commented Jun 22, 2022

@Crell but libraries boundaries are also a concern here. For instance, the list of permissions on which you vote in your security system might be a closed list in your own app, allowing to model it as an enum. But from the library point of view (for instance symfony/security-core), this list needs to be extensible, and so string is a working representation while PermissionEnum is not. And we have lots of other cases like that (almost all Symfony issues that are back-linked here are variants of that same pattern).

@luxemate
Copy link

luxemate commented Jun 22, 2022

Accidentally found out about this PR from the Twig/Enum issue. It was kind of disappointing to see this proposal for couple of reasons:

  1. In our project we heavily use the Stringable interface for enum labels, be it auto-generated or provided by the enum. Both options use a custom interface LabeledEnum + trait). LabeledEnum interface extends Stringable interface and defines a label() method. The trait implements a default transformation logic from enum name to a human readable label (works for upper camel case and pascal case). It works for PHP 8.1 enums and emulated enums on <8.1.
  2. Usually one needs to display the name of some option represented with enums in the templates, so it's more correct to cast enum to a string representation of that case, which is a name or a label, not a backed internal value. It's handy in cases when templates are not controlled directly, like in SonataAdminBundle, though I agree, that we could create our own list and show types/templates exactly for enums, that use the label() method under the hood.
  3. Look at Java's enum for example - it has a toString() method, which returns the same value as name(), although it allows to override the toString() method unlike the name(), which is final. It's more logical, as it works for any enum, not only backed ones. After all, a backed enum is just a special case of an ordinary enum, when you need to store or pass an internal value of an enum to some other system or library.
  4. Some say PHP isn't the most logical language, not without a reason, and making it that only the string backed enums will auto-implement Stringable, without allowing to override the __toString() for no reason won't add up to make it any more logical, IMO, plus it breaks our use case from the 1st paragraph. Though of course we can, and probably will, migrate to using the label() method directly.

@derrabus spotted TypeScript, where string enums extend the string type. This makes so much sense, this is so intuitive.

TypeScript enum cases indeed are not objects, but primitive types, but in this case they also use no type coercion, i.e. int is int and string is string. If PHP would allow to define not only __toString, but also __toInt etc., maybe it wouldn't be such a bad idea to auto-implement them, but again making PHP less strict after all efforts to make it more strict is pointless to me.
I don't think that looking to TS in this case is such a great idea.

Just wanted to share my 2 cents.

UPD: OK, I really missed that 8.1 enums forbid the use of __toString, and well... I agree that this is confusing indeed.

@iluuu1994
Copy link
Member Author

Feature freeze has arrived and due to the feedback there are no immediate plans to pursue this. Anybody else is free to take over if they choose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.