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(serializer): Guess uri variables with the operation and the data instead of hardcoding id #5546

Merged
merged 2 commits into from
May 5, 2023

Conversation

Aerendir
Copy link
Contributor

@Aerendir Aerendir commented Apr 15, 2023

Q A
Branch? main
Tickets #5545
License MIT

In a situation where these two conditions are true, also alternatively and not necessarily at the same time, the code will break:

  1. The IRI has some parameters (ex.: account/{account}/resource/{resource}/sub_resource/{sub_resource}
  2. The uri parameter of the resource is not id (in the above example, the uri parameter is sub_resource)

Unable to generate an IRI for the item of type "App\Entity\ResourceLine""

Why it breaks

In ItemNormalizer:79 there is this todo:

// todo: we could guess uri variables with the operation and the data instead of hardcoding id
$iri = $this->iriConverter->getIriFromResource($context['resource_class'], UrlGeneratorInterface::ABS_PATH, $operation, ['uri_variables' => ['id' => $data['id']]]);

As you can see, the context passed to $this->iriConverter->getIriFromResource() (4th parameter) is hardcoded and always refers to the ID of the current denormalizing entity.

This is not correct (and, in fact, there is also the todo to remember this fact) as this could break the code.

@Aerendir Aerendir changed the title Fix #5545. Guess uri variables with the operation and the data instead of hardcoding id Apr 15, 2023
@Aerendir Aerendir changed the title Guess uri variables with the operation and the data instead of hardcoding id feat: Guess uri variables with the operation and the data instead of hardcoding id Apr 15, 2023
@Aerendir Aerendir marked this pull request as ready for review April 15, 2023 14:51
@Aerendir Aerendir requested a review from mrossard April 15, 2023 14:51
@Aerendir Aerendir changed the title feat: Guess uri variables with the operation and the data instead of hardcoding id fix(serializer): Guess uri variables with the operation and the data instead of hardcoding id Apr 15, 2023
@Aerendir Aerendir changed the title fix(serializer): Guess uri variables with the operation and the data instead of hardcoding id fix(serializer): guess uri variables with the operation and the data instead of hardcoding id (#5545) Apr 15, 2023
@Aerendir Aerendir changed the title fix(serializer): guess uri variables with the operation and the data instead of hardcoding id (#5545) fix(serializer): Guess uri variables with the operation and the data instead of hardcoding id Apr 15, 2023
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.

nice!

src/Serializer/ItemNormalizer.php Outdated Show resolved Hide resolved
src/Serializer/ItemNormalizer.php Outdated Show resolved Hide resolved
@soyuka
Copy link
Member

soyuka commented Apr 28, 2023

Can you target 3.1 as this is a bug fix?

@Aerendir Aerendir changed the base branch from main to 3.1 April 28, 2023 16:13
@Aerendir
Copy link
Contributor Author

@soyuka , done: the PR now targets 3.1.

@tacman
Copy link
Contributor

tacman commented Apr 28, 2023

Thanks! Can you document this an provide an example in the official docs? It's been something I've wanted and worked with subresources in 2.6 but much harder to do in 3.0, in part because there's so few examples.

@soyuka
Copy link
Member

soyuka commented Apr 29, 2023

I'll rebase that thanks!

@Aerendir
Copy link
Contributor Author

Aerendir commented Apr 29, 2023

@tacman , it is already documented: https://api-platform.com/docs/core/subresources/

This PR simply allows to use an URI like /companies/{companyId}/employees/{employeeId} while before you were forced to use one like /companies/{companyId}/employees/{id} (like in the example provided in the link).

@soyuka
Copy link
Member

soyuka commented May 3, 2023

This will be out friday

@soyuka soyuka merged commit ed4bca9 into api-platform:3.1 May 5, 2023
@soyuka
Copy link
Member

soyuka commented May 5, 2023

thanks @Aerendir

@soyuka
Copy link
Member

soyuka commented Aug 30, 2023

Well we're back on this as this can not work, I should've been more careful but this is not how things are supposed to work. Use IRIs or a custom normalizer if you need to do weird stuff.

@Aerendir
Copy link
Contributor Author

@soyuka , please, can you explain better?

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.

4 participants