Skip to content
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

Simplfy controllers #122

Merged
merged 7 commits into from
Feb 17, 2014
Merged

Simplfy controllers #122

merged 7 commits into from
Feb 17, 2014

Conversation

bakura10
Copy link
Member

ping @Ocramius @danizord @Orkin

This PR tries to remove the overhead that was introduced in controllers with the fact that we forced user returning ResourceModel to trigger the automatic serialization. This is indeed a good idea and I want to keep that, however it forced user to complicate their controller, as shown below:

public function get(User $user, ResourceMetadataInterface $resourceMetadata)
{
    return new ResourceModel(new Resource($user, $resourceMetadata));
}

First, I removed the fact that handlers automatically add the metadata as a second parameter. It's useless 99% of the time, as the resource type usually stay the same, and instead I have added a getter in the controller to retrieve the matchedResource, so the metadata can still be retrieved this way:

$resource = $this->getMatchedResource()->getMetadata();

Now, in controllers, the ResourceModel is created using a utility controller plugin, and the controller action can be rewritten as this:

public function get(User $user)
{
    return $this->resourceModel($user);
}

ResourceModel plugin will fetch the matched resource metadata.

If an action changes the type (for instance a get user from the UserController that would return another type of object), hence needing to change the resource metadata, the resource model supports an optional second parameter which is the ResourceMetadata.

With this PR, we keep the fact that a ResourceModel must be returned, but we keep the simpler usage we had before.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.12%) when pulling aba6c07 on simplfy-controllers into e4ea55d on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.12%) when pulling 9b2fd1c on simplfy-controllers into e4ea55d on master.

@@ -79,6 +78,14 @@ public function onDispatch(MvcEvent $event)
}

/**
* @return ResourceInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

|null

@danizord
Copy link
Contributor

👍

);
}

$resourceMetadata = $resourceMetadata ?: $this->controller->getMatchedResource()->getMetadata();
Copy link
Contributor

Choose a reason for hiding this comment

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

We have no access to MvcEvent in renderer?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I suppose we can, but how could you create a ResourceViewModel without that? Returning a ResourceModel from the controller is mandatory then.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can't create a listener to inject the resource in ResourceModel (remove mandatory constructor argument)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Problem is that: https://github.com/zf-fr/zfr-rest/blob/master/src/ZfrRest/Mvc/Controller/MethodHandler/PostHandler.php#L100

I need AT LEAST a resource as a return value for that kind of automation. But if I create a REsource in the controller then.... I can also create a ResourceModel and I prefer returning a Model instead of an object.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.12%) when pulling ca13fa9 on simplfy-controllers into e4ea55d on master.

@bakura10
Copy link
Member Author

I updated the doc too. Can you have a quick look?

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 36cc1d3 on simplfy-controllers into e4ea55d on master.

bakura10 added a commit that referenced this pull request Feb 17, 2014
@bakura10 bakura10 merged commit 067ec09 into master Feb 17, 2014
@bakura10 bakura10 deleted the simplfy-controllers branch February 17, 2014 17:58
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.

None yet

4 participants