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

Use delete button instead of link url #391

Closed
eliasjpr opened this issue Nov 14, 2017 · 9 comments
Closed

Use delete button instead of link url #391

eliasjpr opened this issue Nov 14, 2017 · 9 comments

Comments

@eliasjpr
Copy link
Contributor

eliasjpr commented Nov 14, 2017

Reference: #356

Currently generated views uses

= link_to("delete", "/<%= @name %>s/#{<%= @name %>.id}?_method=delete&_csrf=#{csrf_token}", class: "btn btn-danger btn-xs", onclick: "return confirm('Are you sure?');")

This should use button tag instead, which generates a submit form and sends CSRF as a POST instead of a get. This will make more closely compliance with W3C RFC.

= link_to("delete", "/<%= @name %>s/#{<%= @name %>.id}?_method=delete&_csrf=#{csrf_token}", class: "btn btn-danger btn-xs", onclick: "return confirm('Are you sure?');")
@elorest
Copy link
Member

elorest commented Nov 17, 2017

Could you link to where the current implementation is out of compliance with W3C RFC. My understanding is that "Destructive actions shouldn't not be sent through GET requests unless appropriate precautions are taken."

@eliasjpr eliasjpr self-assigned this Nov 18, 2017
@eliasjpr
Copy link
Contributor Author

I opened this issue in consideration of your comment here, and the discussion that was taken in that issue.

Below find the highlighted text where the article stipulates that all GET request are intended for retrieval operations, while POST, DELETE, and PUT are related in the sense that these method represent some SIDE effect it is more natural to have the DELETE operation be performed in a POST, because their close relationship.

This seems to be the broader accepted convention and we can comply with this pretty easily thru the use of the enhanced button_to functionality recently added to Jasper::Helpers found here. We will continue to support the current implementation just, that generators will use the button_to. This way for those who prefer the current implementation can continue using it.

WDYT?

9.1.1 Safe Methods

Implementors should be aware that the software represents the user in their interactions over the Internet, and should be careful to allow the user to be aware of any actions they might take which may have an unexpected significance to themselves or others.

In particular, the convention has been established that the GET and HEAD methods SHOULD NOT have the significance of taking an action other than retrieval. These methods ought to be considered "safe". This allows user agents to represent other methods, such as POST, PUT and DELETE, in a special way, so that the user is made aware of the fact that a possibly unsafe action is being requested.

Naturally, it is not possible to ensure that the server does not generate side-effects as a result of performing a GET request; in fact, some dynamic resources consider that a feature. The important distinction here is that the user did not request the side-effects, so therefore cannot be held accountable for them.

@elorest
Copy link
Member

elorest commented Nov 18, 2017

Technically we are alerting users to the consequences of their actions, sending csrf, and changing method to delete. I prefer this to using a javascript hack like Rails UJS, or a inserting a form for a link.

However, the world seems to be completely in the camp of not using GET for delete anymore. So I concede we should change it.

I would like to suggest that we change it to some sort of syntax like link_to "delete", "posts/1", method: "delete" where link_to will generate a form and button, instead of button_to.

WDYT?

@drujensen
Copy link
Member

drujensen commented Nov 18, 2017

@elorest The meme is set and breaking it is an uphill battle. There is no W3C RFC regarding method override, but there is a meme that is hard to break. I concede that its not a battle worth fighting.

I do like your approach of using the link_to and doing a method override of a POST instead of a GET but I'm afraid someone will still report it as a W3C RFC violation.

@eliasjpr
Copy link
Contributor Author

eliasjpr commented Nov 18, 2017

Quite frankly, I don't think we are violating anything. According to what I have read DELETE is a request that will send no content this definition leaves anyone to assume that is ok via either GET or POST. This is a topic that definitely is not worth fighting we should continue to support both as a feature to those pound on the subject.

PUT and DELETE are no longer supported in HTML 5 forms and were in fact remove from the specs. However, GET, POST, PUT and DELETE are supported by the implementations of XMLHttpRequest (i.e. AJAX calls) in all the major web browsers (IE, Firefox, Safari, Chrome, Opera). Actually any other method other than GET and POST were not part of the original HTTP specification (HTTP/0.9, or I think HTTP/1.0) specs.

So we either implement the UJS library, which I believe we can just copy and/or link_to to output a form is a method: delete is provided. I honestly okay with either or both

@marksiemers marksiemers modified the milestones: Version TBD, Version 0.7.x Jan 5, 2018
@faustinoaq
Copy link
Contributor

I would like to suggest that we change it to some sort of syntax like link_to "delete", "posts/1", method: "delete" where link_to will generate a form and button, instead of button_to.

Some update about this 😅

@eliasjpr
Copy link
Contributor Author

eliasjpr commented Feb 9, 2018

@faustinoaq I like button_to to be semantically correct, link_to that generates a form is a little deceiving to me. People can weight in on this. I will go with the majority votes here

@eliasjpr
Copy link
Contributor Author

eliasjpr commented Apr 9, 2018

It seems that we can close this issue since this was somehow resolved using an alternative approach here #715 WDYT?

@robacarp
Copy link
Member

robacarp commented Apr 9, 2018

Agree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

6 participants