Convert PHPEnumType constructor to use config #1621
Conversation
| * | ||
| * @throws \Exception | ||
| * @throws \ReflectionException | ||
| */ | ||
| public function __construct(string $enum, ?string $name = null) | ||
| public function __construct(array $config) |
There was a problem hiding this comment.
| public function __construct(array $config) | |
| public function __construct( | |
| string $enumClass, | |
| ?string $name = null, | |
| ?string $description = null, | |
| ?EnumTypeDefinitionNode $astNode = null, | |
| ?array $extensionASTNodes = null, | |
| ) |
How about we introduce the new configurable options in this non-breaking way? Given modern PHP now supports named arguments, it should be even more convenient than using array-based config. Ultimately, we might do the same for all other classes.
There was a problem hiding this comment.
The problem I'm trying to fix here is the inconsistency in the constructor for this one type. If you want to support named arguments then that's something we should do across the board (although I'm not sure it's a great idea, but that's a discussion for another day), which I hope you agree is out of scope for this change.
If you really want to preserve bc I suppose we could make the first argument something like $enumOrConfig and do some fancy checking at runtime.
There was a problem hiding this comment.
The problem I'm trying to fix here is the inconsistency in the constructor for this one type.
In that case, I might consider merging this when making a future release that includes breaking changes. I don't plan on it soon though. And if the plan is to move towards named arguments then, I will most likely not make an intermediary release for this just to fix the inconsistency.
The variant I proposed now is something I would be comfortable merging right now. Even if we ultimately go back to using config arrays everywhere, the breakage won't really be worse when we extend it now.
There was a problem hiding this comment.
I implemented the proposed solution in #1623 and released https://github.com/webonyx/graphql-php/releases/tag/v15.17.0. We can discuss potential breaking changes when it comes to it, I might change my mind on using config arrays or named arguments.
I'm cool with waiting for the next major release (it's not currently too tough to work around this issue), but that was the agreement we had the last time I opened a PR to fix this, and it didn't get merged into 15. I'm also agreeable to supporting both formats (param list and single config object) in the interim via runtime checking, but I don't want to bother doing the work if you're just going to pooh-pooh it on some kind of philosophical grounds. Let me know what you think, thanks. |
|
I did not make a breaking release since you opened #1375. |
With all the activity around enums lately, I thought I'd trot out this old classic from May of last year (I somehow botched the old PR, so this is a do-over). The basic idea is to make the
PhpEnumTypeconstructor take a config like all the other ones.As previously established, this is indeed a breaking change.
Kindly review, thank you.