Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Form] Deprecated "cascade_validation"#12237

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@webmozart
Copy link
Contributor

QA
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?yes
Tests pass?yes
Fixed tickets#11268 (requires explicit work though)
LicenseMIT
Doc PRTODO

The "cascade_validation" option was designed for a 1% use case and comparatively used way too often when theValid constraint should have been used instead. Also, there seem to be bugs with that option (#5204).

The option is now deprecated. When using the 2.5 Validator API, you can set the "constraints" option of the respective child to aValid constraint instead. Alternatively, set the constraint in the model (as most people hopefully do).

@craue
Copy link
Contributor

Instead of removing that option I'd prefer it would be recursively passed properly to all children (as I expected from its name). That would make validation more failsafe when you don't need to remember adding theValid constraint everywhere. When I validate something, I expect that even data in child forms is validated, so I had to generally setcascade_validation in all my forms, but sometimes got hit by#5204 and had to add an additionalValid constraint to make validation work properly. Thus 👎 from me.

@stof
Copy link
Member

@craue cascade_validation was introduced s a way to reproduce the 2.0 behavior of the form validation. It is not intended to be the standard way of validating forms (which is why it is disabled by default).

@peterrehm
Copy link
Contributor

Why don't we think about validation by default all child entites? I think this is what you usually need. I acutally got used to cascade_validation since it is the way how it has been documented.

http://symfony.com/doc/current/book/forms.html#embedding-a-single-object

When docs will be updated this should be covered as well.

@peterrehm
Copy link
Contributor

In addition to that Assert\Valid() works differently compared to cascade_validation as discussed here:

#9650 (comment)

I think the way how cascade_validation works is actually what you would expect. There are cases where your child forms will have validation groups based on there state so you should not add this to the parent form. Or what do you recommend in regard to this?

@peterrehm
Copy link
Contributor

I would say if this is changed the validation_groups of the child forms should be respected accordingly as with the cascade_validation setting. I would assume moving all people from cascade_validation to Assert\Valid() and not providing a solution for the validation_groups setting to have validation groups based on the entity state would lead to many issues.

Or we add another logic and tie the logic to the entity as we started to discuss here#11880.

@webmozart
Copy link
ContributorAuthor

@craue As I mentioned in the ticket description, there's almost no valid use case to use "cascade_validation". Use the Valid constraint, always.

@peterrehm This is in fact a documentation issue. I opened a new ticket there:symfony/symfony-docs#4346

Or we add another logic and tie the logic to the entity as we started to discuss here#11880.

That's the correct approach to solve the problem IMO.

@peterrehm
Copy link
Contributor

Is there anything else affected rather than the validation_groups setting of the form?
How about deprecating the validation_groups setting as well when we find a solution for#11880.
I will try to reply to you later today or tomorrow.

@webmozart
Copy link
ContributorAuthor

How about deprecating the validation_groups setting as well when we find a solution for#11880.

I'm not sure that's a good idea, but it's an option.

@peterrehm
Copy link
Contributor

But if not deprecated (I don't know as well if it is a good idea) I think we should consider cascading them as cascade_validation does.

weaverryan added a commit to symfony/symfony-docs that referenced this pull requestOct 29, 2014
…ed forms to Valid... (peterrehm)This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes#4348).Discussion----------Updated information about handling validation of embedded forms to Valid...| Q             | A| ------------- | ---| Doc fix?      | yes| New docs?     | no| Applies to    | all| Fixed tickets |#4346Applied Valid constraint instead of the cascade_validation option since this option is supposed to be deprecated withsymfony/symfony#12237Commits-------b699731 Updated information about handling validation of embedded forms to Valid constraint
@webmozart
Copy link
ContributorAuthor

ping @symfony/deciders

@Tobion
Copy link
Contributor

The "cascade_validation" option was designed for a 1% use case

@webmozart what is that 1%?

@stof
Copy link
Member

@webmozart this PR should probably be sent again with the 2.7 branch as target (and rebased) so that tests can run (Travis is currently not working on 3.0.x-dev even if you rebase).

I would like to see#11268 fixed as it is a nasty bug.

@peterrehm
Copy link
Contributor

Any progress on that one?

@webmozart
Copy link
ContributorAuthor

@Tobion The use case when you have a subform with an object that is not referred to from the parent form's object (so@Valid can't be used). In practice, this rarely happens.

gimlerand others added6 commitsJune 11, 2015 21:27
…imler)This PR was merged into the 2.8 branch.Discussion----------fix Merge branch '2.7' into 2.8 JsonFileLoaderThis fix a merge commit seesymfony@5593bdd. The `stream_is_local` and `file_exists` checks are all ready done in the parent `FileLoader` class.| Q             | A| ------------- | ---| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | no| Fixed tickets | none| License       | MIT| Doc PR        | noneCommits-------692deff fix Merge branch '2.7' into 2.8 JsonFileLoader
This PR was merged into the 2.8 branch.Discussion----------[Profiler][Translation] added filter.| Q             | A| ------------- | ---| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Fixed tickets  | ~| Tests pass?   | yes| License       | MIT![selection_017](https://cloud.githubusercontent.com/assets/1753742/8159914/c2c8ad68-135a-11e5-9ae3-f2e055ea3230.jpg)Commits-------65f9291 [Profiler][Translation] added filter.
This PR was submitted for the master branch but it was merged into the 2.8 branch instead (closessymfony#12067).Discussion----------[Form] Added the 'range' FormType| Q             | A| ------------- | ---| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |symfony#11979| License       | MIT| Doc PR        |Implemented the "range" FormType.Commits-------b52e197 [Form] Added the 'range' FormType
@webmozart
Copy link
ContributorAuthor

Replaced by#15019.

fabpot added a commit that referenced this pull requestJun 17, 2015
This PR was merged into the 2.8 branch.Discussion----------[Form] Deprecated "cascade_validation"| Q             | A| ------------- | ---| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets |#11268 (requires explicit work though)| License       | MIT| Doc PR        | TODOThis is#12237 rebased on 2.8.The "cascade_validation" option was designed for a 1% use case and comparatively used way too often when the `Valid` constraint should have been used instead. Also, there seem to be bugs with that option (#5204).The option is now deprecated. When using the 2.5 Validator API, you can set the "constraints" option of the respective child to a `Valid` constraint instead. Alternatively, set the constraint in the model (as most people hopefully do).Commits-------6c554c6 [Form] Deprecated "cascade_validation"
weaverryan added a commit to symfony/symfony-docs that referenced this pull requestJun 19, 2015
…n constraint (peterrehm)This PR was submitted for the master branch but it was merged into the 2.8 branch instead (closes#4354).Discussion----------[WCM] Added depreciation note for the cascade_validation constraint| Q             | A| ------------- | ---| Doc fix?      | yes| New docs?     | no| Applies to    | 2.8| Fixed tickets | -This PR was based onsymfony/symfony#12237 and has been updated basedassymfony/symfony#15019.#4348Commits-------22a87b5 Added depreciation note for the cascade_validation constraint and updated position of depreciation notes
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@webmozart@craue@stof@peterrehm@Tobion@gimler@fabpot@aitboudad@jaytaph

[8]ページ先頭

©2009-2025 Movatter.jp