-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Form] Removed extra CSRF field on collection prototype #3996
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
Conversation
Wait a minute please, I oversaw some tests that have to be adapted. |
{ | ||
if ($form->isRoot() && $form->hasChildren() && $form->hasAttribute('csrf_field_name')) { | ||
if (!$view->hasParent() && $view->hasChildren() && $form->hasAttribute('csrf_field_name')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bschussek I came to about the same conclusion: !$view->hasParent() && $form->hasChildren() && $form->hasAttribute('csrf_field_name')
. I thought the children of the view might not be there yet ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see: "BottomUp"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why I changed it to buildViewBottomUp, where the children are already there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to check the children at all ? (what if the root form is an empty collection)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, otherwise a simple input field cannot be submitted. If an empty collection form has a CSRF field, it doesn't hurt anyone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty collection -> hasChildren() === false
-> no CSRF ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, got me there :) An empty collection (or form in general for that sake) doesn't transmit data, so there's nothing to protect, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- couldn't this be used to erase a collection (not protected) ?
- what if children are added dynamically ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have created a new issue as this PR is closed
Fixed. ping @fabpot |
Commits ------- ccd6bbc [Form] Removed extra CSRF field on collection prototype Discussion ---------- [Form] Removed extra CSRF field on collection prototype Bug fix: yes Feature addition: no Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: #3987, #3990 Todo: -  --------------------------------------------------------------------------- by bschussek at 2012-04-19T08:43:32Z Wait a minute please, I oversaw some tests that have to be adapted. --------------------------------------------------------------------------- by bschussek at 2012-04-19T09:22:45Z Fixed. ping @fabpot
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #3987, #3990
Todo: -