Skip to content
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

[Validator][DoctrineBridge][FWBundle] Automatic data validation #27735

Merged
merged 1 commit into from Mar 31, 2019

Conversation

@dunglas
Copy link
Member

commented Jun 26, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR symfony/symfony-docs#11132

This feature automatically adds some validation constraints by inferring existing metadata. To do so, it uses the PropertyInfo component and Doctrine metadata, but it has been designed to be easily extendable.

Example:

use Doctrine\ORM\Mapping as ORM;

/**
 * @ORM\Entity
 */
class Dummy
{
    /**
     * @ORM\Id
     * @ORM\GeneratedValue(strategy="AUTO")
     * @ORM\Column(type="integer")
     */
    public $id;

    /**
     * @ORM\Column(nullable=true)
     */
    public $columnNullable;

    /**
     * @ORM\Column(length=20)
     */
    public $columnLength;

    /**
     * @ORM\Column(unique=true)
     */
    public $columnUnique;
}

$manager = $this->managerRegistry->getManager();
$manager->getRepository(Dummy::class);

$firstOne = new Dummy();
$firstOne->columnUnique = 'unique';
$firstOne->columnLength = '0';

$manager->persist($firstOne);
$manager->flush();

$dummy = new Dummy();
$dummy->columnNullable = 1; // type mistmatch
$dummy->columnLength = '012345678901234567890'; // too long
$dummy->columnUnique = 'unique'; // not unique

$res = $this->validator->validate($dummy);
dump((string) $res);

/*
Object(App\Entity\Dummy).columnUnique:\n
    This value is already used. (code 23bd9dbf-6b9b-41cd-a99e-4844bcf3077f)\n
Object(App\Entity\Dummy).columnLength:\n
    This value is too long. It should have 20 characters or less. (code d94b19cc-114f-4f44-9cc4-4138e80a87b9)\n
Object(App\Entity\Dummy).id:\n
    This value should not be null. (code ad32d13f-c3d4-423b-909a-857b961eb720)\n
Object(App\Entity\Dummy).columnNullable:\n
    This value should be of type string. (code ba785a8c-82cb-4283-967c-3cf342181b40)\n
*/

It also works for DTOs:


class MyDto
{
    /** @var string */
    public $name;
}

$dto = new MyDto();
$dto->name = 1; // type error

dump($validator->validate($dto));

/*
Object(MyDto).name:\n
    This value should be of type string. (code ba785a8c-82cb-4283-967c-3cf342181b40)\n
*/

