-
-
Notifications
You must be signed in to change notification settings - Fork 941
Implement ability to define the controller for a subresource #1830
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
Conversation
soyuka
left a comment
There was a problem hiding this 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?
|
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. |
|
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 |
|
I'll work on some tests. |
|
I spent a while trying to write tests, but couldn't figure out how to set subresource metadata with the way the If tests are going to be written it is going to either be someone else or maybe someone can give me some pointers. |
|
@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 ;). |
|
@Simperfit I understand how testing works, the challenge I'm having is with the way the |
|
@Simperfit It's not that easy subresource are configured through metadata by using only the metadata classe:
|
|
@jeffreymb Okay so to do this we need to add a Let's do this on the subresource identified by:
This resource is on
Currently it has I hope this is enough :). Feel free to ping me on slack! |
|
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? |
|
test added :) |
sroze
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks!
|
Thank you @jeffreymb! Well done for your first contribution on ApiPlatform 👏 |
|
Thanks, @soyuka I wasn't going to get around to it very quickly. :-) |
This PR implements the ability to set the
controlleron a subresource. See referenced issue for details (#1828).