Skip to content

Convert PHPEnumType constructor to use config #1621

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Convert PHPEnumType constructor to use config #1621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
afc3ed3
fix tests
shmax Oct 18, 2024
93dfeb0
fix more tests
shmax Oct 18, 2024
0c03387
go back to enumClass member
shmax Oct 18, 2024
90c9751
Apply php-cs-fixer changes
shmax Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 26 additions & 13 deletions src/Type/Definition/PhpEnumType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,22 @@
namespace GraphQL\Type\Definition;

use GraphQL\Error\SerializationError;
use GraphQL\Language\AST\EnumTypeDefinitionNode;
use GraphQL\Language\AST\EnumTypeExtensionNode;
use GraphQL\Utils\PhpDoc;
use GraphQL\Utils\Utils;

/** @phpstan-import-type PartialEnumValueConfig from EnumType */
/**
* @phpstan-import-type PartialEnumValueConfig from EnumType
*
* @phpstan-type PhpEnumTypeConfig array{
* name?: string|null,
* description?: string|null,
* enumClass: class-string<\UnitEnum>,
* astNode?: EnumTypeDefinitionNode|null,
* extensionASTNodes?: array<int, EnumTypeExtensionNode>|null
* }
*/
class PhpEnumType extends EnumType
{
public const MULTIPLE_DESCRIPTIONS_DISALLOWED = 'Using more than 1 Description attribute is not supported.';
Expand All @@ -16,16 +28,15 @@ class PhpEnumType extends EnumType
protected string $enumClass;

/**
* @param class-string<\UnitEnum> $enum
* @param string|null $name The name the enum will have in the schema, defaults to the basename of the given class
* @phpstan-param PhpEnumTypeConfig $config
*
* @throws \Exception
* @throws \ReflectionException
*/
public function __construct(string $enum, ?string $name = null)
public function __construct(array $config)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

{
$this->enumClass = $enum;
$reflection = new \ReflectionEnum($enum);
$this->enumClass = $config['enumClass'];
$reflection = new \ReflectionEnum($this->enumClass);

/**
* @var array<string, PartialEnumValueConfig> $enumDefinitions
Expand All @@ -40,31 +51,33 @@ public function __construct(string $enum, ?string $name = null)
}

parent::__construct([
'name' => $name ?? $this->baseName($enum),
'name' => $config['name'] ?? $this->baseName($this->enumClass),
'values' => $enumDefinitions,
'description' => $this->extractDescription($reflection),
'description' => $config['description'] ?? $this->extractDescription($reflection),
'enumClass' => $this->enumClass,
]);
}

public function serialize($value): string
{
if ($value instanceof $this->enumClass) {
$enumClass = $this->enumClass;
if ($value instanceof $enumClass) {
return $value->name;
}

if (is_a($this->enumClass, \BackedEnum::class, true)) {
if (is_a($enumClass, \BackedEnum::class, true)) {
try {
$instance = $this->enumClass::from($value);
$instance = $enumClass::from($value);
} catch (\ValueError|\TypeError $_) {
$notEnumInstanceOrValue = Utils::printSafe($value);
throw new SerializationError("Cannot serialize value as enum: {$notEnumInstanceOrValue}, expected instance or valid value of {$this->enumClass}.");
throw new SerializationError("Cannot serialize value as enum: {$notEnumInstanceOrValue}, expected instance or valid value of {$enumClass}.");
}

return $instance->name;
}

$notEnum = Utils::printSafe($value);
throw new SerializationError("Cannot serialize value as enum: {$notEnum}, expected instance of {$this->enumClass}.");
throw new SerializationError("Cannot serialize value as enum: {$notEnum}, expected instance of {$enumClass}.");
}

public function parseValue($value)
Expand Down
4 changes: 2 additions & 2 deletions tests/Type/EnumTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
use GraphQL\Language\Parser;
use GraphQL\Language\SourceLocation;
use GraphQL\Tests\Type\PhpEnumType\BackedPhpEnum;
use GraphQL\Tests\Type\PhpEnumType\PhpEnum;
use GraphQL\Tests\Type\PhpEnumType\MyCustomPhpEnum;
use GraphQL\Tests\Type\TestClasses\OtherEnumType;
use GraphQL\Type\Definition\EnumType;
use GraphQL\Type\Definition\EnumValueDefinition;
Expand Down Expand Up @@ -715,7 +715,7 @@ enum PhpEnum {

$this->schema = BuildSchema::build($documentNode);
$resolvers = [
'phpEnum' => fn (): PhpEnum => PhpEnum::B,
'phpEnum' => fn (): MyCustomPhpEnum => MyCustomPhpEnum::B,
];

self::assertSame(
Expand Down
2 changes: 1 addition & 1 deletion tests/Type/PhpEnumType/PhpEnum.php → tests/Type/PhpEnumType/MyCustomPhpEnum.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
use GraphQL\Type\Definition\Description;

#[Description(description: 'foo')]
enum PhpEnum
enum MyCustomPhpEnum
{
#[Description(description: 'bar')]
case A;
Expand Down
57 changes: 36 additions & 21 deletions tests/Type/PhpEnumTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
use GraphQL\Tests\Type\PhpEnumType\MultipleDeprecationsPhpEnum;
use GraphQL\Tests\Type\PhpEnumType\MultipleDescriptionsCasePhpEnum;
use GraphQL\Tests\Type\PhpEnumType\MultipleDescriptionsPhpEnum;
use GraphQL\Tests\Type\PhpEnumType\PhpEnum;
use GraphQL\Tests\Type\PhpEnumType\MyCustomPhpEnum;
use GraphQL\Type\Definition\ObjectType;
use GraphQL\Type\Definition\PhpEnumType;
use GraphQL\Type\Definition\ResolveInfo;
Expand All @@ -32,10 +32,12 @@ protected function setUp(): void

public function testConstructEnumTypeFromPhpEnum(): void
{
$enumType = new PhpEnumType(PhpEnum::class);
$enumType = new PhpEnumType([
'enumClass' => MyCustomPhpEnum::class,
]);
self::assertSame(<<<'GRAPHQL'
"foo"
enum PhpEnum {
enum MyCustomPhpEnum {
"bar"
A
B @deprecated
Expand All @@ -46,7 +48,7 @@ enum PhpEnum {

public function testConstructEnumTypeFromIntPhpEnum(): void
{
$enumType = new PhpEnumType(IntPhpEnum::class);
$enumType = new PhpEnumType(['enumClass' => IntPhpEnum::class]);
self::assertSame(<<<'GRAPHQL'
enum IntPhpEnum {
A
Expand All @@ -56,7 +58,10 @@ enum IntPhpEnum {

public function testConstructEnumTypeFromPhpEnumWithCustomName(): void
{
$enumType = new PhpEnumType(PhpEnum::class, 'CustomNamedPhpEnum');
$enumType = new PhpEnumType([
'enumClass' => MyCustomPhpEnum::class,
'name' => 'CustomNamedPhpEnum',
]);
self::assertSame(<<<'GRAPHQL'
"foo"
enum CustomNamedPhpEnum {
Expand All @@ -70,7 +75,7 @@ enum CustomNamedPhpEnum {

public function testConstructEnumTypeFromPhpEnumWithDocBlockDescriptions(): void
{
$enumType = new PhpEnumType(DocBlockPhpEnum::class);
$enumType = new PhpEnumType(['enumClass' => DocBlockPhpEnum::class]);
self::assertSame(<<<'GRAPHQL'
"foo"
enum DocBlockPhpEnum {
Expand All @@ -89,24 +94,32 @@ enum DocBlockPhpEnum {
public function testMultipleDescriptionsDisallowed(): void
{
self::expectExceptionObject(new \Exception(PhpEnumType::MULTIPLE_DESCRIPTIONS_DISALLOWED));
new PhpEnumType(MultipleDescriptionsPhpEnum::class);
new PhpEnumType([
'enumClass' => MultipleDescriptionsPhpEnum::class,
]);
}

public function testMultipleDescriptionsDisallowedOnCase(): void
{
self::expectExceptionObject(new \Exception(PhpEnumType::MULTIPLE_DESCRIPTIONS_DISALLOWED));
new PhpEnumType(MultipleDescriptionsCasePhpEnum::class);
new PhpEnumType([
'enumClass' => MultipleDescriptionsCasePhpEnum::class,
]);
}

public function testMultipleDeprecationsDisallowed(): void
{
self::expectExceptionObject(new \Exception(PhpEnumType::MULTIPLE_DEPRECATIONS_DISALLOWED));
new PhpEnumType(MultipleDeprecationsPhpEnum::class);
new PhpEnumType([
'enumClass' => MultipleDeprecationsPhpEnum::class,
]);
}

public function testExecutesWithEnumTypeFromPhpEnum(): void
{
$enumType = new PhpEnumType(PhpEnum::class);
$enumType = new PhpEnumType([
'enumClass' => MyCustomPhpEnum::class,
]);
$schema = new Schema([
'query' => new ObjectType([
'name' => 'Query',
Expand All @@ -118,9 +131,9 @@ public function testExecutesWithEnumTypeFromPhpEnum(): void
'type' => Type::nonNull($enumType),
],
],
'resolve' => static function ($_, array $args): PhpEnum {
'resolve' => static function ($_, array $args): MyCustomPhpEnum {
$bar = $args['bar'];
assert($bar === PhpEnum::A);
assert($bar === MyCustomPhpEnum::A);

return $bar;
},
Expand All @@ -138,7 +151,9 @@ public function testExecutesWithEnumTypeFromPhpEnum(): void

public function testSerializesBackedEnumsByValue(): void
{
$enumType = new PhpEnumType(IntPhpEnum::class);
$enumType = new PhpEnumType([
'enumClass' => IntPhpEnum::class,
]);
$schema = new Schema([
'query' => new ObjectType([
'name' => 'Query',
Expand All @@ -160,7 +175,7 @@ public function testSerializesBackedEnumsByValue(): void

public function testAcceptsEnumFromVariableValues(): void
{
$enumType = new PhpEnumType(PhpEnum::class);
$enumType = new PhpEnumType(['enumClass' => MyCustomPhpEnum::class]);

$schema = null;
$schema = new Schema([
Expand All @@ -174,16 +189,16 @@ public function testAcceptsEnumFromVariableValues(): void
'type' => Type::nonNull($enumType),
],
],
'resolve' => static function (bool $executeAgain, array $args, $context, ResolveInfo $resolveInfo) use (&$schema): PhpEnum {
'resolve' => static function (bool $executeAgain, array $args, $context, ResolveInfo $resolveInfo) use (&$schema): MyCustomPhpEnum {
$bar = $args['bar'];
assert($bar === PhpEnum::A);
assert($bar === MyCustomPhpEnum::A);

assert($schema instanceof Schema);

if ($executeAgain) {
$executionResult = GraphQL::executeQuery(
$schema,
'query ($bar: PhpEnum!) { foo(bar: $bar) }',
'query ($bar: MyCustomPhpEnum!) { foo(bar: $bar) }',
false,
null,
$resolveInfo->variableValues
Expand All @@ -204,7 +219,7 @@ public function testAcceptsEnumFromVariableValues(): void

$executionResult = GraphQL::executeQuery(
$schema,
'query ($bar: PhpEnum!) { foo(bar: $bar) }',
'query ($bar: MyCustomPhpEnum!) { foo(bar: $bar) }',
true,
null,
['bar' => 'A']
Expand All @@ -218,7 +233,7 @@ public function testAcceptsEnumFromVariableValues(): void

public function testFailsToSerializeNonEnum(): void
{
$enumType = new PhpEnumType(PhpEnum::class);
$enumType = new PhpEnumType(['enumClass' => MyCustomPhpEnum::class]);
$schema = new Schema([
'query' => new ObjectType([
'name' => 'Query',
Expand All @@ -233,13 +248,13 @@ public function testFailsToSerializeNonEnum(): void

$result = GraphQL::executeQuery($schema, '{ foo }');

self::expectExceptionObject(new SerializationError('Cannot serialize value as enum: "A", expected instance of GraphQL\\Tests\\Type\\PhpEnumType\\PhpEnum.'));
self::expectExceptionObject(new SerializationError('Cannot serialize value as enum: "A", expected instance of GraphQL\\Tests\\Type\\PhpEnumType\\MyCustomPhpEnum.'));
$result->toArray(DebugFlag::RETHROW_INTERNAL_EXCEPTIONS);
}

public function testFailsToSerializeNonEnumValue(): void
{
$enumType = new PhpEnumType(IntPhpEnum::class);
$enumType = new PhpEnumType(['enumClass' => IntPhpEnum::class]);
$schema = new Schema([
'query' => new ObjectType([
'name' => 'Query',
Expand Down
Loading