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

checking csrf_protection in delete method. #5712

Conversation

oleg-andreyev
Copy link
Contributor

@oleg-andreyev oleg-andreyev commented Oct 11, 2019

Subject

There are cases when I can disable csrf_protection via $formOptions, but when I call deleted method this option is ignored, because deleteAction does not use Form

Ideally when deleteAction should use "delete" form (empty form)

I am targeting this branch, because feature.

Changelog

### Changed
- deleteAction now respects csrf_protection

To do

  • Update the tests

@greg0ire
Copy link
Contributor

There is a tiny CS issue to fix

tests/Controller/CRUDControllerTest.php Outdated Show resolved Hide resolved
tests/Controller/CRUDControllerTest.php Outdated Show resolved Hide resolved
@greg0ire greg0ire requested a review from phansys October 26, 2019 22:40
@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@core23
Copy link
Member

core23 commented Nov 9, 2019

Can you please rebase @oleg-andreyev ?

@oleg-andreyev oleg-andreyev force-pushed the check-csrf_protection-in-delete-action branch from 458f5aa to a887c5b Compare November 9, 2019 19:47
@oleg-andreyev
Copy link
Contributor Author

@core23 done.

@phansys phansys added the minor label Nov 11, 2019
@OskarStark OskarStark merged commit cb6b75f into sonata-project:3.x Nov 11, 2019
@VincentLanglet
Copy link
Member

Hi ! This PR seems to introduce a bug, cf #5915

Isn't another way to get the csrf_protection option without calling the configureFormField ?
Having $this->getSubject() possibly null in the configureFormField seems wrong to me.

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.

None yet

7 participants