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

Normalize non-resource objects in a standalone normalizer #2679

Merged
merged 2 commits into from
Apr 6, 2019

Conversation

teohhanhui
Copy link
Contributor

@teohhanhui teohhanhui commented Apr 2, 2019

Q A
Bug fix? yes
New feature? no
BC breaks? only for a new change introduced in 2.4
Deprecations? no
Tests pass? yes
Fixed tickets api-platform/api-platform#1085
License MIT
Doc PR N/A

Note: Includes a minor BC break because we undo a new change introduced in 2.4. I think it's okay because $handleNonResource was internal anyway. The only people affected would be those who have extended AbstractItemNormalizer and explicitly pass that optional parameter (which, actually, still wouldn't result in an error anyway).

TODO:

@@ -179,7 +179,11 @@ Feature: DTO input and output
},
"@type": "User",
"@id": "/users/1",
"dummy": "/dummies/1"
"dummy": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should probably be handled better?

Copy link
Member

Choose a reason for hiding this comment

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

It'd be better to have @id as a string no?

@bastnic
Copy link
Contributor

bastnic commented Apr 3, 2019

seems cleaner (and better performance, I hope)

@toriqo
Copy link
Contributor

toriqo commented Apr 4, 2019

when using application/vnd.api+json or application/hal+json, non-resources are now properly serialized and returned based on contexts

@soyuka
Copy link
Member

soyuka commented Apr 4, 2019

@teohhanhui my test case also covers @toriqo issue, should I duplicate it to make it run with other formats?

@teohhanhui
Copy link
Contributor Author

@soyuka I'll take care of that, since I've been working on something since yesterday.

@teohhanhui teohhanhui force-pushed the fix/normalize-non-resource branch 3 times, most recently from 4812d4e to d9bb3c1 Compare April 5, 2019 12:55
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.

I like it! Really good to have removed this handleNonResource parameter and I feel that the code is a step forward to simplify composition! Good job!

.travis.yml Show resolved Hide resolved
@bastnic
Copy link
Contributor

bastnic commented Apr 5, 2019

I like it! Really good to have removed this handleNonResource parameter and I feel that the code is a step forward to simplify composition! Good job!

+100

@teohhanhui teohhanhui force-pushed the fix/normalize-non-resource branch 3 times, most recently from 31ea473 to 2893263 Compare April 5, 2019 18:43
@teohhanhui teohhanhui force-pushed the fix/normalize-non-resource branch from 2893263 to 8f27927 Compare April 5, 2019 21:22
@teohhanhui teohhanhui force-pushed the fix/normalize-non-resource branch from 8f27927 to 521fa98 Compare April 6, 2019 08:53
@soyuka soyuka merged commit b7afbf8 into api-platform:2.4 Apr 6, 2019
@soyuka
Copy link
Member

soyuka commented Apr 6, 2019

Thanks @teohhanhui

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants