8000 [Form] Fixed ValidatorTypeGuesser to guess properties without constraints not to be required by webmozart · Pull Request #12004 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Form] Fixed ValidatorTypeGuesser to guess properties without constraints not to be required #12004

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

Merged
merged 1 commit into from
Sep 24, 2014

Conversation

webmozart
Copy link
Contributor
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #6645
License MIT
Doc PR -

Consider the following entity:

class Author
{
    /**
     * @Assert\NotBlank
     */
    private $name;

    private $age;
}

Right now, the "required" HTML attribute is set for both fields (since the default value of the "required" option is true). IMO this is wrong.

With this fix, the ValidatorTypeGuesser guesses false for the "required" option unless a NotNull/NotBlank constraint is present.

@fabpot
Copy link
Member
fabpot commented Sep 24, 2014

Isn't it a BC break?

@webmozart
Copy link
Contributor Author

@fabpot I don't think so. Before, when a field had no constraint, the "required" option was set to true by default, validating it on the client side (but not server side). I can't think of a situation where this behavior makes sense. Most people probably set "required" to false explicitly in such cases.

@fabpot
Copy link
Member
fabpot commented Sep 24, 2014

Got it now. 👍

2.3.20
------

* the validator guesser guesses fields for properties without any constraints
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be removed as the changelogs for minor versions are auto-generated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@webmozart webmozart force-pushed the < 8000 span > issue6645 branch from 2571828 to fd77b09 Compare September 24, 2014 12:11
@fabpot
Copy link
Member
fabpot commented Sep 24, 2014

Thank you @webmozart.

@fabpot fabpot merged commit fd77b09 into symfony:2.3 Sep 24, 2014
fabpot added a commit that referenced this pull request Sep 24, 2014
…out constraints not to be required (webmozart)

This PR was merged into the 2.3 branch.

Discussion
----------

[Form] Fixed ValidatorTypeGuesser to guess properties without constraints not to be required

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

Consider the following entity:

```php
class Author
{
    /**
     * @Assert\NotBlank
     */
    private $name;

    private $age;
}
```

Right now, the "required" HTML attribute is set for both fields (since the default value of the "required" option is true). IMO this is wrong.

With this fix, the ValidatorTypeGuesser guesses `false` for the "required" option unless a NotNull/NotBlank constraint is present.

Commits
-------

fd77b09 [Form] Fixed ValidatorTypeGuesser to guess properties without constraints not to be required
@Tobion
Copy link
Contributor
Tobion commented Sep 24, 2014

@webmozart wasn't this fixed already in #3821
It's also related to #5365 but does not fix it since the inconsistency is still there when not using guessing.

@webmozart
Copy link
Contributor Author

@Tobion Not really, since a class may have properties with and without NotNull/NotBlank constraints. Before this PR, all fields were set to required.

You are right, however, that the origin of this problem lies in the default value of "required". I opened a PR for that: #12018

@webmozart webmozart deleted the issue6645 branch October 22, 2014 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0