Skip to content

Do not use a Symfony Form to delete an article #427

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
Jan 12, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions app/Resources/views/admin/blog/_delete_form.html.twig
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{{ include('blog/_delete_post_confirmation.html.twig') }}
<form action="{{ url('admin_post_delete', { id: post.id }) }}" method="post" data-confirmation="true">
<input type="hidden" name="token" value="{{ csrf_token('delete') }}" />
<input type="submit" value="{{ 'action.delete_post'|trans }}" class="btn btn-lg btn-block btn-danger" />
</form>
9 changes: 2 additions & 7 deletions app/Resources/views/admin/blog/edit.html.twig
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,15 @@
<h1>{{ 'title.edit_post'|trans({'%id%': post.id}) }}</h1>

{{ include('admin/blog/_form.html.twig', {
form: edit_form,
form: form,
button_label: 'action.save'|trans,
include_back_to_home_link: true,
}, with_context = false) }}
{% endblock %}

{% block sidebar %}
<div class="section actions">
{{ include('admin/blog/_form.html.twig', {
form: delete_form,
button_label: 'action.delete_post'|trans,
button_css: 'btn btn-lg btn-block btn-danger',
show_confirmation: true,
}, with_context = false) }}
{{ include('admin/blog/_delete_form.html.twig', { post: post }, with_context = false) }}
</div>

{{ parent() }}
Expand Down
7 changes: 1 addition & 6 deletions app/Resources/views/admin/blog/show.html.twig
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,7 @@
</div>

<div class="section">
{{ include('admin/blog/_form.html.twig', {
form: delete_form,
button_label: 'action.delete_post'|trans,
button_css: 'btn btn-lg btn-block btn-danger',
show_confirmation: true,
}, with_context = false) }}
{{ include('admin/blog/_delete_form.html.twig', { post: post }, with_context = false) }}
</div>

{{ show_source_code(_self) }}
Expand Down
54 changes: 13 additions & 41 deletions src/AppBundle/Controller/Admin/BlogController.php
Original file line number Diff line number Diff line change
Expand Up @@ -126,11 +126,8 @@ public function showAction(Post $post)
throw $this->createAccessDeniedException('Posts can only be shown to their authors.');
}

$deleteForm = $this->createDeleteForm($post);

return $this->render('admin/blog/show.html.twig', [
'post' => $post,
'delete_form' => $deleteForm->createView(),
]);
}

Expand All @@ -148,12 +145,11 @@ public function editAction(Post $post, Request $request)

$entityManager = $this->getDoctrine()->getManager();

$editForm = $this->createForm(PostType::class, $post);
$deleteForm = $this->createDeleteForm($post);
$form = $this->createForm(PostType::class, $post);

$editForm->handleRequest($request);
$form->handleRequest($request);

if ($editForm->isSubmitted() && $editForm->isValid()) {
if ($form->isSubmitted() && $form->isValid()) {
$post->setSlug($this->get('slugger')->slugify($post->getTitle()));
$entityManager->flush();

Expand All @@ -164,16 +160,15 @@ public function editAction(Post $post, Request $request)

return $this->render('admin/blog/edit.html.twig', [
'post' => $post,
'edit_form' => $editForm->createView(),
'delete_form' => $deleteForm->createView(),
'form' => $form->createView(),
]);
}

/**
* Deletes a Post entity.
*
* @Route("/{id}", name="admin_post_delete")
* @Method("DELETE")
* @Route("/{id}/delete", name="admin_post_delete")
* @Method("POST")
Copy link
Contributor

Choose a reason for hiding this comment

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

@lyrixx, you could use _method field in the form instead of changing the required HTTP method to POST. http://symfony.com/doc/current/form/action_method.html

Copy link
Member Author

Choose a reason for hiding this comment

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

I know, but I always found this totally useless ;)

Copy link
Member

Choose a reason for hiding this comment

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

Why don't you like the DELETE method? Is because browsers don't support it and we must simulate it with the _method trick? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

if you change it to POST (which is something I do all the time to avoid having to enable the HttpMethodOverride which is disabled by default), I suggest changing the URL to /{id}/delete though

Copy link
Member

Choose a reason for hiding this comment

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

So you don't use the DELETE method either in your apps? Why don't you want to enable HttpMethodOverride? Security reasons? Performance reasons? Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

@stof good catch, I will change the URL
@javiereguiluz It just simulate a verb. But what does it bring? IMHO nothing except more code for no real value.

Copy link
Member

Choose a reason for hiding this comment

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

@javiereguiluz enabling it make it easier to create CSRF attacks (well, not for this action as it has a CSRF token). This is why it is disabled by default in Symfony.

and it brings no value (the browser is not doing a DELETE request) and can become a nightmare when dealing with cache proxies (because it is a POST request, and you cannot add a Vary on this override to use the right cache.

* @Security("post.isAuthor(user)")
*
* The Security annotation value is an expression (if it evaluates to false,
Expand All @@ -182,40 +177,17 @@ public function editAction(Post $post, Request $request)
*/
public function deleteAction(Request $request, Post $post)
{
$form = $this->createDeleteForm($post);
$form->handleRequest($request);
if (!$this->isCsrfTokenValid('delete', $request->request->get('token'))) {
return $this->redirectToRoute('admin_post_index');
Copy link
Member

@yceruto yceruto Jan 10, 2017

Choose a reason for hiding this comment

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

The request could come from "edit page" or "show page", is right to redirect to index page? would be valid redirect to referer here?

In the other hand, could you add a flash messages (error) when this happens? Similar to:

The CSRF token is invalid. Please try to resubmit the form.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I keep the same behavior than before my changes...

}

if ($form->isSubmitted() && $form->isValid()) {
$entityManager = $this->getDoctrine()->getManager();
$entityManager = $this->getDoctrine()->getManager();

$entityManager->remove($post);
$entityManager->flush();
$entityManager->remove($post);
$entityManager->flush();

$this->addFlash('success', 'post.deleted_successfully');
}
$this->addFlash('success', 'post.deleted_successfully');

return $this->redirectToRoute('admin_post_index');
}

/**
* Creates a form to delete a Post entity by id.
*
* This is necessary because browsers don't support HTTP methods different
* from GET and POST. Since the controller that removes the blog posts expects
* a DELETE method, the trick is to create a simple form that *fakes* the
* HTTP DELETE method.
* See http://symfony.com/doc/current/cookbook/routing/method_parameters.html.
*
* @param Post $post The post object
*
* @return \Symfony\Component\Form\Form The form
*/
private function createDeleteForm(Post $post)
{
return $this->createFormBuilder()
->setAction($this->generateUrl('admin_post_delete', ['id' => $post->getId()]))
->setMethod('DELETE')
->getForm()
;
}
}