Skip to content

Subresource maxDepth #1520

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 28, 2017
Merged

Subresource maxDepth #1520

merged 1 commit into from
Nov 28, 2017

Conversation

Nightbr
Copy link
Contributor

@Nightbr Nightbr commented Nov 24, 2017 •
edited
Loading

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #1316
License MIT
Doc PR api-platform/docs#351

Proper PR from original: #1512

@Nightbr
Copy link
Contributor Author

Nightbr commented Nov 24, 2017

Hey @soyuka,

I tried to create a clean PR and rebase with git fetch origin && git rebase origin/master but event after I resolve all conflict it keeps create all this mess... I don't understand why this is happening :/

@Nightbr Nightbr mentioned this pull request Nov 24, 2017
Merged
@soyuka soyuka changed the base branch from master to 2.1 November 24, 2017 14:29
@soyuka soyuka changed the base branch from 2.1 to master November 24, 2017 14:47
@soyuka
Copy link
Member

soyuka commented Nov 24, 2017 •
edited
Loading

Arrgh I fixed it, why did you push again? ^^

To take the correct version don't use git pull!

git fetch origin
git reset --hard origin/subresource_depth

(assuming your remote is named origin).

@soyuka soyuka force-pushed the subresource_depth branch 2 times, most recently from 4fe81e4 to 21229f8 Compare November 24, 2017 15:21
@Nightbr
Copy link
Contributor Author

Nightbr commented Nov 24, 2017 •
edited
Loading

humm, I think it's Gitkraken... It makes automatic git pull when I load the project... I'm sorry

@Nightbr
Copy link
Contributor Author

Nightbr commented Nov 24, 2017

I think its ok now 😅

@soyuka
Copy link
Member

soyuka commented Nov 24, 2017

@Nightbr no problem haha, FYI git pull does git fetch + git merge which can be an unwanted thing.

meyerbaptiste
meyerbaptiste reviewed Nov 24, 2017
View reviewed changes
src/Metadata/Property/Factory/AnnotationSubresourceMetadataFactory.php Outdated
@@ -82,7 +82,8 @@ private function updateMetadata(ApiSubresource $annotation, PropertyMetadata $pr
$type = $propertyMetadata->getType();
$isCollection = $type->isCollection();
$resourceClass = $isCollection ? $type->getCollectionValueType()->getClassName() : $type->getClassName();
$maxDepth = $annotation->maxDepth ? $annotation->maxDepth : null;
Copy link
Member

@meyerbaptiste meyerbaptiste Nov 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Do we really want to turn 0 into null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

depth 0 has no sens I think. If it's 0 or null, we do not apply maxDepth. If maxDepth=1 we create one subresource operation, if maxDepth=2 we create two subresources depth, ...

Copy link
Member

Choose a reason for hiding this comment

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

null is correct here yes

Copy link
Member

@meyerbaptiste meyerbaptiste Nov 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Right, the behavior should be the same but IMO we should not transform the configured value: 0 is not null (it's important in terms of extension point). What do you think about keeping 0? The behavior will remain the same with your condition in SubresourceOperationFactory::computeSubresourceOperations().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! fixed

meyerbaptiste
meyerbaptiste requested changes Nov 24, 2017
View reviewed changes
src/Operation/Factory/SubresourceOperationFactory.php Outdated
}

if (null !== $maxDepth && $depth >= $maxDepth) {
continue;
Copy link
Member

Choose a reason for hiding this comment

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

Should not it be break here?

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done! ty

dunglas
dunglas requested changes Nov 27, 2017
View reviewed changes
Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

Nice improvement! Just a minor change to do in a docblock, then LGTM.

src/Metadata/Property/SubresourceMetadata.php Outdated
@@ -54,4 +56,12 @@ public function withCollection(bool $collection): self

return $metadata;
}

/**
* @return int
Copy link
Member

Choose a reason for hiding this comment

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

@return int|null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

sroze
sroze reviewed Nov 27, 2017
View reviewed changes
src/Metadata/Property/Factory/AnnotationSubresourceMetadataFactory.php
@@ -82,7 +82,8 @@ private function updateMetadata(ApiSubresource $annotation, PropertyMetadata $pr
$type = $propertyMetadata->getType();
$isCollection = $type->isCollection();
$resourceClass = $isCollection ? $type->getCollectionValueType()->getClassName() : $type->getClassName();
$maxDepth = $annotation->maxDepth;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we check that maxDepth is int here?

Copy link
Member

@dunglas dunglas Nov 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Doctrine Annotation will handle that because of the PHPDoc IIRC.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I didn't know that. Nice.

@soyuka soyuka merged commit a5dc795 into api-platform:master Nov 28, 2017
@soyuka
Copy link
Member

soyuka commented Nov 28, 2017

Thanks @Nightbr !

@Nightbr
Copy link
Contributor Author

Nightbr commented Nov 28, 2017

Thanks for your help!

hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018
@soyuka
Merge pull request api-platform#1520 from MoltenCoreIO/subresource_depth
8d80f38 
Subresource maxDepth
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sroze sroze sroze left review comments

@soyuka soyuka soyuka approved these changes

@meyerbaptiste meyerbaptiste meyerbaptiste requested changes

@dunglas dunglas dunglas approved these changes

Assignees
No one assigned
Labels
enhancement [RFR] Ready for Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Nightbr @soyuka @dunglas @sroze @meyerbaptiste