Skip to content

[WIP][RFC][Validator] Dynamic Validation Groups #16984

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

Closed
wants to merge 1 commit into from
Closed

[WIP][RFC][Validator] Dynamic Validation Groups #16984

wants to merge 1 commit into from

Conversation

peterrehm
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Since the cascade_validation option has been deprecated for forms it makes sense to provide a generic solution for validation groups based on populated entity data.

This is based on the discussion I had with @stof a while ago and the gist he provided. https://gist.github.com/stof/d21838b00d08858ae6bb

/**
 * @DynamicGroups(groupProvider="dynamicGroups")
 */
class Customer
{
    /**
     * @Assert\NotBlank()
     */
    private $customerType; // Customer or Person

    /**
     * @Assert\NotBlank(groups={"Company"})
     */
    private $companyName;

    /**
     * @Assert\NotBlank(groups={"Person"})
     */
    private $lastName;

    public function getDynamicGroups()
    {
        return [$this->customerType];
    }
}

By default the entity will be validated against the Default constraint (or the constraint specified in the validator) and if the DynamicGroups constraint is part of that validation the validation will be cascaded to the dynamic groups.

The PR is just the RFC to see if that functionality is wanted, then I would complete it with tests and documentation. This gives a generic solution where the validation is configured is only in the entity and the validation_groups option in form types then can and should only be used for partial validation.

@linaori
Copy link
Contributor

linaori commented Dec 12, 2015

I like the idea. I often find myself needing a different validation on 1 or more properties based on another value. There's but one problem in your example, getDynamicGroups is static but relies on $this->customerType.

@peterrehm
Copy link
Contributor Author

The example is fixed, shrunk an existing class which caused the error :)

@peterrehm
Copy link
Contributor Author

@weaverryan or @javiereguiluz Any comment from your side? The would be great for the DX IMHO. I can easily polish this up and get this ready, otherwise I am closing the PR.

@webmozart
Copy link
Contributor

I would like to see this implemented with a @DefaultGroup annotation as suggested here: #11880 (comment)

@peterrehm
Copy link
Contributor Author

@webmozart I am not totally sold on the @DefaultGroup proposal as in one use case I have an entity with partial validation and I do not see this necessary as releated to a DefaultGroup. I might also be confused with the DefaultGroup name.

/**
 * @DynamicGroups(groupProvider="dynamicGroups", groups="case1")
 * @DynamicGroups(groupProvider="otherDynamicGroups", groups="case2")
 */
class Customer
{
    /**
     * @Assert\NotBlank()
     */
    private $customerType; // Customer or Person

    /**
     * @Assert\NotBlank(groups={"Company"})
     */
    private $companyName;

    /**
     * @Assert\NotBlank(groups={"Person"})
     */
    private $lastName;

    public function getDynamicGroups()
    {
        return [$this->customerType];
    }

   //...
}

@peterrehm
Copy link
Contributor Author

@webmozart Any further comment?

@peterrehm
Copy link
Contributor Author

@webmozart How about continuing with this way and deprecating it once there is a better way. I think it would be good to have such functionality in core. If not we should just close the PR.

@peterrehm
Copy link
Contributor Author

Looks like such solution is not wanted. Closing due to lack of feedback.

@peterrehm peterrehm closed this Mar 16, 2016
@webmozart
Copy link
Contributor

@peterrehm To me it sounds like you need a better data model. Having one class that stores all fields and data for different representations of a "thing" smells. Without knowing more details, I would try to extract fields of your Customer in interfaces like CustomerData (implemented by CompanyData and PersonData), PaymentDetails (implemented by CreditCard and BankAccount) etc. Those can contain there specific validation rules, (hopefully) rendering the need for dynamic groups obsolete.

@peterrehm
Copy link
Contributor Author

@webmozart I expect there are common use cases for my sample. Of course you can try to abstract small things like your mentioned CompanyData or PersonData, I think for simple cases it makes things more complex.

@webmozart
Copy link
Contributor

@peterrehm I guess the question is how you define "complex". I think dynamic validation groups are quite complex to understand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants