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 strategy #4

Merged
merged 1 commit into from
Jun 1, 2013

Conversation

adrienbrault
Copy link
Contributor

Hey,

Following #3, here's the depth exclusion strategy.

return false;
}

return $property->maxDepth <= $navigatorContext->getDepth();
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'm wondering if the <= should be replaced by <.

Should the MaxDepth annotation mean "include only if current class is at maximum depth X", or "include only if the property will be at maximum depth X" ? (Currently its the second one)

Copy link
Owner

Choose a reason for hiding this comment

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

If I'm not mistaken, a @MaxDepth(1) would currently always skip the property, no? If that's the case, I'd use <.

I'm also wondering whether the name makes sense as the depth is not calculated relative to the property on which it is declared. What I would expect is that if I add the annotation on a property is the meaning "serialize the data of this property up to a maximum depth of X", but what it actually means is "serialize the data of this property if the current depth is smaller than X". I'm not sure if we can make this clearer somehow.

Copy link
Contributor

Choose a reason for hiding this comment

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

when i saw this, i was assuming as well that this controls to what depth the objects in that property get serialized. the other behaviour of saying only serialize if not already that deep is rather tricky, as in the object graph the same object might be at different depths depending on where you start.

what if we would call this setting OnlyAtMaxDepth (or something nicer?) and keep MaxDepth for the other use case?

Choose a reason for hiding this comment

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

I agree there needs to be a distinction.
How about RelativeMaxDepth / AbsoluteMaxDepth

@Narretz
Copy link

Narretz commented Feb 18, 2013

Any news on this? I rebased it on master, it still works, but I was also expecting different behavior, i.e. "serialize this property to a max depth of x" Or, for clarification, I want to use a maxDepth annotation for a class which has children of the same class (tree), and I want to limit the serialization of the children to a certain level. I am confused now if this is in the scope here.

Actually, the way it works now is appropriate. If you have a tree, the other behavior would't limit the serialization of children, because you would e.g. serialize 3 levels deep from each child, as you'd begin at 0 every time. At the moment you begin at 0 and serialize the children until you are at depth 3.

@bendavies
Copy link

Is there any movement on this?

@bendavies
Copy link

is there an other way to achieve this at the moment?
We'd be looking to use this to limit the depth of Doctrine ManyToMany self referencing associations.

@Narretz
Copy link

Narretz commented Mar 21, 2013

It should technically work. Starting with the serialized object, you have a depth of 1. When you set MaxDepth on your relation to 2, it will serialize the relation once, and if the relation has the same relation, it will serialize it once more. (if $property->maxDepth < $context->getDepth())

I remember, getDepth() had a little weird behavior, as it will count arrayCollections too, so you might have to look at that. In general, the current Serializer architecture makes it pretty trivial to implement your own Exclusion strategy.

@Bendihossan
Copy link

Is there an update for this? Been a few months now. I'm also looking to do this ti limit the depth of ManyToMany self referencing associations.

@alex88
Copy link

alex88 commented May 16, 2013

👍 on this

@calumbrodie
Copy link

I can see that the multiple exclusion strategy work has been completed, so this functionality could be combined with groups (Is that right?).

On the subject of the MaxDepth being relative to the property or the overall object graph - I do think there needs to be a distinction (and I would say that only the former is really required - I can't think of a use case for the second one that implementing the former wouldn't solve).

@schmittjoh
Copy link
Owner

I tend to agree. @adrienbrault, would just a relative depth strategy work for you as well?

@adrienbrault
Copy link
Contributor Author

@calumbrodie @schmittjoh I do agree :). But it could still be named MaxDepth

@adrienbrault
Copy link
Contributor Author

Code updated so that the @MaxDepth rule works in a relative way.

@schmittjoh
Copy link
Owner

Looks good.

Could you add some documentation for the annotation, and the exclusion strategy? I think we could also add something to the context like enableMaxDepthChecks.

@adrienbrault
Copy link
Contributor Author

@schmittjoh Done!

schmittjoh added a commit that referenced this pull request Jun 1, 2013
@schmittjoh schmittjoh merged commit 5b8b4b4 into schmittjoh:master Jun 1, 2013
@schmittjoh
Copy link
Owner

Great, thanks!

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.

8 participants