Skip to content

[Subresource] Fixed the serialization context builder #1617

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

Closed
wants to merge 1 commit into from

Conversation

lyrixx
Copy link
Contributor

@lyrixx lyrixx commented Dec 28, 2017

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? ???
Fixed tickets #1616
License MIT
Doc PR

Before going further, I would like to know if this is the right direction.

@dunglas dunglas requested a review from soyuka December 28, 2017 15:50
@@ -111,6 +111,8 @@ public function load($data, $type = null): RouteCollection
'property' => $operation['property'],
'identifiers' => $operation['identifiers'],
'collection' => $operation['collection'],
'parent_resource_class' => $resourceClass,
Copy link
Member

Choose a reason for hiding this comment

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

We can't say that it's the "parent" because there can be an "infinity" IMO.
Anyway, we need to discuss here is whether we take the "Root" as the metadata entrypoint or the "Queue" (last).
Explanation:

/foo/1/bars/2

This path will get you an entity of type Bar. When this is serialized, where should we look to get the normalization_context?

  • Bar metadata, after all this is a Bar response
  • Foo metadata, because it's a subresource

We need to choose one. At the moment, IIRC the code looks on the Bar metadata to find the normalization_context for the given operation.

If we want to take the "Root" (because this is where we declared the subresource), I don't think that you need the "parent_resource_class".
Take a look at the SubresourceOperationFactory:

$rootClass = $identifiers[0][1] is where you want to look at.
the operation_name should be the correct one IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I vote for:

Foo metadata, because it's a subresource


$rootClass = $identifiers[0][1] is where you want to look at.

yes I noticed that. But I thought it was risky to use that ;)

Copy link
Member

Choose a reason for hiding this comment

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

yes I noticed that. But I thought it was risky to use that ;)

It is not and it's meant for that ;)

@soyuka
Copy link
Member

soyuka commented Dec 28, 2017

Anyway I think there are edge cases that may be hard to cover. I haven't slept enough to give a more detailled though on this haha but I'll think about it.

@lyrixx
Copy link
Contributor Author

lyrixx commented Dec 28, 2017

No problem. But for now (and for me) the subresource system is not usable yet :/ as I can not control what the API returns.

@lyrixx
Copy link
Contributor Author

lyrixx commented Jan 4, 2018

So what do you think of this ? Is it the right approach?
I'm adding more sub resource in my API, and right now it's a nightmare to configure it. It's really hard to find where to put the configuration + how to name the operation. Why this PR, everything become much more simple & natural

@soyuka
Copy link
Member

soyuka commented Jan 4, 2018

Having the RootClass as a base metadata for the context seems indeed better.

Though, you don't need parent_resource_class and you should use $identifiers[0][1] as stated above.

@lyrixx
Copy link
Contributor Author

lyrixx commented Jan 4, 2018

Sure. I will update the pr according to

@soyuka
Copy link
Member

soyuka commented Apr 7, 2019

Closing in favor of #2706

@soyuka soyuka closed this Apr 7, 2019
@lyrixx lyrixx deleted the subresource-path branch April 7, 2019 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants