Skip to content

Routing enhancements #20

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

Closed
wants to merge 19 commits into from
Closed

Conversation

axos88
Copy link

@axos88 axos88 commented Mar 7, 2014

Please review my changes carefully. Will recommend reviewing commit by commit, and using ?w=1 parameter to avoid seeing indentation changes as additions/removals.

!!As I could not get master running correctly on my machine, I worked on top of 1.7.5.
Will probably need some merging and conflict resolution, sorry!.

My pull request:

  • fixes routing errors (a get or post route would accept any http verb, even invalid ones, not only the get and post). Same for match
  • added support for put, patch, delete routes and http verbs
  • when adding a rule both /rule and /rule/ was added to the routing list, removed that doubling and now appending the '/' when the incoming requests don't have it.
  • fixes a debug output message
  • now returning 404 instead of 400 in some cases when there is no actual route match.
    • Rails-like support for put, patch, delete. Browsers are incapable of sending anything other than get and post. Rails overcame this shortcoming by reserving the "method" parameter, and when present overriding the incoming verb with it.

Todo:
update documentation about support for put, patch, delete

@axos88
Copy link
Author

axos88 commented Mar 7, 2014

Now that I've got the generators up and running in master, I'll probably rebase this and update.

@axos88
Copy link
Author

axos88 commented Mar 7, 2014

Should be rebased and mergebale.

@axos88
Copy link
Author

axos88 commented Mar 7, 2014

Since the optional _method parameter is now implemented, the docs definately need to be updated.

POST-ing to any url having a "_method" query parameter will override the http verb.

So making a POST to /blog/3?_method=DELETE will invoke the DELETE route rather than the POST route.

@axos88
Copy link
Author

axos88 commented Mar 7, 2014

My work in this PR is now done.

@treefrogframework
Copy link
Owner

Please tell the specification you propose.
Support PUT, PATCH and DELETE method?
Show a example of routes.cfg.

@axos88
Copy link
Author

axos88 commented Mar 9, 2014

Hi!

Sorry for that, it wasn't clear that the current specification allows for empty parameters.
Is there any documentation on how to run the automated regression testing?

Yes, it's supporting put, patch, delete methods like this:

get '/test' "Static#get"
post '/test' "Static#post"
put '/test' "Static#put"
patch '/test' "Static#patch"
delete '/test' "Static#delete"

So a PUT method to /test will route to the put method of the static controller.
But a POST method to /test?_method=PUT will also route to the put method.
A POST to /test?_method=DELETE will route to the delete.
This is to overcome the shortcoming of web browsers.

I'd love to chat with you about a few future enhancemants I have in mind to better support REST-like APIs, and some other stuff.
Is there any mailing list or any sort of thing that we could do that?

@axos88
Copy link
Author

axos88 commented Mar 18, 2014

Just added automated tests for URL routing. That should clear up what I was trying to achieve.

Akos Vandra added 19 commits March 18, 2014 10:47
…GET request is made to a static resource or a request is made to a non-existent resource 404 is returned instead of 400
…efaulting to trailing slash and if a request comes in without it, we just append it before searching for the routes
…,PATCH,DELETE for browsers incapable of sending them
…d a warning if the number of supplied parameters is not exactly the same as expected
This was referenced Mar 18, 2014
@axos88
Copy link
Author

axos88 commented Mar 18, 2014

Rebased changes onto origin/master.

@adderly
Copy link

adderly commented May 27, 2014

+1

@treefrogframework
Copy link
Owner

REST support was implemented.
Up to e1bbba4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants