-
-
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
test: Backed enum resource tests #6288
Conversation
d859ba1
to
cb41845
Compare
cb41845
to
3785ce0
Compare
31ecb0c
to
d18e262
Compare
… translatable (GwendolenLynch) This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [Serializer] reset backed_enum priority, and re-prioritise translatable | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #54478 Fix api-platform/core#6285 Fix api-platform/core#6279 | License | MIT - `serializer.normalizer.translatable` -920 (was -890) - `serializer.normalizer.backed_enum` -915 (originally -915, changed to -880) Floating this as as solution to the knock-on issues from #54478 Context: - #54478 (comment) - api-platform/core#6288 Commits ------- b559aa5 [Serializer] reset backed_enum priority, and re-prioritise translatable
Symfony have merged #54484 so I've taken this out of draft. I've added a conflict for |
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.
I really like this but I won't be able to merge it right away as we need to work on the Hydra implementation. I reverted the priority change on our source code 2a87671
Nice addition, API Platform supports enums only on properties for documentation via OpenAPI and using GraphQl but we did not add the support for RDF formats such as JSON-LD. #[ApiResource(operations: [])] // An enum can't have operations so this needs to be fixed in he Metadata Factory
enum BackgroundColor: string
{
case Sand = 'sand';
case Magenta = 'magenta';
case Teal = 'teal';
case Blue = 'blue';
case Mauve = 'mauve';
/**
* @return string[]
*/
public static function values(): array
{
return array_column(self::cases(), 'value');
}
} My reference for the work is what's done on schema.org, for example: https://schema.org/EnergyEfficiencyEnumeration This is of type Enumeration and you can see it's "Enumeration members" at https://schema.org/EUEnergyEfficiencyEnumeration. When looking at the schema.org jsonld https://github.com/schemaorg/schemaorg/blob/main/data/releases/26.0/schemaorg-all-https.jsonld we find lots of examples: {
"@id": "schema:GamePlayMode",
"@type": "rdfs:Class",
"rdfs:comment": "Indicates whether this game is multi-player, co-op or single-player.",
"rdfs:label": "GamePlayMode",
"rdfs:subClassOf": {
"@id": "schema:Enumeration"
}
}, And one of the value of the enum has this type: {
"@id": "schema:SinglePlayer",
"@type": "schema:GamePlayMode",
"rdfs:comment": "Play mode: SinglePlayer. Which is played by a lone player.",
"rdfs:label": "SinglePlayer"
}, https://github.com/schemaorg/schemaorg/blob/c7dc8e5a3ad9f1e1645e282a18e46b5d2c19a657/data/releases/26.0/schemaorg-all-https.jsonld#L24689-L24694 (there are other values on this enum, just like for EnergyEfficiency). So, for each value of the Backed Enum we should provide a given type to be referenced in the |
Suggestion on that, it is probably safe to revert the change to Oh, and it is probably worth at least cherry-picking the composer change from here as that will prevent Symfony "broken" versions. |
No stress. I was more trying to get coverage for how it is now, as it is "in the wild" as far as use (I'm also guilty there) … supported or not. |
Actually this is a bad test, I already picked the change on
I plan to do that indeed.
No problem we can work on integrating this, I'm afraid that it won't get into 3.3 as I'm already a month late on that release but it's definitely going to be a top priority for API Platform 3.4. |
'value' => 30, | ||
], | ||
], | ||
]); |
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 is probably correct, @dunglas could you review these tests?
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.
LGTM
ad6d402
to
0c01d40
Compare
To catch future versions of some of the issues hit, I have added test for
Edit: GraphQL additions are based on https://gist.github.com/dunglas/f2e756725b1b842331dc8cdd38b4cc75 |
97df211
to
54ebdf2
Compare
492ef97
to
4637e8e
Compare
composer.json
Outdated
@@ -82,17 +82,18 @@ | |||
"symfony/expression-language": "^6.1 || ^7.0", | |||
"symfony/finder": "^6.1 || ^7.0", | |||
"symfony/form": "^6.1 || ^7.0", | |||
"symfony/framework-bundle": "^6.1 || ^7.0", | |||
"symfony/framework-bundle": "^6.4.3 || ^7.0", |
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.
let's target main or remove composer changes?
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.
Updated to main
. There is no way to get --prefer-lowest
to pass without these.
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.
but is this tight to enums?
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.
symfony/framework-bundle
change seems to be not needed now the rest of the SF components are ^6.4
so I've dropped it.
symfony/phpunit-bridge
is to include symfony/symfony#52844 which meant the tests just wouldn't run
Warning: The lock file is not up to date with the latest changes in composer.json. You may
be getting outdated dependencies. It is recommended that you run `composer update` or
`composer update <package name>`.
- Required package "symfony/phpunit-bridge" is not present in the lock file.
symfony/string
is 100% about enums, and was the hardest to track down at the time. The change does mirror the constraint used in the component composer.json
files.
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.
Anticipating the question "Why isn't CI failing?" (I had that question too), see this example from main
yesterday:
https://github.com/api-platform/core/actions/runs/8679784849/job/23799224716?pr=6309
PHPUnit is not installed. Please run ./vendor/bin/simple-phpunit to install it
There seems to be a new-ish issue with CI set-up.
Originally I caught the failure on CI and then moved to local testing where it work correctly with 6.4.1
+
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.
See #6113
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.
I want to bump to 6.4 for all dependencies (on main) but I need to do this in every components (needs a feature on the monorepo plugin) hopefully this is done this week
4637e8e
to
d3380bf
Compare
e651c1d
to
b8b11d4
Compare
b8b11d4
to
c99011d
Compare
composer.json
Outdated
@@ -89,11 +89,12 @@ | |||
"symfony/maker-bundle": "^1.24", | |||
"symfony/mercure-bundle": "*", | |||
"symfony/messenger": "^6.4 || ^7.0", | |||
"symfony/phpunit-bridge": "^6.4 || ^7.0", | |||
"symfony/phpunit-bridge": "^6.4.1 || ^7.0", |
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.
"symfony/phpunit-bridge": "^6.4.1 || ^7.0", | |
"symfony/phpunit-bridge": "^6.4 || ^7.0", |
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.
It will disappear on rebase when added to #6316 👍
also we need to add the enum to the jsonld context, something like this:
|
c99011d
to
fe6ac3b
Compare
tyvm @GwendolenLynch ! Looking forward to merging the enum feature next (I still need to try the branch and see what I can improve on the URI thing). |
* test: backed enum resources * chore: update minimal dev dependencies
* test: backed enum resources * chore: update minimal dev dependencies
* test: backed enum resources * chore: update minimal dev dependencies
@soyuka this is just an updated version of the tests I linked yesterday when discussing #6264 & #6279 … and if I m still wrong please just close this PR. 👍
These tests illustrate what was working for
json
,jsonhal
, andjsonld
formats, but breaks now with the change in symfony/symfony#54315However, if a fix/workaround is found these tests will still add additional coverage for enum resources.
Notes:
3.2
as-ofv3.2.19
(i.e. pre fix(graphql): increment graphql normalizer priority #6283)symfony/framework-bundle
<=7.0.5
(i.e. pre [Serializer] Fixed BackedEnumNormalizer priority for translatable enum symfony/symfony#54315)Missing(Derp! serializer !== schema factory)jsonapi
tests due to target branch