-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DependencyInjection] Allow using BackedEnum in Target attribute #46363
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
to me, this looks like a use case for using constants rather than a BackedEnum. Using PHP enums makes sense when you typehint them, providing type safety. Here, you allow any enum case just to read its backing value, which voids all benefits of the enum. |
@stof Thanks for your reply. In this case, using the enum only for this might not make sense. But in our system, we use the enum to typehint other attributes etc. For example: #[ObjectType(
schema: GraphQLSchema::PUBLIC,
name: 'Movie',
)]
final class MovieType The attribute's signature would look like this: public function __construct(
public readonly GraphlQLSchema $schema,
public readonly string $name,
) {
} Now we get full type safety wherever we use the GraphQLSchema. Since PHP 8.1 doesn't allow you to do this: public function __construct(
#[Target(GraphQLSchema::PUBLIC->value)]
private Schema $schema
) {} or this: public function __construct(
#[Target((string) GraphQLSchema::PUBLIC)]
private Schema $schema
) {} the only way to use the enum is to either get support in Target attribute, or to just have a separate constant instead. I wish PHP would just automatically cast an BackedEnum (string) to a string when the function requires a |
Hmm, php since version 8.1 allows nested attributes, so you can do something with casting to a string:
|
@korkoshko But that still won't work for the Target attribute as that only accepts |
@ruudk if you don't use a strict types declaration, php will auto cast the object to a string if it implements To fully support strict types, it might be better to add a union type |
This is really touching a deficiency of the language and enums in this case IMHO. They are really invading the codebase in a way that makes everything more complex. Like you said: "more attributes would support using enums but that's another discussion". Fixing one "random" attribute doesn't make sense to me. I'd rather look for a more generic approach to them. eg
This might also be The generic solution I'm looking for. Enums being the new toy in school, we might be trying to use them when they don't fit - or where consts would fit better. BTW: shouldn't /cc @Crell |
… which is not allowed btw:
|
If we take this upstream to PHP core, I think we should try to make something like this possible: #[MyAttribute(MyEnum::FOO->value)] Right now, it is not: https://3v4l.org/NI78X |
This would be perfect. Implementing |
This is currently blocked by It sounds like no no one is against fixing that; someone just needs to do the work and file an RFC. That would be the best solution here, IMO. |
Yes and no :) Take eg this PR. I'm not keen to accept it, and I'm not keen either with letting the community in a state where they cannot use enums before some hypothetical improvement in the future. I'm not sure this process followed by enums is beneficial for the community, for this reason... |
@nicolas-grekas I think, we encountered a similar situation with attributes where the implementation that shipped with PHP 8.0 did not include nesting. We've discussed various workarounds back then and decided to wait for PHP to implement that missing feature properly. If |
Best course of action here, I think, is to help get the core issue resolved for 8.2 and reject this PR, as fixing the core issue will make this irrelevant. |
Closing the while we wait for 8.2 to improve. |
This might be the improvement we're looking for: php/php-src#8825 |
This adds support for using backed enums on the Target attribute.
In our code base, we want to do something like this:
Ideally, more attributes would support using enums but that's another discussion / PR.