Hi @dunglas, this means in sf 4.3 it is a default feature (doctrine field are alwas by default validated) ?
the dev need to optout to get rid off it? if yes, why not the other side?
thank you

@noniagriconomie see symfony/recipes#612

BTW, I think we also need an opt-in annotation
having both config enabling+exclusion list and annotations to enable/disable looks the most sensible to me. Every style covered, maximum flexibility.

I agree with Nicolas here, Opt-In is definitely needed too 👍🏻

thx for both answers @nicolas-grekas @OskarStark :)

IMHO it should be enabled by default in the recipes (and allow to opt-out some classes or properties) and having an opt-in feature is a nice to have.

As we experienced issues when we enabled it by default and side effects that are difficult to understand, I'm in favor of only supporting the opt-in mode and only for classes/properties.

Being opt-in only defects the purpose of this feature and decreases the security benefits it provides: #32070 (comment)

Most side effects are because this feature is young, this PR should be enough to prevent the last remaining ones.

The feature is already opt-in using the config file (it’s off by default). I don’t understand how the AutoMapping constraint would work? I mean, you must already opt in a specific directory using the config, and you can exclude some classes or properties using this new annotation. How the proposed annotation would integrate with this?

Opt-in can still be made the default in maker-bundle, so that the security you want @dunglas is still a thing.

Many people (including me) don't use maker bundle to create entities (for instance, almost all PHPStorm users, I guess). And it's one more annotation to not forget to add to every new class, for no reasons. Actually, I prefer the current situation (having to uncomment one time for all some lines in a config file) than having to add an annotation to all new entities.

Travis failures are expected (we'll have to bump the required Validator version or to add conflict rules when merged).

That's why I think we need config + annotation, opt-in/out + list/exclusion, to cover all styles.

Here is a new proposal that should satisfy everybody:

• Change the constraint to @AutoMapping(true/false), true being the default value.
• 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
• The config stays unchanged (with the new behavior, there is no need to a config for exclusions, it's already handled by the constraint) ; and is uncommented by default in the recipe

It should cover all use cases, the feature is still on by default for the users of the framework thanks to the recipe. Do you folks agree?

Annotations always win, looks sensible to me.
I would still like to have the exclusion list in the config, but someone else can provide it in another PR.

Should target 4.4 to me, except the bug fix part of course.

With this proposal, if no annotations are added (assuming we re-add the auto mapping config in the recipe in 4.4), then auto mapping would still be turned on, correct? We could then, in make:entity decide to always generate entities with AutoMapping(true) to be explicit and make it more obvious how to turn it off (or even answer the question “where is this validation coming from?”). I think that sensible... though we may debate later whether to have the default config be “always on without annotation” or “off, unless the annotation is present” (or an I misunderstanding how the config would work?)

Thanks for the conversation. This feature makes me a bit uncomfortable... but @dunglas may very well be correct that it’s just this that’s needed to make it shine. After all, the form fields themselves hav built-in, automatic “sanity” validation, and that’s never been a problem (not an exact comparison).

I think disabling all automatic constraints at once is a bit rough. Think, for example, of a decimal property type-hinted to float (or a Decimal object or whatever) instead of Doctrine's internal type string. At the moment, this leads to an invalid Type constraints. Disabling all automatic constraints for that property would mean to give up the wanted ones. This wouldn't be a big problem as the missing ones can easily be readded explicitly and as it is an Annotation, a dev will see that the automatic is disabled. But doing that the correct way requires devs to know about them all and keep track, when new ones will be added in the future.

Wouldn't it be less invasive to only disable the specifically problematic automatic constraints? Like @Assert\NoAutoMapping(['Type', 'NotNull'])?

This one should be ready to be merged now. The last decision to make is between this single annotation and EnableAutoMapping+DisableAutoMapping as suggested by @xabbuh. Ping @symfony/mergers

I think I prefer Enable/DisableAutoMapping because it opens for better extensibility.
One idea I have in mind would be to handle an auto_mapper attribute on the validator.auto_mapper tag:

<tag name="validator.auto_mapper" auto_mapper="property_info" />

Then @Assert\DisableAutoMapping("property_info") would allow specifically disabling this auto mapper (or enable it with @Assert\EnableAutoMapping("property_info")).

EnableAutoMapping and DisableAutoMapping added. It should be ready to be merged now. /cc @xabbuh

Thank you Kévin.