Proof of concept #135

bakura10 · 2014-03-08T16:55:49Z

First try for #132 .

This PR does not work, it's just a quick prototype. As said in the issue, this assumes that we have an hydrator that only extract scalar names, and let associations untouched (so extract will actually return associations, BUT as object).

It's then the ResourceRenderer that recursively calls the hydrators.

I suspect this to be quite slow but... it seems the "best" way to do it no?

TODO:

tests
check circular rendering
allow to override association directly in ResourceModel.

coveralls · 2014-03-08T16:58:38Z

Coverage decreased (-6.8%) when pulling 287bbc1 on extraction into 31a6713 on master.

coveralls · 2014-03-09T10:15:42Z

Coverage decreased (-8.33%) when pulling 4ee5dbb on extraction into 31a6713 on master.

danizord · 2014-03-10T12:33:45Z

src/ZfrRest/View/Renderer/DefaultResourceRenderer.php

+
+        // Then, handle each association
+        $classMetadata = $resourceMetadata->getClassMetadata();
+        $associations  = $classMetadata->getAssociationNames();


You can't implement getAssociationsMetadata() in ResourceMetadata?

Nope. I know this is confusing. Actually, in the resource level, associations have mapping like the path, the name of the association. But if you want to retrieve the resource mapping for this association, it's something different (it's another instance of ResourceMetadataInterface, actually).

I'm thinking about renaming "getAssociationMetadata" to "getAssociation", so that it is less confusing between the metadata you get for a given association and the specific metadata bound to an association inside a given resource metadata.

danizord · 2014-03-10T13:22:56Z

Sounds great 👍 We just need to add more options to ResourceModel in order to allow developers to change the renderer behavior.

bakura10 · 2014-03-10T16:38:38Z

What are you thinking about for options? It's still very confusing in my own head, to be honest.

danizord · 2014-03-10T16:59:04Z

@bakura10 See #132 (comment)

…ction

bakura10 · 2014-03-16T11:49:11Z

@danizord , I've made some more work on the renderer. Can you check it? :)

bakura10 · 2014-03-16T13:17:11Z

Note : I moved the JSON serialization from the renderer to the strategy instead. This brings much more flexibility as I can recursively render now.

Ocramius · 2014-03-16T13:22:50Z

src/ZfrRest/Hydrator/ResourceHydrator.php

@@ -16,23 +16,29 @@
 * and is licensed under the MIT license.


Did GIT simply get this one wrong? Or did you explicitly use git mv?

Strange. It was supposed to be removed. I surely messed with it.

Ocramius · 2014-03-16T13:24:14Z

src/ZfrRest/Resource/Metadata/Annotation/Association.php

     * @var string
     */
    public $path;

    /**
+     * @var string
+     *
+     * @Enum({"NONE", "EMBED", "ID"})


Wondering if constants can be used here:

@Enum({Association::NONE, Association::EMBED, Association::ID})

I'd like to! I'll test that once I finish it.

Can be ignored for now

bakura10 · 2014-03-16T15:01:17Z

With this new code, skipping specific field is now super easy. For instance, let's say you don't want to output password from User entity, you simply define a new hydrator, and add a filter:

use Zend\Stdlib\Hydrator\ClassMethods;
use Zend\Stdlib\Hydrator\Filter\MethodMatchFilter;

class UserHydrator extends ClassMethods
{
    public function __construct()
    {
        parent::__construct();
        $this->filterComposite->addFilter("password", new MethodMatchFilter('getPassword'));
    }
}

And boom! No more need to define hydrate, extract...

bakura10 · 2014-03-16T22:16:34Z

If you have some time to review this @Ocramius it would be awesome :D. I'd like to merge this tomorrow or on tuesday, so I can test it in my app and know if it's viable.

bakura10 · 2014-03-17T09:54:02Z

Last update includes an important fix that is due to the way ZF2 hydrate data. Basically, the default ClassMethods output as underscore separated keys. This means that an association like "myAssociation" will be rendered as "my_association" by the hydrator. Therefore, the association name retrieved from Doctrine metadata will be "myAssociation", and checks will fail.

Therefore, I have added a similar parameter to the resource renderer, called underscoreSeparatedKeys (which is set to true to be on pair with default values of ClassMethods). This automatically inflects any association name to udnerscore_separated, so that everything can still work.

danizord · 2014-03-17T13:44:36Z

config/module.config.php

@@ -21,7 +21,7 @@
        'factories' => [
            /* Factories that do not map to a class */
            'ZfrRest\Cache'                                                   => 'ZfrRest\Factory\CacheFactory',
-            'ZfrRest\View\Renderer\ResourceRenderer'                          => 'ZfrRest\Factory\SimpleResourceRendererFactory',
+            'ZfrRest\View\Renderer\ResourceRenderer'                          => 'ZfrRest\Factory\DefaultResourceRendererFactory',


