[Validation] Activate auto-mapped validation via an annotation #32070

weaverryan · 2019-06-17T14:00:45Z

Description
The new auto-mapping validation feature is activated via configuration. I think this is too obtuse, and people are wondering "Why is this field required? Where did that validation come from"?

The problem is that ALL your validation currently lives as annotations (or XML, YAML)... except that now this automatic system is activated in an entirely different location. Additionally, the auto-mapping (as nice at it is) is imperfect - and always will be - automatic things will never always get it right. For example, make:registration is broken with auto-mapping as the feature adds NotNull to the $password field, which will eventually hold the encoded password. This field should not have validation - symfony/maker-bundle#412

Additionally, an annotation would allow you to specify the validation group.

Example

/**
 * @Entity()
 * @Assert\AddAutomaticConstraints()
 */
class User
{
}

And ideally you could disable it automatic constraints on a property-by-property basis:

/**
 * @Entity()
 * @AddAutomaticConstraints()
 */
class User
{
    // ...

    /**
     * @NoAutomaticConstraints()
     */
    private $password;
}

This makes the feature a bit less useful, I admit. But my initial instinct is that this is causing a bit more pain than convenience.

Btw, an added problem is that the presence/absence of symfony/property-info makes this feature behave differently - and that's very not obvious.

The text was updated successfully, but these errors were encountered:

fabpot · 2019-06-17T16:45:37Z

/cc @dunglas

dunglas · 2019-06-17T22:00:43Z

I didn't anticipated this case, and it's indeed a problem. However, it's a very specific one, that should be the exception, not the standard. Having to manually enabling auto-validation kind of defects the point of the feature: catching missing validation constraints, and providing a basic security by default.

I do like the proposed annotations, but as this feature already has to be enabled explicitly in the configuration, I would prefer to have auto-validation enabled by default, and annotations allowing to opt-out:

// Automatic validation is enabled, as currently
class Foo {
    private $password;
}

// Automatic validation is disabled for the given property only
class Foo {
    /** @DisableAutomaticValidation */
    private $password;
}

// Automatic validation is disabled for the whole class
/** @DisableAutomaticValidation */
class Foo {
    private $password;
    private $foo;
}

It's a better security practice to let the user enabling the potentially insecure feature, than having to optin for the secure behavior. WDYT?

Such annotations will also fix #32015.

weaverryan · 2019-06-18T13:30:07Z

Hmm, yes, I think we can try this :). 2 more questions:

would this be for 4.3? It strikes me as a new feature. However, without doing something on 4.3, the auto-validation is almost unusable. What I mean is, it's very difficult to work-around this edge case - the only reasonable thing for a user to do would be to disable auto-validation entirely (right?). Can we put this in 4.3? And if not, what can we do? Disable the feature in the recipe?
I'm still also concerned about the optional property-info component. We activate auto-validation by default... but users don't have property-info by default (the serializer-pack is probably the way most people get it, which is totally unrelated to validation). This means we're giving them "half" auto-validation. Even the DoctrineLoader (enabled even without property-info) "defers" nullable & type checks to the PropertyInfoLoader, which isn't present by default. What can we do here? Add it to symfony/skeleton? Make a validator-pack and give it the validator alias? Throw a warning somewhere, somehow?

Cheers!

weaverryan · 2019-06-18T17:45:36Z

FYI we’ve disabled auto-mapping in the recipe for now.

dunglas · 2019-06-18T17:57:38Z

I’ll work on patch tonight. I also agree it qualifies as a bug fix.
Regarding PropertyInfo, a quick fix is to add it as a dependency of the validator-pack.

weaverryan · 2019-06-19T13:53:02Z

Regarding PropertyInfo, a quick fix is to add it as a dependency of the validator-pack

We would need to create that pack too :). But, looking into it, I don't think that any of the Symfony components directly require symfony/validator. This means that, at some point, to get validation, the user needs to run composer require ... in order to get the validator. This is a good things: it means that a pack will work: we can direct the user to run composer require validato to get the pack (without worrying about only symfony/validator being installed because some other dependency requires it directly).

In MakerBundle, we *could( also (in any commands that require the validator) make sure that PropertyInfo is installed.

Regarding PropertyInfo, a quick fix is to add it as a dependency of the validator-pack

Also... any ideas for a non-quick fix? :) Or is the pack the only thing we can think of?

dunglas · 2019-06-19T20:44:25Z

Also... any ideas for a non-quick fix? :) Or is the pack the only thing we can think of?

