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 support for clearing relationships #33

Merged
merged 1 commit into from
Aug 8, 2016

Conversation

emil-nasso
Copy link
Contributor

If you sent a request, trying to clear a relationship (remove the relationship) by sending "relationshipname" : { "data": null}" yin ignored this, behaving exactly like it did when the relationship was completely omitted.
The hydrator for the relationship was not called and calls to $jsonApi->getRequest()->getResourceToOneRelationship('relationshipname') behaved just as if no relationship was sent.
This patch intends to change that by handling the null-case and creating empty ToOneRelationships where appropriate.

A request for clarification of this behavior has been made to the folks over at jsonapi, here: json-api/json-api#1070

@emil-nasso emil-nasso force-pushed the empty-relationships branch 2 times, most recently from b567e21 to 953b8c3 Compare August 4, 2016 08:37
If you sent a request, trying to clear a relationship (remove the relationship) by sending `"relationshipname" : { "data": null}"` yin ignored this, behaving exactly like it did when the relationship was completely omitted.
The hydrator for the relationship was not called and calls to `$jsonApi->getRequest()->getResourceToOneRelationship('relationshipname')` behaved just as if no relationship was sent.
This patch intends to change that by handling the null-case and creating empty ToOneRelationships where appropriate.
@kocsismate
Copy link
Member

You did a great job with your PR and I couldn't find anything which needs fixing (apart from some really minor formatting things)! :) I'll gladly take care of updating the documentation, change log and tests this evening, but I won't stop you here if you want to do them.

And I think the best thing would be to throw exceptions when either the type or the id field is missing from the resource identifier object, because the specs explicitly declares that they are required: http://jsonapi.org/format/#document-resource-identifier-objects For this purpose a new ResourceIdentifierTypeMissing and a ResourceIdentifierIdMissing exception could be used via the ExceptionFactory. If you also think that this is the best solution I will make this change too.

Furthermore, I noticed one thing I don't like: it is the names of the getResourceToOneRelationship() and getResourceToManyRelationship() methods. They are really awkward and I can't remember why I named them like that. :) But what would you think if I renamed them to getToOneRelationship() and getToManyRelationship()? And maybe the JSON API specific functionality should be separated from the generic RequestInterface to a JSON API specific class. And probably one could access them the following way:

$authors = $jsonApi->getRequest()->getJsonApi()->getToOneRelationship("authors");

But this is really long a line :S But if you have a good solution, feel free to tell me :)

@kocsismate kocsismate merged commit 0aac12b into woohoolabs:master Aug 8, 2016
@emil-nasso
Copy link
Contributor Author

+1 on the exception suggestion and the renaming. :) The exception thing shouldn't be a breaking change, i think, but the renaming of the methods are.

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.

2 participants