ZfrRest\View\Renderer\DefaultResourceRenderer?

Currently it's configured through the factory. The strategy factory fetches the "ZfrRest\View\Renderer\ResourceRenderer" key, and if you want to switch factory, you need to override this key.

Maybe we could expose an option into ZfrRest config, like "renderer" => "foo". Or even a config like that:

'zfr_rest' => [ 'renderers' => [ 'application/json' => 'ZfrRest\View\Renderer\DefaultRenderer' ] ];

What do you think @Ocramius ?

@bakura10 that can be dealt with later, as an implementation detail of something like a ZfrRest\View\Renderer\RendererStrategy or such

bakura10 · 2014-03-18T16:04:39Z

I'll merge that tomorrow guys. If you want review, either do it today or throw any comments here after it being merged so that I can read the feedback.

Ocramius · 2014-03-18T16:19:52Z

docs/02. Quick Start.md

+ * @var Tweet[]
+ *
+ * @ORM\OneToMany(targetEntity="Tweet", mappedBy="user")
+ * @REST\Association(routable=true, extraction="NONE")


I like the routable=false as current approach - makes it easier for us. Is the default false?

Yes, false is default to avoid any leaking route.

coveralls · 2014-03-19T09:07:59Z

Changes Unknown when pulling 3df1b95 on extraction into * on master*.

bakura10 · 2014-03-20T16:20:27Z

I'm merging for now. I've heard you about segregating the metadata hydrator, but that's something I'm not sure how to implement for now, and that's easy to change without breaking it.

Proof of concept

Proof of concept

287bbc1

Unset associations

4ee5dbb

danizord reviewed Mar 10, 2014
View reviewed changes

bakura10 added 5 commits March 16, 2014 11:49

Merge branch 'master' of https://github.com/zf-fr/zfr-rest into extra…

2481ac6

…ction

Set doc and updated metadata

aa267a4

Remove old renderer

a1380f2

Fix tests

7863fc3

Boo

301cd24

bakura10 added 4 commits March 16, 2014 12:57

Implement embedded

7994613

Allow to render meta

eed81e9

Fix CS

4c57ecd

Move Json serialization to strategy

e66afa4

Ocramius reviewed Mar 16, 2014
View reviewed changes

Remove hydrator

3f374e0

Ocramius reviewed Mar 16, 2014
View reviewed changes

Add extraction constants

c37feaf

bakura10 mentioned this pull request Mar 16, 2014

Default hydrator should skip associations. #96

Closed

Update doc

f43dd53

Allow underscore separated keys

f2ae3de

bakura10 added 3 commits March 17, 2014 11:29

No longer needed (done in the resource factory)

bd072e6

Bring more flexibility to the interface

f941caf

Remove useless import

a067b07

danizord reviewed Mar 17, 2014
View reviewed changes

Update doc

7bce3d0

Ocramius reviewed Mar 18, 2014
View reviewed changes

Feedback

3df1b95

bakura10 added a commit that referenced this pull request Mar 20, 2014

Merge pull request #135 from zf-fr/extraction

d14aef3

Proof of concept

bakura10 merged commit d14aef3 into master Mar 20, 2014

bakura10 deleted the extraction branch March 20, 2014 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proof of concept #135

Proof of concept #135

bakura10 commented Mar 8, 2014

coveralls commented Mar 8, 2014

coveralls commented Mar 9, 2014

danizord Mar 10, 2014

bakura10 Mar 10, 2014

bakura10 Mar 16, 2014

danizord commented Mar 10, 2014

bakura10 commented Mar 10, 2014

danizord commented Mar 10, 2014

bakura10 commented Mar 16, 2014

bakura10 commented Mar 16, 2014

Ocramius Mar 16, 2014

bakura10 Mar 16, 2014

Ocramius Mar 16, 2014

bakura10 Mar 16, 2014

Ocramius Mar 18, 2014

bakura10 commented Mar 16, 2014

bakura10 commented Mar 16, 2014

bakura10 commented Mar 17, 2014

danizord Mar 17, 2014

bakura10 Mar 17, 2014

Ocramius Mar 18, 2014

bakura10 commented Mar 18, 2014

Ocramius Mar 18, 2014

bakura10 Mar 18, 2014

coveralls commented Mar 19, 2014

bakura10 commented Mar 20, 2014

Proof of concept #135

Proof of concept #135

Conversation

bakura10 commented Mar 8, 2014

coveralls commented Mar 8, 2014

coveralls commented Mar 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danizord commented Mar 10, 2014

bakura10 commented Mar 10, 2014

danizord commented Mar 10, 2014

bakura10 commented Mar 16, 2014

bakura10 commented Mar 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakura10 commented Mar 16, 2014

bakura10 commented Mar 16, 2014

bakura10 commented Mar 17, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakura10 commented Mar 18, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 19, 2014

bakura10 commented Mar 20, 2014