DASPRiD / Dash

Flexible PSR-7 compliant HTTP router
BSD 2-Clause "Simplified" License
29 stars 9 forks source link

Default allowed HTTP method for route `Generic` should allow any HTTP method #13

Closed Ocramius closed 10 years ago

Ocramius commented 10 years ago

https://github.com/DASPRiD/Dash/blob/2271fdbfdb7a627f22cb8cb51223a6ec1a98aeeb/src/Dash/Mvc/Router/Http/Route/Generic.php#L35 should be *

DASPRiD commented 10 years ago

I had a short discussion about that yesterday with @Spabby and @macnibblet. Having * as default may only be useful for new-comers, but every other developer would hate it because they'd have to specify get for most of their routes explicitly then (really, most routes are GET only).

GeeH commented 10 years ago

We both disagreed with you :-1:

While I totally agreed this was a best practice change, in terms of the amount of support requests generated, this will be a nightmare -- people will not realise they have to define a route to accept POST when they submit forms to the same routes.

Summary - this is a great idea in theory, but unworkable in practice :tongue:

DASPRiD commented 10 years ago

Dash is not the only router doing this. I've seen many other frameworks doing exactly the same. You either have GET as default there, or you have to specify the method every time anyway, independent of what you want to do.

I'd argue that if this is documented properly, it would be a one-time hit per developer at most.

GeeH commented 10 years ago

As I say, if this were a brand new project I'd agree with you totally, but as people will be upgrading from zf2 router this will be a completely alien and unexpected behaviour. I'd argue it's better to leave matching open and allow "power" users to lock down the methods than vice versa.

DASPRiD commented 10 years ago

As I said, this will annoy the power users, since they have to explicitly write get every time. The users have to learn new configuration here anyway, so this is just a minor brick in the wall. Those staying with the old syntax and using the fallback module will not have any difference.

GeeH commented 10 years ago

I will happily concede this point if you will handle any and all confusion in #zftalk.

Thoughts @ocramius?

On Fri, Nov 22, 2013 at 12:04 PM, Ben Scholzen notifications@github.comwrote:

As I said, this will annoy the power users, since they have to explicitly write get every time. The user have to learn new configuration here anyway, so this is just a minor brick in the wall. Those staying with the old syntax and using the fallback module will not have any difference.

— Reply to this email directly or view it on GitHubhttps://github.com/DASPRiD/Dash/issues/13#issuecomment-29067838 .

manuakasam commented 10 years ago

I am with @DASPRiD on this one. Defaulting to ['get'] imo is a very SAFE way to do things. While yes, this may provide another BC-Bridge for users from ZF2, i feel like it's a good default. Allowing everything - at least to me - doesn't seem right.

It will result in lots of support requests, yes, but i still feel it is a good choice.

GeeH commented 10 years ago

It's a BC change for no good reason. It gains no performance, and 99% of people won't care which method you are requesting with.

On Fri, Nov 22, 2013 at 12:13 PM, Manuel St. notifications@github.comwrote:

I am with @DASPRiD https://github.com/DASPRiD on this one. Defaulting to ['get'] imo is a very SAFE way to do things. While yes, this may provide another BC-Bridge for users from ZF2, i feel like it's a good default. Allowing everything - at least to me - doesn't seem right.

It will result in lots of support requests, yes, but i still feel it is a good choice.

— Reply to this email directly or view it on GitHubhttps://github.com/DASPRiD/Dash/issues/13#issuecomment-29068268 .

tfountain commented 10 years ago

I agree with Spabby here, although I'd prefer if the default was to allow GET, POST and HEAD.

To give another use case, let's say I want to allow HEAD for all my routes (some tools such as link checkers use HEAD to check URL validity). With the current setup I'd have to specify the method for every route to achieve this?

Alternatively perhaps we could provide a way for users to override the default methods value for all routes?

DASPRiD commented 10 years ago

Mhhh… we currently aren't handling HEAD within the framework, are we? This would be something which had to be added to the framework then, I guess.

Allowing the user to set an application-wide default doesn't sane, because then you could potentially break third-party modules.

DASPRiD commented 10 years ago

Re-Opening for further discussion.

tfountain commented 10 years ago

Good point about breaking third party modules, so yes application wide default would be bad.

FYI HEAD does work in ZF2 (in that requests are successful and don't send a response body), I'm not quite sure what is handling that at the moment.

DASPRiD commented 10 years ago

Well, I'm wondering if ZF itself actually handles the HEAD request, or if the web server does.

tfountain commented 10 years ago

Looks like its the web server. I think the point still stands though - supporting HEAD on an app would require adding it as a method to every route, whilst in ZF2 this requires no action at all.

DASPRiD commented 10 years ago

Actually that point is not really valid then. If the web server handles the HEAD request completely by itself, it will never make its way through to the application.

Ocramius commented 10 years ago

I don't see any advantage (security-wise) in blocking http methods in routing config. routing and authorization are different problems, and introducing the restriction by default is very confusing, verbose and annoying imo.

DASPRiD commented 10 years ago

Valid HTTP methods do not have anything to do with authorization. On the other hand, it'd be verbose to always write get.

juriansluiman commented 10 years ago

I agree here there is no need to specify the route to accept only one given HTTP verb. Nofi, but setting a default HTTP method sounds just as stupid as setting a default route / to match a default controller Foo. Why setting a default?

If you want to specify it is a GET-only route, make it explicit. We just started zf2 by making things explicit, right? Why can't we continue that route (pun intended :p)? If you want to restrict the route, you can do it explicitly.

I agree with @Spabby here as it would be insanely difficult to provide support. Mind that ZF2 is already considered a difficult framework to learn, this will not make things any better. I really think we've to understand user's need from all type of expertise, and this is only addressing quite a specific need for the "expert" class imho. If we look at our neighbours (Symfony) they also match all given HTTP methods. Not saying this is the argument to support it, but it does show people might expect this behaviour; and complying to the given behaviour is in these cases a good thing.

Ocramius commented 10 years ago

sorry, I read the email thread, and it doesn't show edits. By the way, most (if not all) of the times, http method routing is used to disambiguate once a route was coded. Power users can still do that via config for the rare cases where this occurs

DASPRiD commented 10 years ago

@juriansluiman You are quite right with that argument. The generic route right now limits nothing at all. This means without any configuration, it will match anything, except the method right now. Thus we could argue that having no method matching by default would be valid, yes.

DASPRiD commented 10 years ago

As we discussed this in parallel on IRC, here's the current conclusion:

We will set * as the default, which will allow any kind of method to match. Additionally the following behaviour will be added, as suggested by @Bittarman:

When the user sets GET as allowed method, HEAD will be automatically enabled as well, and vise versa. The reason for this is that those two verbs are implicitly linked. Anything GETtable should be HEADable, and the other way around.

manuakasam commented 10 years ago

:+1: