ActionController::ForceSSL accepts options for redirect_to #7570

Closed
wants to merge 2 commits into
from

Projects

None yet

6 participants

@deepak
Contributor
deepak commented Sep 8, 2012

force_ssl can accept options for redirect_to. like status for the http code to use for redirect

@deepak deepak ActionController::ForceSSL accepts options for redirect_to
force_ssl can accept options for redirect_to. like status for the http code to use for redirect
47cf659
Contributor
deepak commented Sep 8, 2012

also for calling force_ssl_redirect, host can be passed as either nil or :existing. They both mean the same but think :existing reads better

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Sep 8, 2012
actionpack/lib/action_controller/metal/force_ssl.rb
def force_ssl(options = {})
host = options.delete(:host)
before_filter(options) do
- force_ssl_redirect(host)
+ [:only, :except, :if, :unless].each {|x| options.delete(x) }
carlosantoniodasilva
carlosantoniodasilva Sep 8, 2012 Owner

This is likely change the before_filter options hash, I'd rather not delete the options but create a dup hash without the options here.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Sep 8, 2012
actionpack/lib/action_controller/metal/force_ssl.rb
@@ -37,10 +37,12 @@ module ClassMethods
# will be called only when it returns a true value.
# * <tt>unless</tt> - A symbol naming an instance method or a proc; the callback
# will be called only when it returns a false value.
+ # * <tt>status</tt> - The http status to to used for redirect, 302-307
@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Sep 8, 2012
actionpack/lib/action_controller/metal/force_ssl.rb
unless request.ssl?
- redirect_options = {:protocol => 'https://', :status => :moved_permanently}
- redirect_options.merge!(:host => host) if host
+ redirect_options.delete(:protocol)
+ redirect_options.delete(:params)
+ redirect_options = {:protocol => 'https://', :status => :moved_permanently}.merge(redirect_options)
carlosantoniodasilva
carlosantoniodasilva Sep 8, 2012 Owner

You can use merge!

@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Sep 8, 2012
actionpack/lib/action_controller/metal/force_ssl.rb
unless request.ssl?
- redirect_options = {:protocol => 'https://', :status => :moved_permanently}
- redirect_options.merge!(:host => host) if host
+ redirect_options.delete(:protocol)
+ redirect_options.delete(:params)
+ redirect_options = {:protocol => 'https://', :status => :moved_permanently}.merge(redirect_options)
+ redirect_options.merge!(:host => host) if host && host != :existing
carlosantoniodasilva
carlosantoniodasilva Sep 8, 2012 Owner

I don't think there's the need to add :existing to the current api, it already works and is well known by people that use it.

deepak
deepak Sep 9, 2012 Contributor

otherwise i will need to pass nil as the first option. does not seem semantic (and is not pretty).
as per this patch, we can pass nil or :existing

I have made a few comments. Do you have any real use case that required you to add such a feature? The status option may come in handy, but it's unlikely to be changed from the default.

Contributor
deepak commented Sep 9, 2012

Was trying to use SSL and non-SSL on webrick. That did not seem to be possible. so i ended up running two webrick servers, one to serve http and one to serve https.

And pass an option for port to force_ssl so that the redirect happens from the http port to the https port.

Also i did not like to do a permanent redirect. if possible i would like to do a temporary_redirect as this is my development box and even otherwise want the choice of redirect http code

PS: will revise code as per comments

Contributor
deepak commented Sep 9, 2012

please help. not able to add the test - https://gist.github.com/3682659

Member

What's the status of this?

Contributor

@carlosantoniodasilva Are you able to make a call on this? Should this be pulled in our punted completely? @deepak Do you still feel strongly that this should be added as a feature?

Contributor
deepak commented Mar 18, 2013

@mikepmunroe no. it is simple enough to write a patch

although i would like if i can pass a custom http status code to redirect_to
do not care much for custom host name

Contributor
JonRowe commented Apr 18, 2013

Is this still under active consideration? Or has it been abandoned to the point of being closed?

@pixeltrix pixeltrix was assigned Apr 25, 2013
@pixeltrix pixeltrix added a commit that closed this pull request Apr 25, 2013
@pixeltrix pixeltrix Add support for extra options to `force_ssl`
This commit adds support for passing additional url options along
with a :status option and any of the flash-related options to
`redirect_to` (i.e. :flash, :alert & :notice).

Closes #7570.
f99ce3c
@pixeltrix pixeltrix closed this in f99ce3c Apr 25, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment