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

[WIP] ZfrRest NEXT #184

Merged
merged 29 commits into from
Jan 28, 2015
Merged

[WIP] ZfrRest NEXT #184

merged 29 commits into from
Jan 28, 2015

Conversation

bakura10
Copy link
Member

No description provided.

@bakura10
Copy link
Member Author

I've made several progress to ZfrRest today.

I'm pretty happy with the controller part. It is basically only a simple controller, no custom route, no overhead. Plain and simple. You return a ResourceViewModel, with variables and an otpional version that is used to resolve the template.

Contrary to my first proposal, I've decided to remove all the complexity in views. Instead, you expect to return an array that will make the response. That's the easier and allows the most flexibility. Hydrators (for extract) are no longer used, I don't know if that's a good idea or not yet.

I have added two view helpers: one for rendering pagination data, and one that will render another resource by resolving to another template. A typical template therefore look like this:

$data = [];

foreach ($this->users as $user) {
    $data['users'][] = [
       'first_name' => $user->getFirstName(),
       // ...
      'other_resource' => $this->renderResource('user.something', ['foo' => $user->getFoo()])
   ]
}

$data['meta'] = $this->renderPaginator($this->users); // Assuming it is a paginator

return $data;

ping @Orkin , is this what you expected?

@bakura10
Copy link
Member Author

However I don't like that so much formatting happen in the view (like directly setting meta), because you cannot directly include another resource. Maybe I'll add again the idea of "postProcessor", but I need to find a simple way to configuring them.

@Orkin
Copy link
Member

Orkin commented Jan 23, 2015

Yes it's seem good we can easily construct our response. I prefer this implementation than in the template.
I think it will be cool to have a plugin that can render only specified attributes of an object like I told you. Maybe $this->plugin($object, $this->params('fields')); where fields are attributes of the object. Like facebook graph api using fields.

@bakura10
Copy link
Member Author

Hi everyone,

ping @danizord @Ocramius @Orkin

I have faced some few problems, and I've tried to take advantage of what already exists. Let me recap how it works now.

Template resolution

I've give up with the idea of trying to automatically convert template names such as "users/user" to another hierarchy, because the Router does not offer any way to resolve this to a controller (that is needed to know which namespace it belongs to).

Instead, I've taken a simpler and more explicit path. Let's take this simple example, for the route "users/user":

class UserController extends AbstractRestfulController
{
    public function get($params)
    {
        // Get your user from service...

        return new ResourceViewModel(['user' => $user]);
    }
}

By default, Zend will resolve that that "application/user", and therefore ignore the hierarchy between "users" and "user". However, I consider this as a good default, after all.

If you need something, you can always manually use the setTemplate method from the ResourceViewModel, and use the "template_map" to map a name to a given template.

Once ZfrRest has this information, it will do two things before passing the name to the resolver:

  1. It will prepend the template name by the API version, or use the "default" name as a default. For now, it does not extract anything from Request, but that would be easy to add. You can also explicitly pass a version as an option when creating a ResourceViewModel: return new ResourceViewModel(['user' => $user], ['version' => 'v2']).
  2. Finally, it will append the ".php" suffix. I've abandonned the idea of appending the method name, because there is very few interest to have two different representations of the same resources for two different methods, and it would complicate the code.

For instance, if ZfrRest resolves a template to "application/users", the final template ZfrRest will look for will be "default/users.php".

View

The template (or view) are simple PHP files, and expect you to return a simple array. You are completely free on how you want to use the template. Because this is a standard ZF2 view, you have access to all built-in helpers, like identity (useful to add/remove fields based on role, for instance).

The template should be only here for rendering purpose. You should not do complex logic. But logic could include rendering additional resources.

For instance, if your backend understand an "expand" property to automatically expand sub-resources, you can pass an "expand" parameter to your ResourceViewModel, and use that:

if ($this->expand['sub-resource']) {
    $data[] = // render sub resource
}

ZfrRest comes with two new view helpers:

  • RenderPaginator: this one accepts a paginator instance, and returns the limit, offset and count property.
  • RenderResource: render another resource

The RenderPaginator is straightforward, but the RenderResource is more interesting.

It accepts three parameters: a template name, values (that will become the context inside the view) and an optional version.

The template name follow exactly the same rules: we prepend the version and append the .php suffix. If no version is specified, it will use the version of the currently rendered view model.

In other words, if you are rendering a resource "User" on "v1", then render a nested "Tweet" resource and explicitly ask "v2", it will render Tweet using v2. If Tweet itself render sub-resource, then it will use "v2" (not v1), unless explicitly specified.

Differentiating root

I was facing one problem when using this "renderResource" helper. The problem is that, in my template, I usually wrap the data around a key ("user" for instance) and also add the paginator data. However, when doing that:

return [
    'user' => [
        'first_name' => $this->user->getFirstName(),
        'tweets'     => $this->renderResource('tweets', [$tweets => $this->user->getTweets()])
    ]
];

It will lead to wrong results, because tweets will itself be wrap, and also return pagiantor data, which will lead to output like that:

'user' => [
    'first_name' => 'Foo',
    'tweets' => [
        'tweets' => [
            [
                'id' => 5,
                'content' => 'Bar'
            ]
        ],
        'meta' => [
            'limit' => 15,
            'count' => 200
        ]
    ]
]

Fortunately, ZF2 already has a solution with the "ViewModel" helper, and ZfrRest reuse it. This helper allows you to specify the root view model and the current view model (the one currently rendered).

In this case, "user" is the root view model (and also the current view model when rendering the tempalte "user"), but when rendering the tweets, the root view model stay user but the current view model is the Tweet resource model.

In other words, in your template you can add very simple logic to detect if you are in the root, and render additional data:

if ($this->viewModel()->getRoot() === $this->viewModel()->getCurrentModel()) {
    // Wrap the data and output "meta"
}

Maybe we could add a shortcut like "isRootResource".

I'm going to start writing tests, but do not hesitate to comment it, as I'd like to have this as soon as possible.

@bakura10
Copy link
Member Author

All the doc have been written!!

protected function hydrateData($hydratorName, $object, array $values)
{
/** @var \Zend\Stdlib\Hydrator\HydratorInterface $hydrator */
$hydrator = $this->serviceLocator->get('HydratorManager')->get($hydratorName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this stuff to controller plugins?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Thanks, that's much cleaner now.

@danizord
Copy link
Contributor

@bakura10 Did not used this new branch yet, I'll try it next weekend. I'll finish review later today (a bit tired now)

@bakura10
Copy link
Member Author

Thanks @danizord for your review!

I've moved things to controller plugin, that's definitely a lot clearer, thanks a lot. I will not merge now then, and continue using this branch directly in my code.

@danizord
Copy link
Contributor

As a short-feedback: I'm not very very very happy with the view layer. I'll talk more a about it later, but in short, I think that we can use classes instead of php scripts since it's about returning an array. Well, the templates have the benefit of view helpers and such, but they are not testable.

@danizord
Copy link
Contributor

@bakura10 can you wait a bit before merging? I still need to finish review (will do that in 2~3 hours)

EDIT: Ah, nvm, I misread your comment :D

@bakura10
Copy link
Member Author

Sure, I'm going to bed anyway.

@bakura10
Copy link
Member Author

Why aren't they testable? You can test your view helper in isolation, regarding the rendering, those are completely logicless., so nothing prevent you to include the template with the context, and test the returned values.

I actually find it very natural to work with, especially with view helpers. It feels very logical, it is actually rendering ! So template makes perfect sense.

@bakura10 bakura10 mentioned this pull request Jan 28, 2015
@bakura10
Copy link
Member Author

I'm merging @danizord , if you have any other feedback please open an issue or new PR :).

bakura10 added a commit that referenced this pull request Jan 28, 2015
@bakura10 bakura10 merged commit e27d31f into master Jan 28, 2015
@bakura10 bakura10 deleted the refactor branch January 28, 2015 11:29
@grizzm0
Copy link

grizzm0 commented Jan 28, 2015

@bakura10
Just started a new project and was using 0.3.0. Just noticed this merge. Is dev-master useable atm?

@bakura10
Copy link
Member Author

Hi,

Dev master is a completely different version than 0.3, I'd suggest you to read the doc if it fits your project, as it does not have the same philosophy.

Dev master is usable (I've started back porting my code and fixing the code if I have error). I hope to tag a beta of 0.4 next week, so I'd be very happy if you use dev master so that we can debug it faster!

@grizzm0
Copy link

grizzm0 commented Jan 28, 2015

Yeah, I noticed it was completly different. But as I haven't really started on the code yet I tought I might aswell use 0.4.0.

@bakura10
Copy link
Member Author

Just be warn that zfrrest 0.3 was full of automatism while 0.4 is DIY ;)

@grizzm0
Copy link

grizzm0 commented Jan 28, 2015

Awesome. ;) Is there no packagist hook for Zfr-Rest? dev-master on packagist is stuck on 9571d61.

@bakura10
Copy link
Member Author

Should be fixed now!

@bakura10
Copy link
Member Author

@grizzm0 , I've tagged 0.4.0, and I've also added a new section in the documentation that could be helpful: https://github.com/zf-fr/zfr-rest/blob/master/docs/05.%20Best%20practices.md

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

5 participants