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

Add links for all available includes to JsonApiSerializer #331

Merged
merged 5 commits into from
Jun 29, 2019
Merged

Add links for all available includes to JsonApiSerializer #331

merged 5 commits into from
Jun 29, 2019

Conversation

matt-allan
Copy link
Contributor

@matt-allan matt-allan commented Sep 22, 2016

This PR changes the JsonApiSerializer to always add a 'relationships' key with links. Before the serializer would only add links if the resource was included.

I had to update a lot of tests since the output changes. This is a breaking change since new data is added to the output, but nothing is removed from the output so it shouldn't actually break any clients.

Closes #292 & #242. Supersedes the changes in #272, so that PR should not be merged if this one is.

Here is a link to the relevant part of the JSON:API specification.

@greydnls
Copy link
Contributor

👍 I really like this implementation and it makes sense to return the available includes as relationships.

This is a breaking change, though. It will need to wait for 1.0.

@greydnls greydnls added this to the 1.0.0 milestone Dec 28, 2016
greydnls
greydnls previously approved these changes Dec 28, 2016
@matt-allan
Copy link
Contributor Author

Cool, thanks Graham!

@michaelcurry
Copy link

I was about to start working on this but now I can move onto other things. Thanks @yuloh!

@michaelcurry
Copy link

Looking for an update on this PR. IS there a time line for getting this merged in?

* @param array $resource The resource to add relationship links to
* @param string $relationshipKey The resource key of the relationship
*/
private function addRelationshipLinks($resource, $relationshipKey)
Copy link

Choose a reason for hiding this comment

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

Would it make sense to make this method protected? I can see cases where it would be nice to have a custom link structure here.

@greydnls
Copy link
Contributor

greydnls commented Apr 5, 2017

After some consideration I'm going to go ahead and merge this pre-1.0.

It'll be going in to 0.17.0 which should be tagged within the next week.


return $resource;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is a /** missing.

@matt-allan
Copy link
Contributor Author

Looks like the merge went bad. I will push a fix soon.

@johannesschobel
Copy link
Contributor

any news on this?

@philtweir
Copy link

As far as I can tell, adding the change suggested by @Art4 lets the build tests pass, (tested with the more recent changes on current master pulled in) - is there anything we can do to help update?

@matt-allan
Copy link
Contributor Author

Sorry it took so long everybody.

@greydnls this PR should be ready to merge now.

@dennisschroer
Copy link

Any update on this one?

@eflorit
Copy link

eflorit commented Aug 17, 2018

Is this PR going to be merged? It's been over a year now. :(

@Art4
Copy link
Contributor

Art4 commented Dec 4, 2018

Any updates?

@milroyfraser
Copy link

milroyfraser commented Jan 26, 2019

@yuloh there are cases where we have includes which are not relationships.

e.g.
include comment_count instead of comments
/posts?include=comment_count

this can be appended to the resource attributes by returning a primitive resource from the include method.

How about allowing to define $includeRelationshipsLinks and use $availableIncludes as fallback may be.

Nice job!

@matthewtrask
Copy link
Contributor

@matt-allan is this still active?

@matt-allan
Copy link
Contributor Author

It’s not really active, I’ve just been waiting for it to be merged. Last I heard it was approved to merge.

@matthewtrask
Copy link
Contributor

@matt-allan a few things have changed, so we will look to getting this merged. There is a merge conflict that needs to be addressed.

@matt-allan
Copy link
Contributor Author

@matthewtrask I fixed the merge conflicts and updated the tests.

Copy link
Contributor

@KorvinSzanto KorvinSzanto left a comment

Choose a reason for hiding this comment

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

👍

@KorvinSzanto KorvinSzanto merged commit f1a3d71 into thephpleague:master Jun 29, 2019
@matt-allan matt-allan deleted the link-available-includes branch June 29, 2019 17:01
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.

Auto-Add links for available includes