Skip to content

Implement ability to define the controller for a subresource#1830

Merged
sroze merged 3 commits into
api-platform:2.2from
jeffreymb:issue_1828
Apr 12, 2018
Merged

Implement ability to define the controller for a subresource#1830
sroze merged 3 commits into
api-platform:2.2from
jeffreymb:issue_1828

Conversation

@jeffreymb

Copy link
Copy Markdown
Contributor
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #1828
License MIT
Doc PR n/a

This PR implements the ability to set the controller on a subresource. See referenced issue for details (#1828).

@soyuka soyuka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

May you add some test?

@jeffreymb

Copy link
Copy Markdown
Contributor Author

I wanted to but was unable to get phpunit to execute the tests without throwing an exception about a missing class that actually existed so I didn't burn more time on it.

@soyuka

soyuka commented Apr 9, 2018

Copy link
Copy Markdown
Member

You can target 2.2 imo this is a bug fix. Also when a class is missing it's often because they're missing so just composer update :).

@jeffreymb jeffreymb changed the base branch from master to 2.2 April 9, 2018 12:41
@jeffreymb

Copy link
Copy Markdown
Contributor Author

I'll work on some tests.

@jeffreymb

Copy link
Copy Markdown
Contributor Author

I spent a while trying to write tests, but couldn't figure out how to set subresource metadata with the way the \ApiPlatform\Core\Tests\Bridge\Symfony\Routing\ApiLoaderTest tests are set up. I did fix a couple things so that tests would pass with the new code, but didn't add any new tests.

If tests are going to be written it is going to either be someone else or maybe someone can give me some pointers.

@Simperfit

Simperfit commented Apr 10, 2018

Copy link
Copy Markdown
Contributor

@jeffreymb You need to assert the new code you have written, firstly by testing that the controller passed is the one used, then by testing that the exception is send.

If you really don't see how I can do it for you ;).

@jeffreymb

Copy link
Copy Markdown
Contributor Author

@Simperfit I understand how testing works, the challenge I'm having is with the way the ApiLoaderTest is set up to configure operations. It appears that sub resource operations are not configurable the way the ApiLoaderTest is currently written.

@soyuka

soyuka commented Apr 10, 2018

Copy link
Copy Markdown
Member

@Simperfit It's not that easy subresource are configured through metadata by using only the metadata classe:

private function getApiLoaderWithResourceMetadata(ResourceMetadata $resourceMetadata, $recursiveSubresource = false): ApiLoader

@soyuka

soyuka commented Apr 10, 2018

Copy link
Copy Markdown
Member

@jeffreymb Okay so to do this we need to add a subresourceOperation on a ApiResource right?

Let's do this on the subresource identified by:

$propertyMetadataFactoryProphecy->create(RelatedDummyEntity::class, 'secondrecursivesubresource')->willReturn(new PropertyMetadata());

This resource is on RelatedDummyEntity::class, we can find it's metadata a few lines above:

$resourceMetadataFactoryProphecy->create(RelatedDummyEntity::class)->willReturn((new ResourceMetadata())->withShortName('related_dummies'));

Currently it has (new ResourceMetadata())->withShortName('related_dummies') but we also want to call withSubresourceOperations([]) (ref).

I hope this is enough :). Feel free to ping me on slack!

@sroze

sroze commented Apr 12, 2018

Copy link
Copy Markdown
Contributor

I share the pain you have @jeffreymb, this part of the code is a bit weird and would benefit a bit of refactoring because testing it is very painful. Do you want me to take over you PR (i.e. writing the tests) or you’re happy to continue digging into it?

@soyuka

soyuka commented Apr 12, 2018

Copy link
Copy Markdown
Member

test added :)

@sroze sroze left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great, thanks!

@sroze sroze merged commit d34fba5 into api-platform:2.2 Apr 12, 2018
@sroze

sroze commented Apr 12, 2018

Copy link
Copy Markdown
Contributor

Thank you @jeffreymb! Well done for your first contribution on ApiPlatform 👏

@jeffreymb

Copy link
Copy Markdown
Contributor Author

Thanks, @soyuka I wasn't going to get around to it very quickly. :-)

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.

4 participants