Skip to content
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

fix: handle item iri with identifiers in LegacyIriConverter #5670

Merged

Conversation

jonnyeom
Copy link
Contributor

Q A
Branch? 2.7
Tickets #5620
License MIT

Hello, a small fix for when using the \ApiPlatform\Core\Api\LegacyIriConverter

The Problem:

#5630 Introduced a nice way to fix the IriConverterInterface deprecation before turning off the metadata_backward_compatibility_layer.

One issue I ran into is that LegacyIriConverter::getIriFromResource decides if the iri will be Item level or Collection level solely based on if the given $item parameter is a string vs an object.

With this methodology, I would have instantiate a new object just to trigger the Item level iri conversion.
(To compare, The new IriConverterInterface handles this decision smartly, using the metadata of the Operation and the given Object).

Proposed solution

Previously, the old IriConverterInterface had 2 distinct methods to split this (getItemIriFromResourceClass and getIriFromResourceClass)— the only difference being an extra parameter array $identifiers.

This adds back the ability to identify a Resource based on className + identifiers.

@jonnyeom jonnyeom marked this pull request as draft July 13, 2023 18:01
@jonnyeom jonnyeom marked this pull request as ready for review July 13, 2023 18:18
Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

Nitpicking, otherwise LGTM, but I'll wait for @soyuka review before merging because he knows this part better!

src/Core/Api/LegacyIriConverter.php Outdated Show resolved Hide resolved
Co-authored-by: Kévin Dunglas <kevin@dunglas.fr>
Copy link
Member

@soyuka soyuka left a comment

Choose a reason for hiding this comment

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

lgtm

@soyuka soyuka merged commit deed442 into api-platform:2.7 Aug 1, 2023
28 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants