-
-
Notifications
You must be signed in to change notification settings - Fork 894
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
feat: BackedEnum resources #6309
Conversation
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.
would love to see json-ld supported, I need to add my commit to this at some point :)
8c96867
to
8acce05
Compare
I would use URIs in GraphQL too, as we do for existing IDs and relations. WDYT? |
@dunglas it must be my week for getting confused. 🌞 I'm very happy to make required changes, but could you be so kind as to give a bit more context, please? |
Please look at how I configure a backed enum as an API resource so that each instance has an IRI just like any other API resource. https://github.com/gnito-org/problem-replicator-api-platform-enum-example This is important for machine discovery of APIs and for consistency. |
Thank you @gnito-org, I can reproduce it here. |
37672db
to
b088d10
Compare
e227f3c
to
a069f68
Compare
@GwendolenLynch I merged #6288 could you rebase I this should fix some of the failing tests right? |
a069f68
to
a232425
Compare
I'll continue working on this tomorrow, hopefully merging it! |
84612ab
to
cc755bf
Compare
cc755bf
to
f5e7ad2
Compare
<service id="api_platform.state_provider.backed_enum" class="ApiPlatform\State\Provider\BackedEnumProvider"> | ||
<tag name="api_platform.state_provider" key="ApiPlatform\State\Provider\BackedEnumProvidevr" /> | ||
<tag name="api_platform.state_provider" key="api_platform.state_provider.backed_enum" /> | ||
</service> |
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.
For the record @GwendolenLynch instead of decorating the provider chain, I choose to declare this as an operation provider instead. This will be called inside the ReadProvider:
core/src/State/Provider/ReadProvider.php
Lines 75 to 79 in ef4812a
try { | |
$data = $this->provider->provide($operation, $uriVariables, $context); | |
} catch (ProviderNotFoundException $e) { | |
$data = null; | |
} |
It's easier for userland to decorate our enum provider (or any item provider) instead of hooking into the main decoration chain.
use ApiPlatform\Metadata\Operation; | ||
use Symfony\Component\Serializer\Attribute\Groups; | ||
|
||
trait BackedEnumStringTrait |
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.
For further readers, this was duplicated from the BackedEnumTrait as graphql "guesses" the type from an union:
core/src/GraphQl/Type/FieldsBuilder.php
Lines 232 to 239 in ef4812a
// guess union/intersect types: check each type until finding a valid one | |
foreach ($propertyTypes as $propertyType) { | |
if ($fieldConfiguration = $this->getResourceFieldConfiguration($property, $propertyMetadata->getDescription(), $propertyMetadata->getDeprecationReason(), $propertyType, $resourceClass, $input, $operation, $depth, null !== $propertyMetadata->getSecurity())) { | |
$fields['id' === $property ? '_id' : $this->normalizePropertyName($property, $resourceClass)] = $fieldConfiguration; | |
// stop at the first valid type | |
break; | |
} | |
} |
I'm not a huge fan, ideally we should provide a fix at some point there, even though when representing resources one should avoid scalar union types in my opinion.
Thanks @GwendolenLynch! |
* fix(metadata): Only add GET operations for enums when ApiResource doesn't specify operations * feat(state): backed enum provider * fix(metadata): enum resource identifier default to value * fix(metadata): get method metadata for BackedEnums * test: resource with enum properties schema * what I would like * test: backed enums --------- Co-authored-by: soyuka <soyuka@users.noreply.github.com>
* fix(metadata): Only add GET operations for enums when ApiResource doesn't specify operations * feat(state): backed enum provider * fix(metadata): enum resource identifier default to value * fix(metadata): get method metadata for BackedEnums * test: resource with enum properties schema * what I would like * test: backed enums --------- Co-authored-by: soyuka <soyuka@users.noreply.github.com>
} | ||
|
||
$newGraphQlOperations = []; | ||
foreach ($resourceMetadata->getGraphQlOperations() as $operationName => $operation) { |
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.
This does not work if there are no GraphQlOperations
Hi, @soyuka could you please send me a link to the docs please? (we tried to use it, but no provider registered message is comming up) |
Intended for 3.4, so now it is just for review.
Depends on #6288 as I want to refactor
BackedEnumPlainResourceTest
test methods into those classes.BackedEnum
svalue
by default, and implementingfunction getId()
, etc, works tooGet
&GetCollection
unless otherwise configuredGET /statuses
GET /statuses/1