Another alternative is to make property-info a hard dependency of validator. It's maybe not so stupid after all.

develth · 2019-07-04T10:46:24Z

I recognized an additional case / issue with the Doctrine Extension "Timestampable".
It complains that createdAt (on="create")and updatedAt (on="update") are null.

stepanselyuk · 2019-10-18T14:36:10Z

Any way to fix validation for Doctrine Extension "Timestampable"?

@Orm

…sable auto-validation (dunglas) This PR was squashed before being merged into the 4.4 branch (closes #32107). Discussion ---------- [Validator] Add AutoMapping constraint to enable or disable auto-validation | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | #32070, #32015  | License | MIT | Doc PR | todo As discussed in #32070 and #32015, it's sometimes mandatory to prevent some classes or properties to be auto mapped (auto-validated). This PR introduces a new constraint, `@AutoMapping` allowing to do exactly that. Examples: Class: ```php use Doctrine\ORM\Mapping as ORM; use Symfony\Component\Validator\Constraints as Assert; /** * @Orm\Entity * @Assert\AutoMapping(false) */ class DoctrineLoaderNoAutoMappingEntity { /** * @Orm\Id * @Orm\Column */ public $id; /** * @Orm\Column(length=20, unique=true) */ public $maxLength; } ``` Property: ```php namespace Symfony\Bridge\Doctrine\Tests\Fixtures; use Doctrine\ORM\Mapping as ORM; use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity; use Symfony\Component\Validator\Constraints as Assert; /** * @Orm\Entity */ class DoctrineLoaderEntity extends DoctrineLoaderParentEntity { /** * @Orm\Id * @Orm\Column */ public $id; /** * @Orm\Column(length=10) * @Assert\AutoMapping(false) */ public $noAutoMapping; } ``` The rules are the following: * If the constraint is present on a property, and set to true, auto-mapping is always on, regardless of the config, and of any class level annotation * If the constraint is present on a property, and set to false, auto-mapping is always off, regardless of the config, and of any class level annotation * If the constraint is present on a class, and set to true, auto-mapping is always on except if a the annotation has been added to a specific property, and regardless of the config * If the constraint is present on a class, and set to false, auto-mapping is always off except if a the annotation has been added to a specific property, and regardless of the config Commits ------- f6519ce [Validator] Add AutoMapping constraint to enable or disable auto-validation

weaverryan added the Validator label Jun 17, 2019

xabbuh added the Feature label Jun 17, 2019

fabpot mentioned this issue Jun 18, 2019

Disable auto-mapping for now symfony/recipes#612

Merged

dunglas mentioned this issue Jun 19, 2019

[Validator] Add AutoMapping constraint to enable or disable auto-validation #32107

Merged

phpdev mentioned this issue Jun 25, 2019

[Validation] Automatic validation for boolean columns #32168

Closed

javiereguiluz mentioned this issue Jul 4, 2019

Following doctrine/registration_form.html Fails symfony/symfony-docs#11885

Closed

soyuka mentioned this issue Aug 2, 2019

Automatic validation may cause issues with fields that are generated api-platform/api-platform#1211

Open

xabbuh mentioned this issue Oct 17, 2019

auto validation always fail for not null field, filled in doctrine events #34009

Closed

nicolas-grekas closed this as completed Oct 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Validation] Activate auto-mapped validation via an annotation #32070

[Validation] Activate auto-mapped validation via an annotation #32070

weaverryan commented Jun 17, 2019

fabpot commented Jun 17, 2019

dunglas commented Jun 17, 2019

weaverryan commented Jun 18, 2019

weaverryan commented Jun 18, 2019

dunglas commented Jun 18, 2019 •

edited

Loading

weaverryan commented Jun 19, 2019

dunglas commented Jun 19, 2019

develth commented Jul 4, 2019 •

edited

Loading

stepanselyuk commented Oct 18, 2019

[Validation] Activate auto-mapped validation via an annotation #32070

[Validation] Activate auto-mapped validation via an annotation #32070

Comments

weaverryan commented Jun 17, 2019

fabpot commented Jun 17, 2019

dunglas commented Jun 17, 2019

weaverryan commented Jun 18, 2019

weaverryan commented Jun 18, 2019

dunglas commented Jun 18, 2019 • edited Loading

weaverryan commented Jun 19, 2019

dunglas commented Jun 19, 2019

develth commented Jul 4, 2019 • edited Loading

stepanselyuk commented Oct 18, 2019

dunglas commented Jun 18, 2019 •

edited

Loading

develth commented Jul 4, 2019 •

edited

Loading