-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Conversation
aa2be7f
to
94ffcf3
Compare
@@ -20,6 +20,7 @@ | |||
#include "zend_API.h" | |||
#include "zend_compile.h" | |||
#include "zend_enum_arginfo.h" | |||
#include "zend_interfaces_arginfo.h" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
94ffcf3
to
9801deb
Compare
/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? |
da6202e
to
8532d26
Compare
Great, it's a start. Of course I would rather allow to redeclare |
I'm against allowing redeclaring |
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 For example, should 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. |
@Crell Generally I agree.
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. |
The value obviously, let's no pretend the name is something that should be used for anything else than an identifier in the code.
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.
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.
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. |
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. |
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 |
@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. |
Sure, let me chat about this with a few other contributors to sum up our experience and get back here. |
@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 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. |
An alternative idea might be to allow implementing |
And here we are in Symfony's problemspace, I suppose.
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.
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 And that brings us to attributes. People cannot perform a string cast there because all ways to do that are forbidden by the engine:
The
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.
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. |
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.
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
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.
@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
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:
Since there is only one sensible implementation for the method, i think it's fine for the engine to provide one. There is work happening, by @iluuu1994 also, to allow writing Note that for the non-strict camp, this PR covers more use cases than the I hope this helps clarify why this PR is highly desired. |
👍
You know, great that |
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. |
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). |
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". 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). |
For the record I opened an RFC at https://wiki.php.net/rfc/auto-implement_stringable_for_string_backed_enums |
@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 |
Accidentally found out about this PR from the Twig/Enum issue. It was kind of disappointing to see this proposal for couple of reasons:
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 Just wanted to share my 2 cents. UPD: OK, I really missed that 8.1 enums forbid the use of |
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. |
/cc @nicolas-grekas