[2.1] Conditional Validation #1151
Comments
|
I come up with another solution : https://github.com/sonata-project/AdminBundle/commit/4022538f66a7f36e9cb5cd90bcdb5f22c1343221#diff-3 Validation is done at runtime with an external service. |
|
I'm not really convinced by your approach, as you seem to re-invent how validation should be done. Maybe I'm wrong, can you explain it a bit more? |
|
not sure yet about the best approach, but i agree that its an important thing to get worked out. |
|
Not sure if my proposal is good or not, however is perfectly match a use case : https://gist.github.com/1046563. I have many block instances managed by different services, the validation must be done inside a service as each block service might have a different logic. I also want something to quickly code new block services without coding specific validators or create dedicated validation group. btw, this solution rely on the validator components, so it just about adding a bit more flexibility and save time. |
|
I'm really new on Symfony. Even I'm not sure if you're talking about this issue, anyway here is an idea about how class ParentObject
{
/**
* @Assert\Choice(choices={"choice1", "choice2"})
* @Assert\Choice(choices={"choice1"}, groups={"isChoice1"})
* @Assert\Choice(choices={"choice2"}, groups={"isChoice2"})
*/
private $choice;
/**
* @Assert\NotNull(conditions={"isChoice1"})
*/
private $choice1;
/**
* @Assert\NotNull(conditions={"isChoice2"})
*/
private $choice2;
}If a constraint has a condition/s, validation will be re-executed using the group , if the (sub-validation) is ok , Constraint will be applied |
|
Groups are not flexible enough |
|
What about adding a special method on such entities (yet another idea) : <?php
class User implements ValidationGroupsInterface
{
// ...
// Form would call this method, passing it its validation_groups option
public function getValidationGroups(array $groups = array())
{
if ($this->getType() === 'something') {
$groups[] = 'foo';
} else {
$groups[] = 'bar';
}
return $groups;
}
} |
|
The conditional validator must be a service, as it might require some externals services to validate the data. So settings such method in a model/entity is not flexible enough. |
|
But… "Validator" is also a component service and is set directly in the entity , Validation and conditional validation are such a common practice that should have a quick way of develop. I think the last two examples will mitigate the 99% of needs… Following the logic of the validator component . First would be nice to have a simple and quick method like annotations, then if you want to get deep you can use PHP inside your public validation method in your "entity", if this is not flexible enough then you can go directly to the service in your: formtypes, services, models or whatever Anyway I totally agree that should be a service, but these examples show how implement it, not how to develope it |
|
See my PR: #2498 |
Commits ------- d08ec5e Add DelegatingValidator tests e1822e7 Enable dynamic set of validation groups by a callback or Closure Discussion ---------- [Form][Validator] Enable dynamic set of validation groups based on callback Bug fix: no Feature addition: yes Backwards compatibility break: no Symfony2 tests pass: yes Symfony2 tests written for new feature: yes closes tickets: #2498 #1151 This will allow developer to pass a Closure or a callback array as a Validation groups option of a form. Eg: ``` class ClientType extends AbstarctType { // ... public function getDefaultOptions(array $options) { return array( 'validation_groups' => function(FormInterface $form){ // return array of validation groups based on submitted user data (data is after transform) $data = $form->getData(); if($data->getType() == Entity\Client::TYPE_PERSON) return array('Default', 'person'); else return array('Default', 'company'); }, ); } // ... } ``` ``` class ClientType extends AbstarctType { // ... public function getDefaultOptions(array $options) { return array( 'validation_groups' => array( 'Acme\\AcmeBundle\\Entity\\Client', 'determineValidationGroups' ), ); } // ... } ``` This will make developers life easier ! --------------------------------------------------------------------------- by schmittjoh at 2011/10/27 06:39:56 -0700 Does that work if your ClientType were added to another form type? e.g. ```php <?php class MyComplexType extends AbstractType { public function buildForm(FormBuilder $builder, array $options) { $builder->add('client', new ClientType()); } // ... } ``` --------------------------------------------------------------------------- by canni at 2011/10/27 06:44:33 -0700 This is doing nothing more than injecting array of validation groups, should work, but I have not tested this use case. --------------------------------------------------------------------------- by canni at 2011/10/28 01:58:26 -0700 PHPUnit output ``` OK, but incomplete or skipped tests! Tests: 5011, Assertions: 12356, Incomplete: 36, Skipped: 32. ``` --------------------------------------------------------------------------- by canni at 2011/11/02 11:37:47 -0700 Now functionality is complete, test are written, and implementation is clean. :) --------------------------------------------------------------------------- by stloyd at 2011/11/02 11:50:44 -0700 Can tou `squash` your commits ? Thanks. --------------------------------------------------------------------------- by canni at 2011/11/02 11:58:41 -0700 Done --------------------------------------------------------------------------- by fabpot at 2011/11/07 07:51:18 -0800 Can you add some tests for the `DelegatingValidator` class, which is where we can ensure that the new feature actually works as expected? --------------------------------------------------------------------------- by canni at 2011/11/07 13:53:16 -0800 OK, I've written proof-of-concept tests, also I've squashed few commits to make things clear. Personally I think this should go straight into 2.0 series, as it do not beak BC, and a feature is really nice to use. --------------------------------------------------------------------------- by stof at 2011/11/07 14:17:15 -0800 @canni the 2.0 branch is for bug fixes, not for new features. This is the difference between maintenance releases and minor releases.
|
@schmittjoh do dynamic validation groups ( #2498 ) solve yours case? Maybe this issue can be closed? |
|
@canni it does not. the dynamic validation groups are a feature of the Form component. It does not change the fact that the Validator does not support conditional inheritance |
|
@schmittjoh I'm confused. Your case a) is already possible with the GraphWalker, no? You can launch validation of custom fields, fields in different groups or whatever else you like. I agree that the implementation is not straightforward - should we work on improving this? |
|
Does #3114 solve your issues? |
Commits ------- 92f820a Renamed registerConstraints to loadDynamicValidatorMetadata dd12ff8 CS fix, getConstraints renamed 09c1911 [Validator] Improved dynamic constraints 54cb6e4 [Validator] Added dynamic constraints Discussion ---------- [Validator] Dynamic constraints Bug fix: no Feature addition: yes Backwards compatibility break: no Symfony2 tests pass: yes By now the Validator component is based on a per-class configuration of constraints, but in some cases it might be neccessary to add new constraints dynamically at runtime. This pull request adds a "ConstraintProviderInterface" to the Validator component. If an object is validated that implements this interface the method "getConstraints" is used to add dynamic constraints: class User implements ConstraintProviderInterface { protected $isPremium; protected $paymentInformation; public function getConstraints(ClassMetadata $metadata) { if ($this->isPremium) { $metadata->addPropertyConstraint('paymentInformation', new NotBlank()); } } } --------------------------------------------------------------------------- by alexandresalome at 2012-01-15T11:20:04Z Related to #1151 --------------------------------------------------------------------------- by canni at 2012-01-16T09:22:28Z :+1: --------------------------------------------------------------------------- by bschussek at 2012-01-16T12:32:44Z I think this is a good addition. I think we still have a naming problem though. When constraints are loaded using a static method, the default name for the loader method is `loadValidatorMetadata`. Since the method for dynamic constraint loading is basically the same, I think the two names should be related. Solution (1): Rename the method in your interface to `loadDynamicValidatorMetadata`. Ugly and long. class MyClass implements ConstraintProviderInterface { public static loadValidatorMetadata(ClassMetadata $metadata) ... public loadDynamicValidatorMetadata(ClassMetadata $metadata) ... } Solution (2): Rename the default method name in `StaticMethodLoader` to `registerConstraints` and adjust the docs. Breaks BC. class MyClass implements ConstraintProviderInterface { public static registerConstraints(ClassMetadata $metadata) ... public registerDynamicConstraints(ClassMetadata $metadata) ... } @fabpot: Are we allowed to break BC here? If not, we should probably stick to (1). --------------------------------------------------------------------------- by fabpot at 2012-01-16T12:36:14Z I would prefer to not break BC if possible. --------------------------------------------------------------------------- by blogsh at 2012-01-16T15:25:46Z So "loadDynamicValidatorMetadata" would be the best solution? --------------------------------------------------------------------------- by althaus at 2012-01-17T13:39:19Z >So "loadDynamicValidatorMetadata" would be the best solution? Sounds fine for me based on @bschussek's comment.
|
Solved by #3199 |
Just making sure, this does not get lost, see also http://groups.google.com/group/symfony-devs/msg/a73a70c4b696b8d9
There are two solutions I can imagine:
a) Having an re-entry point for validation that can be used from a class validator
b) Having a way to influence the graph walker as to which members should be visited
@bschussek, what do you think?
The text was updated successfully, but these errors were encountered: