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

Add note about upgrading custom routes from put to patch. #10747

Merged
merged 1 commit into from
Jun 3, 2013
Merged

Add note about upgrading custom routes from put to patch. #10747

merged 1 commit into from
Jun 3, 2013

Conversation

javan
Copy link
Contributor

@javan javan commented May 24, 2013

I assume this is expected behavior and not a bug?

@carlosantoniodasilva
Copy link
Member

Hm I'm afraid this wasn't intentional, it should be accepting both put and patch verbs interchangeably. At least that was the original idea of introducing patch as far as I know, so we may need to review it to see if we aren't missing some place that needs fixing. Thanks!

@steveklabnik
Copy link
Member

Yeah, this feels like an accident.

@javan
Copy link
Contributor Author

javan commented May 24, 2013

Aha! I suspect this was introduced somewhere in between 4.0.0.beta1 and 4.0.0.rc1. At least that's when we noticed it in Basecamp.

@trevorturk
Copy link
Contributor

I'm digging into this a bit more...

I don't think it was caused by the 4.0.0.beta1 -> 4.0.0.rc1 upgrade, I think we just didn't notice the bug right away.

I think the change came from this commit: b7a0945 /cc @fxn

I made a test app here to isolate the commit: https://github.com/trevorturk/rails-issue-patch

According to this blog post we want to continue supporting PUT routes: http://weblog.rubyonrails.org/2012/2/25/edge-rails-patch-is-the-new-primary-http-method-for-updates/

Perhaps this should be added to the Rails 4.0.0 milestone? It's a potential blocker for release. Perhaps it's worth considering making this a breaking change in the upgrade from 3.x?

I'll dig some more to see if I can fix the issue, but I'm not 100% sure how to proceed. If we see a PUT route with no conflicting PATCH route, should we make one? (I haven't thought through all of the potential areas of conflict yet.)

@trevorturk
Copy link
Contributor

Another option would be to undo some of b7a0945. We could set default_method_for_update to put by default, but generate new apps with it set to patch to make the change opt-in for existing apps and set by default for new apps like ye olde new_rails_defaults did.

@trevorturk
Copy link
Contributor

If we decide to leave things as-is, another possible note for the upgrade guide would be suggesting an alternative workaround. You can force form_for to use put instead of patch if, for example, you didn't want to upgrade your routes to avoid breaking API clients:

# Rails 3
<%= form_for [ :update_name, @user ] do |f| %>

# Rails 4
<%= form_for [ :update_name, @user ], method: :put do |f| %>

@dhh
Copy link
Member

dhh commented Jun 3, 2013

We've gone with PATCH, so I think we should stick to it and document the issue. Applying this. Please add further doc changes if necessary to make this clear.

dhh added a commit that referenced this pull request Jun 3, 2013
Add note about upgrading custom routes from `put` to `patch`.
@dhh dhh merged commit 0512286 into rails:master Jun 3, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants