pallets-eco/flask-social

Browsers don't allow DELETE from regular forms

Closed this issue · 8 comments

pib commented

The examples and view code assume the use of the DELETE method to remove connections, but most (if not all) browsers don't allow DELETE as a method for regular forms.

pib commented

The fact that it doesn't work without the middle ware makes me think it should either do what the middleware does automatically or not require it.

It's kind of ugly to have to require all that when just using a regular POST would do the trick.

Why complicate things so much so a simple form post?

Ugly is subjective. FWIW, I don't think its ugly. A middleware is a very common pattern to support other HTTP verbs across browsers. You'll see it in almost all major web frameworks. Additionally, the HTTP API was designed with REST principles in mind. I believe following those principles keep us from introducing breaking changes if the future to a minimum.

pib commented

Alright, I can see your point on this one. I suppose the thing that surprised me most was that it was not mentioned at all in the documentation that something along the lines of the middleware was a requirement.

You can expect that there will be another pull request which will at the very least update the documentation and perhaps offer a built-in way to support the method rewrite without requiring the use of a middleware (or maybe just bundle a middleware implementation with flask-social?).

I think the documentations is sufficient, possibly with a link to the Flask documentation about it.

pib commented

The documentation has this in it:

    <form action="{{ url_for('social.remove_connection', provider_id=conn.provider_id, provider_user_id=conn.provider_user_id) }}" method="DELETE">
      <input type="submit" value="Disconnect {{ display_name }}" />
    </form>

That explicitly uses method="DELETE" which straight up doesn't work as written. It makes no mention of needing to install a middleware to convert a POST request to a DELETE request.

At the very least that needs to be updated to match the code (or at least link to it) in the example Matt linked to above.

Sorry, I meant that I think adding to the documentation is sufficient. Since most Flask apps use some sort of Middleware, offering a built-in solution could become messy.

pib commented

Ok, I see what you meant.

I think bundling a middleware that handles this would be handy since it is required for the extension to work properly. If a user wants to replace it with their own, they can easily do so, but for users who don't want to have to think about it, they can just wrap the middleware and be done with it. Also, even if they already have a middleware they are using, they can just stack this one in there as well.

Then again, the one in the example documentation is pretty simple, so I suppose copy-pasting it is not the worst thing ever.