core icon indicating copy to clipboard operation
core copied to clipboard

feat(mercure): Pass 'resource_class' into serialization context

Open kgilden opened this issue 4 years ago • 4 comments

Q A
Branch? 2.6
Tickets n/a
License MIT
Doc PR n/a

The $context array passed to custom normalizers is different whether a resource is being serialized as part of a request-response lifecycle or whether it's being serialized to publish to the Mercure hub. This makes it difficult to ensure that the same resource gets serialized consistently.

The resource_class key defines the root resource getting serialized and it's missing when serializing for the Mercure hub. To not rock the boat too much, I decided just to add this to the serialization context.

Questions to reviewers

  1. Is this perhaps small enough of a feature that it could be merged into current stable (2.6) branch?
  2. If documentation update is necessary, which article should I update?

Future work

In the longer run it might be beneficial to ensure the following.

  1. All keys of the context variable declared by API Platform are documented (including their types).
  2. All keys of the context variable are the same regardless of whether serializing during a request-response lifecycle or whether serializing to publish a Mercure update. If a context variable makes sense only in one of the two cases, then its type can simply by T|null.

But, I acknowledge this might be a lot of work and I don't want to burden the maintainers by opening yet another issue.

Background

Making this PR to make my workaround for #4338 work.

I myself came up with a workaround, which uses the resource_class context property to determine what the root is and just use an IRI converter for all other cases. But this falls short, when trying to publish to a Mercure hub, because the context is created differently (here vs here).

kgilden avatar Nov 27 '21 15:11 kgilden

you can patch this on 2.6 if you want but please also add the fix on main if you can thanks

soyuka avatar Dec 03 '21 09:12 soyuka

+1 on my side when @soyuka's review will be handled.

I also agree with your other proposals.

dunglas avatar Dec 04 '21 03:12 dunglas

Alright, sounds good. I'll try to see if I can make the fixes some time later this week.

kgilden avatar Dec 07 '21 16:12 kgilden

@soyuka I missed the train to 2.6.7, but I made the changes. As for adding the fix also to main, do you mean create another PR against the main branch?

kgilden avatar Dec 27 '21 13:12 kgilden

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Nov 04 '22 21:11 stale[bot]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Jan 03 '23 21:01 stale[bot]