Supported constraints currently are:

  • @NotNull (using PropertyInfo type extractor, so supports Doctrine metadata, getters/setters and PHPDoc)
  • @Type (using PropertyInfo type extractor, so supports Doctrine metadata, getters/setters and PHPDoc)
  • @UniqueEntity (using Doctrine's unique metadata)
  • @Length (using Doctrine's length metadata)

Many users don't understand that the Doctrine mapping doesn't validate anything (it's just a hint for the schema generator). It leads to usability and security issues (that are not entirely fixed by this PR!!).
Even the ones who add constraints often omit important ones like @Length, or @Type (important when building web APIs).
This PR aims to improve things a bit, and ease the development process in RAD and when prototyping. It provides an upgrade path to use proper validation constraints.

I plan to make it opt-in, disabled by default, but enabled in the default Flex recipe. (= off by default when using components, on by default when using the full stack framework)

TODO:

  • Add configuration flags
  • Move the Doctrine-related DI logic from the extension to DoctrineBundle: doctrine/DoctrineBundle#831
  • Commit the tests

@dunglas dunglas force-pushed the dunglas:doctrine-validator-bridge branch from 3d02781 to 1dd97e4 Jun 26, 2018

@nicolas-grekas nicolas-grekas added this to the next milestone Jun 26, 2018

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

Truly black magic. 🔮

It should at least be possible to opt-in or out for each class via annotation.

@linaori

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

I don't really like this to be honest, but primarily because I don't like RAD or entity validation. In my opinion entities should never even reach an invalid state. While I do agree that for RAD purposes this might be nice, I don't know if this should be available in the core.

@ostrolucky

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

This is not just for entities. There are multiple different loaders. Those who like DTOs benefit from this too.

@dunglas

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2018

Truly black magic. 🔮

Why? It's not magic at all (and definitely not more than an annotation that can automatically create the underlying database table 😄).

It should at least be possible to opt-in or out for each class via annotation.

👍, it can be a new dummy constraint, @NoAutoValid, so it works using annotations, XML and YAML, and is similar to the existing @All and @Valid special constraints.

While I do agree that for RAD purposes this might be nice, I don't know if this should be available in the core.

It's 100% opt-in, and in sync with what Symfony already allows/encourages for RAD (@UniqueEntity for instance). It doesn't force to use entity validation, it's up to the developper, but for typical CRUD/RAD/small apps, it allows to improve the UX/security by default, and to save some precious time.

I recently stumbled upon this quote (in another context, Kubernetes deployment):

For applications that are brand new, their biggest risk is that they don’t find product/market fit. That is, they get deployed and never used. It’s code that ends up getting thrown away because it was something built that no one actually wanted. This represents the vast majority of application code that gets written. It’s certainly where I’ve spent a good deal of my career, iterating on code and features in a search for the right set. Once you find that application and feature set, you can then scale it out. But to do so before that point is a premature optimization.

https://www.influxdata.com/blog/will-kubernetes-collapse-under-the-weight-of-its-complexity/

It's exactly my feeling about RAD: iterate quickly, get the shit done as fast as possible, test, fail, try again. And when the app is proved to be useful, stop using RAD tools and move to a more heavy and time-resistant design.

If it is useful for RAD, it should be in core. Symfony 4 is all about allowing to develop faster (while still allowing to handle more complex needs, of course). And we had a bad experience with less visible external bundles that are abandoned after some time...

@dkarlovi

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

Could it be opt-in per class, not opt-out per class?

@linaori

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

And when the app is proved to be useful, stop using RAD tools and move to a more heavy and time-resistant design.

Except that this part usually doesn't happen. Developers see "Symfony does this, so it must be good", they are still binding entities to forms for example, causing overly complex situations with big forms that simply would require a DTO and take only a fraction of the time to write. Stack-overflow, IRC (previously) and Slack are full of those related questions. It's probably one of the top 3 issues arising via support channels.

I'm not going to say "don't add this to Symfony!", because for that specific use-case, it can be nice for new-comers to see it work. However, I'm also of opinion that it's wrong of the docs to show those "bad practices" as the primary examples and advocate it as easy. Yes it's RAD, yes it's easy to setup and yes, there are some advantages when you look at it from a short-term perspective. But honestly, writing a DTO takes 2 minutes and would make forms so much more simple as there is no complex binding, no changes between data types, no guessing for the entity type, no repository magic via callable form options, no form events that need to be listened to etc.

I understand exactly what problem you're trying to solve, but what about this?

  • Document DTOs instead of entities for forms and the validator
  • Use the awesome new maker bundle to generate DTOs + FormTypes + the validation, based on an entity and a (subset) of its fields
  • Find a way to easily heave those values into entities via tools such as AutoMapper+
@dunglas

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2018

@dkarlovi opt-in per class breaks the overall experience don't you think? It will already be opt-in globally. Opt-in globally + opt-out per-class with an annotation as suggested by @teohhanhui looks like a better compromise to me.

@iltar I personally don't use the Form component anymore for a while (not because of the docs, but because I use JS + AJAX for my forms). Honestly I mostly have API Platform in mind here, even if it will probably help for forms too.

Regarding docs and forms suggestion, can they be discussed in another issue? I don't think it's related.

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

When something has side effects without being asked for, it's magic. Having annotations to provide mapping information for the ORM is not comparable to having validation rules automagically added based on the ORM mapping. It's also not the same as convention where it simply removes the need for explicit configuration by following some obvious and well-defined rules.

@dunglas

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2018

@teohhanhui it's opt-in, and it also works for DTOs (may be less "magic"). I've updated the main description to show an example.

Btw, Doctrine 1 used to have exactly this behavior 😄 http://doctrine1.readthedocs.io/en/latest/en/manual/data-validation.html. Back in the old days. (full disclosure: I'm still a Symfony 1 fan)

@dkarlovi

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

@dunglas TBH I'd rather see per-class opt-in, not too keen on global opt-in at all, it seems "far away". Maybe local and global opt-in as a compromise?

Since this already seems like magic, it would make it less so it was localized together where you'd expect to see validation rules either way.

@dunglas

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2018

@dkarlovi if an user don't want to use this convention, he will just not enable this feature. Requiring to add an annotation on every class you create breaks the "add a class, map it and you're done" experience and make the feature less useful. I prefer to keep it as is, with - why not - the opt-out extra annotation. With this feature feature, to get a 100% valid API you'll just have to create this class:

/** @Entity @ApiResource */
class Book
{
    /** @Id @Column */
    public $id;
    /** @Column */
    public $name;
    /** @Column(type="int") */
    public $price;
}
@jvasseur

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

Having this feature globally opt-in will break third party bundles both both when they expect this feature to be enabled when it isn't and when they don't use it and it's enabled. So a bit 👎 for having this globally configured, if it's configured on a per-class basis I'm ok with it.

Maybe a solution would be to add the "auto" constraints in a separate validation group, that would make it easy to decide if you want to use them or not.

@dunglas

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2018

We discussed it with @nicolas-grekas, and the config can look like this:

validation:
    autovalidate:
        'App\': ~ # enable all built-in loaders
        'App\Foo': ['My\Loader', 'SF\Validator\Loader\PropertyInfo', 'DoctrineBundle\ValidatorLoader'] # enable only some specific services
@jvasseur

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

BTW, I don't think "auto[matic] validation" is the right name for this feature, it make me thinks data will automatically be validated without having to call the validate method.

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

auto_mapping, perhaps?

@ogizanagi

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

I had this in mind since months for DTOs, so big 👍 from me.
I also think this should be opt-in only, an opt-out annotation would be really weird and likely to cause issues with third parties and WTF moments. But the autovalidate configuration looks good (regarding the name, I'd rather like auto_mapping too).

@linaori

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2018

Perhaps adding a new annotation would help to enable this feature? @Assert\BasedOnEntity?

@dunglas conceptually seen, forms are almost the same as what API platform achieves. You have request data, which gets transformed into an object (unserialized) and this is being validated. I understand your focus on api platform (great project btw!), but to me, it's the same as forms on a conceptual level, hence I feel like DTOs are also a better option for APIs.

@ostrolucky

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2018

enable_implicit_constraints?

I don't want having to enable this per-class, it defeats its purpose.

This is not a place for discussion about Forms and DTOs. Please, don't drag it here and go discuss it somewhere else. But like I said, this is useful even for DTOs.

@linaori

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2018

@ostrolucky this feature seems to aim specifically at doctrine annotations, thus it's tightly coupled with doctrine (and thus not re-usable for DTOs): https://github.com/symfony/symfony/pull/27735/files#diff-b24d5fab7288b31bac42d19f60ab9f49R71

I highly disagree with your comment about Forms and DTOs as well. Forms are a form (no pun intended) of (un)serializing a request. While this particular PR is broader than just forms, one of the most used components where this new feature would hit, would be forms. Due to this PR aiming to solve automatic validation based on Doctrine mapping on Entities, and forms being the biggest component to be impacted by this feature, I think it most certainly is important to discuss this. If you use DTOs and not entities as subject (regardless of forms or API), this feature would not be necessary. In combination of exposing entity state via (un)serialization, with the whole "use entities in forms" culture in the Symfony documentation, is what leads to features like this. Solve the core problem (entities being misused), don't patch a symptom.

The design of Symfony will always allow RAD and entities being used as such. There's nothing wrong with a developer picking this path. But making a bad practice easier to be used, rather than the good practice, seems like a step backwards to me.

Related discussion in the docs: symfony/symfony-docs#8893

@ostrolucky

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2018

Why are you still ignoring that this is useful for DTOs too? Even if you use DTO, you will make a mistake and forget to add Type/NotBlank constraints and this will help with that. This feature isn't about necessity, but about safer and easier validation. It happened to me so many times we did forget about such constraints and user gets error 500, then having to come back and add those constraints manually.

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

What's the configuration you are thinking to enable/disable this feature?

An annotation for each entity?

/**
 * @ORM\Entity
 * @Autovalidation
 */
class Dummy
{
    // ...
}

A global framework.autovalidation option? Both? Something different? Thanks!

@linaori

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2018

@ostrolucky if you check what I've linked and read the PR, you'll see that this particular feature is to read doctrine mappings on an object and translate that to the corresponding symfony validation constraints. DTOs don't have doctrine mappings, only entities do, thus this won't ever work for DTOs.

@javiereguiluz an annotation like that is what I was thinking of, it's completely opt-in at that point. It's just really hard to figure out what it will validate, especially considering fields are never nullable by default, but relations are always nullable by default. So if you wonder why it's never validated if a relation is created, you might not figure it out any time soon.

@dunglas

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2018

@iltar

@ostrolucky this feature seems to aim specifically at doctrine annotations, thus it's tightly coupled with doctrine (and thus not re-usable for DTOs): https://github.com/symfony/symfony/pull/27735/files#diff-b24d5fab7288b31bac42d19f60ab9f49R71

There actually 3 parts in this PR:

  • A new mechanism to allow registering custom loaders (generic)
  • A loader using PropertyInfo (generic, already support getter/setters, PHPDoc and Doctrine metadata)
  • A loader for Doctrine metadata (length and unique, specific)

The 2 first are useful for DTOs too, I updated the PR description yesterday to highlight this use case.

@javiereguiluz regarding config, WDYT about this proposal? #27735 (comment)

@dunglas

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2018

hence I feel like DTOs are also a better option for APIs.

It's where API Platform differs from Forms. Class marked with @ApiResource are essentially DTO, that can also and optionally be entities for RAD: api-platform/core#1747 (comment)

@fabpot

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

I think that this is the kind of PR that needs some docs before we can merge it. Would you agree to write a short doc as a PR on symfony-docs @dunglas?

@Kronhyx

Kronhyx approved these changes Jan 7, 2019

@dunglas

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2019

@fabpot @symfony/deciders docs added and PR rebased.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

Small rebase needed.
@weaverryan I'd love your +1 here :)

@dunglas

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2019

Rebased

@dunglas dunglas force-pushed the dunglas:doctrine-validator-bridge branch from 74de5e6 to 2d64e70 Mar 21, 2019

@fabpot

fabpot approved these changes Mar 31, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

Thank you @dunglas.

@fabpot fabpot merged commit 2d64e70 into symfony:master Mar 31, 2019

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 31, 2019

feature #27735 [Validator][DoctrineBridge][FWBundle] Automatic data v…
…alidation (dunglas)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Validator][DoctrineBridge][FWBundle] Automatic data validation

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes<!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | n/a   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#11132

This feature automatically adds some validation constraints by inferring existing metadata. To do so, it uses the PropertyInfo component and Doctrine metadata, but it has been designed to be easily extendable.

Example:

```php
use Doctrine\ORM\Mapping as ORM;

/**
 * @Orm\Entity
 */
class Dummy
{
    /**
     * @Orm\Id
     * @Orm\GeneratedValue(strategy="AUTO")
     * @Orm\Column(type="integer")
     */
    public $id;

    /**
     * @Orm\Column(nullable=true)
     */
    public $columnNullable;

    /**
     * @Orm\Column(length=20)
     */
    public $columnLength;

    /**
     * @Orm\Column(unique=true)
     */
    public $columnUnique;
}

$manager = $this->managerRegistry->getManager();
$manager->getRepository(Dummy::class);

$firstOne = new Dummy();
$firstOne->columnUnique = 'unique';
$firstOne->columnLength = '0';

$manager->persist($firstOne);
$manager->flush();

$dummy = new Dummy();
$dummy->columnNullable = 1; // type mistmatch
$dummy->columnLength = '012345678901234567890'; // too long
$dummy->columnUnique = 'unique'; // not unique

$res = $this->validator->validate($dummy);
dump((string) $res);

/*
Object(App\Entity\Dummy).columnUnique:\n
    This value is already used. (code 23bd9dbf-6b9b-41cd-a99e-4844bcf3077f)\n
Object(App\Entity\Dummy).columnLength:\n
    This value is too long. It should have 20 characters or less. (code d94b19cc-114f-4f44-9cc4-4138e80a87b9)\n
Object(App\Entity\Dummy).id:\n
    This value should not be null. (code ad32d13f-c3d4-423b-909a-857b961eb720)\n
Object(App\Entity\Dummy).columnNullable:\n
    This value should be of type string. (code ba785a8c-82cb-4283-967c-3cf342181b40)\n
*/
```

It also works for DTOs:

```php

class MyDto
{
    /** @var string */
    public $name;
}

$dto = new MyDto();
$dto->name = 1; // type error

dump($validator->validate($dto));

/*
Object(MyDto).name:\n
    This value should be of type string. (code ba785a8c-82cb-4283-967c-3cf342181b40)\n
*/
```

Supported constraints currently are:

* `@NotNull` (using PropertyInfo type extractor, so supports Doctrine metadata, getters/setters and PHPDoc)
* `@type` (using PropertyInfo type extractor, so supports Doctrine metadata, getters/setters and PHPDoc)
* `@UniqueEntity` (using Doctrine's `unique` metadata)
* `@Length` (using Doctrine's `length` metadata)

Many users don't understand that the Doctrine mapping doesn't validate anything (it's just a hint for the schema generator). It leads to usability and security issues (that are not entirely fixed by this PR!!).
Even the ones who add constraints often omit important ones like `@Length`, or `@type` (important when building web APIs).
This PR aims to improve things a bit, and ease the development process in RAD and when prototyping. It provides an upgrade path to use proper validation constraints.

I plan to make it opt-in, disabled by default, but enabled in the default Flex recipe. (= off by default when using components, on by default when using the full stack framework)

TODO:

* [x] Add configuration flags
* [x] Move the Doctrine-related DI logic from the extension to DoctrineBundle: doctrine/DoctrineBundle#831
* [x] Commit the tests

Commits
-------

2d64e70 [Validator][DoctrineBridge][FWBundle] Automatic data validation

@dunglas dunglas deleted the dunglas:doctrine-validator-bridge branch Mar 31, 2019

@Hanmac

This comment has been minimized.

Copy link

commented Apr 2, 2019

@dunglas the @var thing is not done yet with this PR, or did i read it wrong because i didn't found it in the code?

@stof

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

@Hanmac the PropertyInfo component was already able to read them. And this PR adds a PropertyInfoLoader.

@dmaicher

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

@dunglas @stof is there a dependency on symfony/property-info missing?

I just updated the symfony-demo application to use dev-master components and now I have this error:

Loading composer repositories with package information
Updating dependencies (including require-dev)
Restricting packages listed in "symfony/symfony" to "dev-master"
Package operations: 0 installs, 0 updates, 0 removals
Writing lock file
Generating autoload files
ocramius/package-versions:  Generating version class...
ocramius/package-versions: ...done generating version class
Executing script cache:clear [KO]
 [KO]
Script cache:clear returned with error code 255
!!  
!!  Fatal error: Interface 'Symfony\Component\PropertyInfo\PropertyListExtractorInterface' not found in /var/www/symfony-demo/vendor/symfony/doctrine-bridge/PropertyInfo/DoctrineExtractor.php on line 29
!!  PHP Fatal error:  Interface 'Symfony\Component\PropertyInfo\PropertyListExtractorInterface' not found in /var/www/symfony-demo/vendor/symfony/doctrine-bridge/PropertyInfo/DoctrineExtractor.php on line 29
!!  
!!  In DoctrineExtractor.php line 29:
!!                                                                                                                   
!!    Attempted to load interface "PropertyListExtractorInterface" from namespace "Symfony\Component\PropertyInfo".  
!!    Did you forget a "use" statement for another namespace?                                                        
!!                                                                                                                   
!!  
!!  

Edit: see #30948

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

@dmaicher can you open a separate PR please if this needs tracking?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.