Allow to specify validation group #140

bakura10 · 2014-04-02T08:15:08Z

Hi,

I run into a problem this morning, because of lack of validation group. With this PR, your controllers now can implement the new ValidationGroupProviderInterface (ideally, input filters should support named validation groups out of the box, this is something I'll think about):

class PostController extends AbstractRestfulController implements ValidationGroupProviderInterface
{
    public function getValidationGroups()
    {
        return [
            'post' => ['username', 'password'],
            'put' => ['username']
        ]
    }
}

The data validation trait will automatically detect if the controller implement this interface, and get the appropriate input filter.

Thoughts? ping @danizord

coveralls · 2014-04-02T08:18:15Z

Coverage decreased (-0.6%) when pulling eb6f090 on validation-group into ce3346e on master.

coveralls · 2014-04-02T08:26:16Z

Coverage increased (+0.13%) when pulling aa7f41b on validation-group into ce3346e on master.

bakura10 · 2014-04-02T08:44:40Z

What would be ideal would be to being able to do something like this directly in the input filter:

class PostInputFilter
{
    public function __construct()
    {
        // ...
        $this->addValidationGroup('update', ['foo', bar']);
    }
}

Then the controller specification would simply be "put => 'update'". But this not possible so we need to stick with this for now :(.

coveralls · 2014-04-02T08:44:44Z

Coverage decreased (-0.73%) when pulling 3bfdfa6 on validation-group into ce3346e on master.

coveralls · 2014-04-02T08:48:46Z

Coverage increased (+0.13%) when pulling 3bfdfa6 on validation-group into ce3346e on master.

Ocramius · 2014-04-02T09:55:05Z

http://youtube.com/watch?v=YWf5BLUOhNM

bakura10 · 2014-04-02T10:17:07Z

WTH? You don't like?

Ocramius · 2014-04-02T10:19:31Z

@bakura10 no, this seems limiting to me, as well as very shady/hidden.

What happens if I want some kind of filtering on one http method, and a different filtering on a different one (PUT vs POST, for example)? Shouldn't the input filter be directly manipulated instead of pulling something out of the controller?

bakura10 · 2014-04-02T10:23:05Z

Well, we HAVE to use validation group (this is made for that). The issue is that we cannot move this logic into the input filter because it has no idea of the context. So the only place where we actually know the context is the handlers and the controllers.

Regarding your thing, the name of the method (post, put...) is used as name. So:

class PostController extends AbstractRestfulController implements ValidationGroupProviderInterface
{
    public function getValidationGroups()
    {
        return [
            'post' => ['username', 'password'],
            'put' => ['username']
        ]
    }
}

The PostHandler automatically set the validation group name to "post", while PutHandler to "put". If you have a custom handler you can pass "myMethod", and having a validation group "myMethod" in your specification. This is quite flexible because, as you are in the controller, you can also have access to the authorization service, to change the validation group according to the roles the user has;

If you have any idea to make this clearer/less magic, don't hesitate. But we don't have A LOT of choices, actually.

Ocramius · 2014-04-02T10:27:46Z

Well, we HAVE to use validation group (this is made for that). The issue is that we cannot move this logic into the input filter because it has no idea of the context. So the only place where we actually know the context is the handlers and the controllers.

The idea is that wherever the input filter is built, we then set the validation group on it. Ideally in a factory here, or otherwise in the controller action itself (not sure if we handle validation before the action tho)

bakura10 · 2014-04-02T10:34:01Z

The idea is that wherever the input filter is built, we then set the validation group on it.

That's what we do.

otherwise in the controller action itself (not sure if we handle validation before the action tho)

Not possible, because if you do that, you cannot have the awesome feature of having the complete validated and hydrated entity in the action. So this has to happen before :'(.

regarding factory, of course it would be nice BUT I'm against forcing people to write even more factories for setting this, especially as this is pretty common to need validation groups. Input filters can, most of the time, be written without having to alter the input filter manager (thanks to the autoAddInvokable), and I'd like to keep this feature to avoid a factory explosion.

Maybe @danizord has a good idea =).

danizord · 2014-04-02T13:23:17Z

We can't set it in metadata?

@REST\Resource(
    validationGroups = {
        @REST\ValidationGroup(name = "post", fields = {"name", "age", "gender"}),
        @REST\ValidationGroup(name = "put", fields = {"name", "age"}),
    }
)

bakura10 · 2014-04-02T13:25:29Z

You loose a lot of flexibility by doing this. For instance what if I want only admin to edit the "role" property? You can't do that in annotations, while ou can do that dynamically in the getValidationGroupSpecification by getting any authorization service, for instance.

Ocramius · 2014-04-02T13:27:23Z

@bakura10 I don't see much of a difference between stuffing all the metadata in metadata or in a controller. An admin controller could still do whatever it wants by just ignoring that mapping in particular.

bakura10 · 2014-04-02T13:28:50Z

Well, the validation group task is to actually "strip" any unwanted data from the incoming data. In ZfrRest all the data does input filter => hydrator.

Ocramius · 2014-04-02T13:29:43Z

@bakura10 yes, that with the default controller though. We had a setting to disable automatic validation/hydration iirc

bakura10 · 2014-04-02T13:30:30Z

Yep. You had a setting to disable auto hydration and autovalidation. But once they disable those sane settings, they are up to instantiating the input filter themselves, and doing what they want in the controller. It's not much our problem then.

Ocramius · 2014-04-02T13:35:35Z

I think what you are trying to do is new CustomInputFilter($authService)

Ocramius · 2014-04-02T13:39:24Z

Note that filtering and authorization rules quickly escalate in much worse/complex things, that are mostly impossible to map via static config:

So I don't think that trying to solve the problem in a controller's method will do the trick. Instead, I'd suggest having a dependency deal with it, and in this case it is tied to the filter itself.

Ocramius · 2014-04-02T13:55:59Z

src/ZfrRest/Mvc/Controller/AbstractRestfulController.php

+     * @param  InputFilterInterface $inputFilter
+     * @return InputFilterInterface
+     */
+    public function configureInputFilter(InputFilterInterface $inputFilter)


protected

How is the handler supposed to call it then? :)

Interface plz! 😃

@bakura10 isn't the handler trait applied to the controller itself?

No. a handler is a different object than the controller. The traits are applied to the handler.

Not sure an interface is needed here @danizord .

@bakura10 I see... public is fine then. And yes, @danizord is correct about the interface, but that's overkill for now since we assume a ZfrRest abstract controller anyway.

bakura10 · 2014-04-02T14:14:41Z

ping @Ocramius , @danizord , can you do a last read as well as doc please?

Ocramius · 2014-04-02T14:17:54Z

tests/ZfrRestTest/Asset/Mvc/DataValidationObject.php

@@ -25,12 +25,13 @@ class DataValidationObject
 {
    use DataValidationTrait;

+    protected $controller;


coveralls · 2014-04-02T14:22:41Z

Coverage decreased (-0.08%) when pulling 7a1540e on validation-group into ce3346e on master.

danizord · 2014-04-02T14:22:44Z

src/ZfrRest/Mvc/Controller/MethodHandler/DataValidationTrait.php

@@ -19,7 +19,10 @@
 namespace ZfrRest\Mvc\Controller\MethodHandler;

 use Zend\InputFilter\InputFilterPluginManager;
+use Zend\Mvc\Controller\AbstractController;


Ocramius · 2014-04-02T14:24:22Z

src/ZfrRest/Mvc/Controller/AbstractRestfulController.php

@@ -133,6 +134,17 @@ public function getMethodHandlerManager()
    }

    /**
+     * Hook to configure an input filter depending on the method


Should mention that the passed in input filter is the one produced/fetched by zfrrest

coveralls · 2014-04-02T14:31:58Z

Coverage decreased (-0.1%) when pulling 275e736 on validation-group into ce3346e on master.

coveralls · 2014-04-02T14:41:31Z

Coverage remained the same when pulling 275e736 on validation-group into ce3346e on master.

Allow to specify validation group

danizord · 2014-04-02T14:54:21Z

docs/04. Controllers.md

+```php
+use Zend\InputFilter\InputFilterPluginManager;
+
+class UserController


extends AbstractRestfulController

Thanks, I'll update this.

bakura10 · 2014-04-07T12:33:07Z

After a bit of usage, this feature is really really nice. Most of the time, it's sufficient to only play with validation group, so this thing is really a big win.

However, at usage, I don't like the getInputFilter logic. Having this in the controller not only require the user to manually call the parent getInputFilter to create the input filter, but it also complicate really much a feature that, I suppose, will be often used.

I really preferred the configureInputFilter where you received an already constructed input filter from the validation trait.

The reasons for having getInputFilter were:

Performance wise, you may throw an input filter to create another one: I'd say this is not the problem of ZfrRest. If we have a faster Input filter (like the one coming in ZF3), it's solved. Furthermore, if you NEED to create a new input filter for a specific action, you could proxy the base input filter using OcraProxyManager, and access manually the input filter manager from the controller service locator.

I'd say that for that kind of thing, usage must have priority over an edge case. And actually, this:

public function configureInputFilter(InputFilterInterface $inputFilter)
{
    // Configure it
}

is a more nicer than:

public function getInputFilter(InputFilterPluginManager $mnager, $inputFillterName)
{
    $inputFilter = parent::getInputFilter($maanger, $inputFilterName);
    // Configure it
}

ping @Ocramius @danizord

Ocramius · 2014-04-07T12:43:28Z

tl;dr: performance is NOT a problem (here).

However, at usage, I don't like the getInputFilter logic. Having this in the controller not only require the user to manually call the parent getInputFilter to create the input filter, but it also complicate really much a feature that, I suppose, will be often used.

If you override a method, you know you need to respect the LSP - nothing wrong with this.

I really preferred the configureInputFilter where you received an already constructed input filter from the validation trait.

It was much less flexible tbh. What would you have done if the validation logic for persistence and update differed by more than setting validation groups? For example, updating an email address of a user (assuming it's the identity) requires different validation while persisting or updating.

Performance wise, you may throw an input filter to create another one: I'd say this is not the problem of ZfrRest. If we have a faster Input filter (like the one coming in ZF3), it's solved. Furthermore, if you NEED to create a new input filter for a specific action, you could proxy the base input filter using OcraProxyManager, and access manually the input filter manager from the controller service locator.

I don't see the performance concern - I approved @danizord's approach for the flexibility - I couldn't care less for the small performance footprint.

public function getInputFilter(InputFilterPluginManager $manager, $inputFillterName)
{
    $inputFilter = parent::getInputFilter($manager, $inputFilterName);
    // Configure it
}

This code block is simple and less magic, and it is really using simple principles. And most importantly, it allows me to build other input filters on demand.

bakura10 · 2014-04-07T12:46:59Z

Well, you still can with the configure approach:

public function configureInputFilter(InputFilterInterface $inputFilter)
{
    // Wow, I've reached an edge case, I don't like this one. Let's create another one:
    $inputFilterManager = $this->serviceLocator->get(InputFilterPluginManager::class);
    $input = $inputFilterManager->get('foo');

    return $input;
}

What I don't like with this approach is that it introduces some incoherencies in the API. The HydrationTrait that is used in handlers actually create himself the hydrator, but for input filter, it's actually a constructor that construct it. I preferred to keep the construction in the handle, and let the controller just configuring it.

Ocramius · 2014-04-07T12:56:30Z

Yes, but then you have an unused parameter, and that's indeed a code smell indicating some sort of waste.

The smell indicates indeed a too specific implementation that cuts out customization

Allow to specify validation group

eb6f090

Add test

aa7f41b

bakura10 added 2 commits April 2, 2014 10:40

Transfer controller

769a163

Fix test

3bfdfa6

Allow to have a hook through controller method

a513776

Ocramius reviewed Apr 2, 2014
View reviewed changes

Add doc and update tests

2bbaace

Ocramius reviewed Apr 2, 2014
View reviewed changes

Remove unused variable

7a1540e

danizord reviewed Apr 2, 2014
View reviewed changes

Remove unused imports

e9a6ba2

Ocramius reviewed Apr 2, 2014
View reviewed changes

Feedback

275e736

bakura10 added 2 commits April 2, 2014 16:43

Use controller to create input filter

71717d2

Update tests and docs

687fbe6

Ocramius added the enhancement label Apr 2, 2014

Ocramius self-assigned this Apr 2, 2014

bakura10 added a commit that referenced this pull request Apr 2, 2014

Merge pull request #140 from zf-fr/validation-group

45ece77

Allow to specify validation group

bakura10 merged commit 45ece77 into master Apr 2, 2014

bakura10 deleted the validation-group branch April 2, 2014 14:53

Ocramius added this to the 0.2.0 milestone Apr 2, 2014

danizord reviewed Apr 2, 2014
View reviewed changes

Allow to specify validation group #140

Allow to specify validation group #140

Conversation

bakura10 commented Apr 2, 2014

coveralls commented Apr 2, 2014

coveralls commented Apr 2, 2014

bakura10 commented Apr 2, 2014

coveralls commented Apr 2, 2014

coveralls commented Apr 2, 2014

Ocramius commented Apr 2, 2014

bakura10 commented Apr 2, 2014

Ocramius commented Apr 2, 2014

bakura10 commented Apr 2, 2014

Ocramius commented Apr 2, 2014

bakura10 commented Apr 2, 2014

danizord commented Apr 2, 2014

bakura10 commented Apr 2, 2014

Ocramius commented Apr 2, 2014

bakura10 commented Apr 2, 2014

Ocramius commented Apr 2, 2014

bakura10 commented Apr 2, 2014

Ocramius commented Apr 2, 2014

Ocramius commented Apr 2, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakura10 commented Apr 2, 2014

Choose a reason for hiding this comment

coveralls commented Apr 2, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 2, 2014

coveralls commented Apr 2, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakura10 commented Apr 7, 2014

Ocramius commented Apr 7, 2014

bakura10 commented Apr 7, 2014

Ocramius commented Apr 7, 2014