Skip to content

Conversation

@jeffreymb
Copy link
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).

Copy link
Member

@soyuka soyuka left a comment

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
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
Copy link
Member

soyuka commented Apr 9, 2018

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
Contributor Author

I'll work on some tests.

@jeffreymb
Copy link
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
Copy link
Contributor

Simperfit commented Apr 10, 2018

@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
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
Copy link
Member

soyuka commented Apr 10, 2018

@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
Copy link
Member

soyuka commented Apr 10, 2018

@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
Copy link
Contributor

sroze commented Apr 12, 2018

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
Copy link
Member

soyuka commented Apr 12, 2018

test added :)

Copy link
Contributor

@sroze sroze left a comment

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
Copy link
Contributor

sroze commented Apr 12, 2018

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

@jeffreymb
Copy link
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