Skip to content

WIP - Subresource maxDepth #1512

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

Closed
wants to merge 100 commits into from

Conversation

Nightbr
Copy link
Contributor

@Nightbr Nightbr commented Nov 23, 2017

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

Hey,

I just made my first PR to api-platform!

It adds the possibility to pass a maxDepth parameter to ApiSubresources in order to stop the route generation.

Because it's my first PR on api-platform, I don't know how all this works so I need some help.

I'm not sure about the annotation system, I think I am good with my code but if anyone have some advice it would be appreciated. Add to that, is there a modification for yaml and xml configuration to take into account the new parameter?

Finally, someone can validate my code on SubresourcesOperationFactory. Seems good to me and I created a test and it doesn't break other tests but like I said first PR on Api platform and I think I have a lots of thing to learn on this!

After this, I will update the documentation and create more test case.

Thanks in advance.

@soyuka

dunglas and others added 30 commits September 12, 2017 09:54
Enable FOSUser support if the bundle is installed
Deprecate in the constructor
…formances

Add a partial paginator to prevent SQL COUNT queries
Add payload to ConstraintViolationListNormalizer
2.1.x-dev is already taken by the 2.1 branch, and this should represent
the next minor version anyway.
Parent class filters (needs test)
Support for nested properties
Tests
Fix some comments and remove id=>id in compiler pass
If api-platform uses upstream libs in a deprecated way, we should be aware
of it.
$maxDepth = $subresource->getMaxDepth();
}

if(null !== $maxDepth && count($visited) >= $maxDepth) {
Copy link
Member

Choose a reason for hiding this comment

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

use the $depth counter instead of count or there is no point :p.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes my bad... I commit/push too quickly

@@ -154,7 +165,7 @@ private function computeSubresourceOperations(string $resourceClass, array &$tre

$tree[$operation['route_name']] = $operation;

$this->computeSubresourceOperations($subresourceClass, $tree, $rootResourceClass, $operation, $visited + [$visiting => true]);
$this->computeSubresourceOperations($subresourceClass, $tree, $rootResourceClass, $operation, $visited + [$visiting => true], $counter+1, $maxDepth);
Copy link
Member

Choose a reason for hiding this comment

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

use ++$depth instead of $counter + 1

@soyuka
Copy link
Member

soyuka commented Nov 23, 2017

Try to use git fetch origin && git rebase origin/master instead of merging.

@Nightbr
Copy link
Contributor Author

Nightbr commented Nov 23, 2017

Wow button Update branch in Github create a real mess... Sorry about that...

@soyuka
Copy link
Member

soyuka commented Nov 24, 2017

I can fix you branch if you want ?

@Nightbr
Copy link
Contributor Author

Nightbr commented Nov 24, 2017

Yes if you can, I would really appreciate! Thanks

@soyuka soyuka changed the base branch from master to 2.1 November 24, 2017 10:32
@soyuka
Copy link
Member

soyuka commented Nov 24, 2017

Fixed, please use the following:

git fetch --all
git reset --hard subresource_depth

@Nightbr Nightbr closed this Nov 24, 2017
@Nightbr Nightbr deleted the subresource_depth branch November 24, 2017 10:51
@Nightbr Nightbr mentioned this pull request Nov 24, 2017
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.