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

Max depth issues when used with "embedded" relations #246

Merged
merged 5 commits into from
Apr 15, 2017

Conversation

goetas
Copy link
Collaborator

@goetas goetas commented Apr 12, 2017

Fixes #236 #136
Closes #237 #244

Hi, this is an alternative version of #237 and #244 that fixes some issues with @MaxDepth when used inside "embedded" relations.

I'm currently the maintainer of jms/serializer, this requires the 1.6.1 (currently the latest) version, I've spent days to figure out the issue and to properly test it. 😥 But here it is. The XML version was even more complicated that uncovered also a bug in the serializer library.

Is really similar to what @adrienbrault proposed long time ago, but for me that solution never worked when was used in combination with @Embedded. The issue was that the max-depth configs were not copied to the RelationPropertyMetadata.
This version does not require additional classes ( the empty EmbeddedPropertyMetadata class ) as in the @adrienbrault's version

jms/serializer requires PHP 5.5, this forces this library to use at least php 5.5...

@goetas
Copy link
Collaborator Author

goetas commented Apr 12, 2017

The biggest change is in Hateoas\Model\Embedded that has a new "mandatory" parameter (the RelationPropertyMetadata). I guess Embedded can be considered an internal class and not part of a public API, so should not be considered as BC-break.

@willdurand @adrienbrault If you think that this new mandatory parameter is a BC break, I'm ready to provide an alternative solution to it.

Copy link
Contributor

@adrienbrault adrienbrault left a comment

Choose a reason for hiding this comment

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

Looks good! What commit/fix in the serializer is required for this to work ?

@goetas
Copy link
Collaborator Author

goetas commented Apr 12, 2017

schmittjoh/serializer@793406a is the commit that fixed it (the check is done inside isEmptyObject private method), only the XML case was affected.

The JSON case was ok from the serializer point of view.

@goetas
Copy link
Collaborator Author

goetas commented Apr 15, 2017

ping 😃

@willdurand willdurand merged commit 7e1a233 into willdurand:master Apr 15, 2017
@willdurand
Copy link
Owner

Thanks @goetas

@goetas
Copy link
Collaborator Author

goetas commented Apr 15, 2017

thanks to you guys for the hateoas library.
i'm working on some performance improvements when using heavily embedded relations combined with doctrine... as soon as i have hews will open a PR

This was referenced Apr 15, 2017
@willdurand
Copy link
Owner

top!

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.

MaxDepth behaviour has changed
3 participants