Skip to content

fix issue #1717 - Support Subresources MaxDepth for yaml and xml #2019

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

Merged

Conversation

Nightbr
Copy link
Contributor

@Nightbr Nightbr commented Jun 13, 2018

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #1717
License MIT
Doc PR -

I found it needed more integration in Extractor and Property Factory in order to use Subresources MaxDepth.

I have update related test cases.

Question about Type class which is used in ExtractorPropertyMetadataFactory:
$type = $propertyMetadata->getType();

It uses the built-in symfony Type class and we can't retrieve maxDepth info. So I set maxDepth to null in this case. If you have better idea.

@Nightbr
Copy link
Contributor Author

Nightbr commented Jun 13, 2018

The code coverage seems to have an issue: https://codecov.io/gh/api-platform/core/pull/2019/diff

It seems it doesn't take into account case 'xxx' in the coverage... (in red). But all cases are covered because the code below each case is green... weird one...

@soyuka
Copy link
Member

soyuka commented Jun 13, 2018

Thanks for the bug fix, can you target the 2.2 branch?

@Nightbr Nightbr changed the base branch from master to 2.2 June 13, 2018 14:30
@antograssiot
Copy link
Contributor

The diff is weird, you need to rebase on 2.2 I think

@Nightbr
Copy link
Contributor Author

Nightbr commented Jun 13, 2018

It's when I change target from master to 2.2

If I rebase it messed up everything....

EDIT: I finally achieved to rebase properly but I lost my other PR in the procedure... I will recreate it quickly when it is fresh in my head...

@Nightbr Nightbr force-pushed the fix-subresourceMaxDepth-xml-yaml branch from 7e66193 to 8760548 Compare June 13, 2018 17:17
@Nightbr Nightbr force-pushed the fix-subresourceMaxDepth-xml-yaml branch from 8760548 to c7c4f3b Compare June 13, 2018 17:18
@@ -146,13 +146,15 @@ private function createSubresourceMetadata($subresource, PropertyMetadata $prope
if (null !== $type) {
$isCollection = $type->isCollection();
$resourceClass = $isCollection ? $type->getCollectionValueType()->getClassName() : $type->getClassName();
$maxDepth = null; // for Type we can't configure maxDepth, maxDepth is always null
Copy link
Member

Choose a reason for hiding this comment

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

can you declare this var l145 instead?

we don't need the comment, null is default anyway

@soyuka soyuka merged commit 9515552 into api-platform:2.2 Jun 14, 2018
@soyuka
Copy link
Member

soyuka commented Jun 14, 2018

Thanks @Nightbr !

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.

3 participants