Skip to content

Allow abstract resource #311

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

Merged
merged 1 commit into from
Nov 18, 2015
Merged

Conversation

jderusse
Copy link
Contributor

Given the following entities

abstract class Notification {}
class MailNotification extends Notification {}
class PushNotification extends Notification {}

When I configure configure ApiBundle to provider getters on /notifications (the abstract class)
I should retrieves a list of MailNotification and PushNotification but got an exception No resource found for object of type "MailNotification"

This PR allow:

  • Parent's class to be resolved.
  • Child resource to be properly displayed (with right fields and @type)

@theofidry theofidry changed the title Allow abstract resouce Allow abstract resource Oct 14, 2015
@jderusse jderusse force-pushed the allow-abstract-resource branch from 5c87d16 to e6d04dc Compare October 14, 2015 21:43
@ogizanagi
Copy link
Contributor

👍

@dunglas
Copy link
Member

dunglas commented Oct 15, 2015

👍

Can you add some tests?

@jderusse
Copy link
Contributor Author

I'm on it.

I wonder if we should remove POST and PUT routes to the abstract resource (because can't instanciate abstract class when denormalize the input data)
ping @dunglas

@jderusse jderusse force-pushed the allow-abstract-resource branch 2 times, most recently from 0edf6a7 to 5c17ca9 Compare October 16, 2015 07:29
@dunglas
Copy link
Member

dunglas commented Oct 16, 2015

Yes looks good to remove such methods as they can be added manually if necessary.

@jderusse
Copy link
Contributor Author

@dunglas Should be OK now, but travis wont build my PR :(

@dunglas
Copy link
Member

dunglas commented Oct 16, 2015

Maybe because of the conflict. Can you rebase your PR?

@jderusse jderusse force-pushed the allow-abstract-resource branch 2 times, most recently from 9479cfa to d2eb55c Compare October 16, 2015 17:38
@jderusse
Copy link
Contributor Author

Rebased.

With an assert equals of > 1000 lines, the doc.feature is no more a test nor maintenable...
We just are asserting that, the output of the run 2 is identical to the copy/paste from the run 1. It's useless.

Shouldn't we removing it?

@jderusse jderusse force-pushed the allow-abstract-resource branch from c2da7ba to 49b4363 Compare October 16, 2015 17:57
@dunglas
Copy link
Member

dunglas commented Oct 16, 2015

Many tests needs to be refactored. Removing it totally is not an option but IMO we should test only some keys in the doc (the description of some selected endpoints) to ease the refactoring.

@sroze
Copy link
Contributor

sroze commented Oct 23, 2015

@jderusse related to the tests, I think the way it has been done in #242 is good.

@sroze
Copy link
Contributor

sroze commented Oct 23, 2015

LGTM, thank you @jderusse ! 👍

*/
private function createOperations(ContainerBuilder $container, $serviceId, $methods, $collection)
{
return array_map(function ($method) use ($container, $serviceId, $collection) {
Copy link
Member

Choose a reason for hiding this comment

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

Why array_map and not a simple foreach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question if habit, avoid temporary variable and gain 3 lines of code.

I can revert it if you prefer something more explicit

Copy link
Member

Choose a reason for hiding this comment

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

Using methods as key should make the call more explicit and more performant (even if it not so important here because this method should not be called at runtime).

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the array_map version too :trollface:

@dunglas
Copy link
Member

dunglas commented Oct 26, 2015

Tests like in #242 is the way to go!

@dunglas
Copy link
Member

dunglas commented Oct 26, 2015

👍 great.

@dunglas dunglas added this to the 1.1.0 milestone Oct 29, 2015
@csarrazi
Copy link
Contributor

This PR needs yet another rebase! :)

@jderusse jderusse force-pushed the allow-abstract-resource branch from e70f1c6 to 3d92bab Compare November 17, 2015 12:12
@jderusse
Copy link
Contributor Author

I rebase the PR.
Is anything missing? ping @dunglas

@dunglas
Copy link
Member

dunglas commented Nov 17, 2015

No I just need to find the time to merge it in master too. If you're in a hurry can you open the PR against master yourself and I'll merge both.

@jderusse jderusse force-pushed the allow-abstract-resource branch from 3d92bab to 4caaf3e Compare November 17, 2015 14:49
@jderusse
Copy link
Contributor Author

here we go :) #328

dunglas added a commit that referenced this pull request Nov 18, 2015
@dunglas dunglas merged commit b1234a9 into api-platform:1.0 Nov 18, 2015
@dunglas
Copy link
Member

dunglas commented Nov 18, 2015

Thank you @jderusse

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants