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

ActionMailer https on URL with force_ssl = true #17388

Merged
merged 1 commit into from Oct 12, 2015

Conversation

Projects
None yet
9 participants
@akampjes
Contributor

akampjes commented Oct 25, 2014

config.force_ssl = true will set
config.action_mailer.default_url_options = { protocol: 'https' }

If you have turned on force_ssl, and then gone to the effort of setting
a config.action_mailer.default_url_options = {host: 'example.com'} then
you are probably pointing people back to your current app and want
https on that too.

@egilburg

View changes

actionmailer/lib/action_mailer/railtie.rb Outdated
@@ -16,6 +16,9 @@ class Railtie < Rails::Railtie # :nodoc:
paths = app.config.paths
options = app.config.action_mailer
options.default_url_options ||= {}
options.default_url_options.reverse_merge!(protocol: 'https') if app.config.force_ssl == true

This comment has been minimized.

@egilburg

egilburg Oct 25, 2014

Contributor

options.default_url_options[:protocol] ||= 'https' if app.config.force_ssl == true

also do you need to worry about symbol vs string as key?

@akampjes akampjes force-pushed the akampjes:master branch Oct 26, 2014

@akampjes

This comment has been minimized.

Contributor

akampjes commented Oct 26, 2014

Thanks for spotting that @egilburg I've just fixed that up.

@akampjes akampjes force-pushed the akampjes:master branch Oct 26, 2014

@robin850

View changes

actionmailer/lib/action_mailer/railtie.rb Outdated
@@ -16,6 +16,9 @@ class Railtie < Rails::Railtie # :nodoc:
paths = app.config.paths
options = app.config.action_mailer
options.default_url_options ||= {}
options.default_url_options.symbolize_keys!.reverse_merge!(protocol: 'https') if app.config.force_ssl == true

This comment has been minimized.

@robin850

robin850 Oct 26, 2014

Member

It looks like you didn't include the change suggested by Eugene, am I wrong ? Thanks for your patch though !

@akampjes

This comment has been minimized.

Contributor

akampjes commented Oct 26, 2014

Hi @robin850 , I've used symbolize_keys! to convert any string keys to symbols.

@robin850

This comment has been minimized.

Member

robin850 commented Oct 28, 2014

@akampjes : I mean Eugene wanted you to use ||= instead of reverse_merge since you're only passing a single value inside the given hash.

@akampjes

This comment has been minimized.

Contributor

akampjes commented Oct 28, 2014

Woops! How'd that get passed me 😕 :
Should be good now

@akampjes akampjes force-pushed the akampjes:master branch Oct 28, 2014

@seuros

View changes

actionmailer/lib/action_mailer/railtie.rb Outdated
@@ -16,6 +16,9 @@ class Railtie < Rails::Railtie # :nodoc:
paths = app.config.paths
options = app.config.action_mailer
options.default_url_options ||= {}
options.default_url_options.symbolize_keys![:protocol] ||= 'https' if app.config.force_ssl == true

This comment has been minimized.

@seuros

seuros Oct 28, 2014

Member

if app.config.force_ssl without the == true

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Oct 28, 2014

Member

Also we should not change the object in place here, we might be changing user's config this way. Make sure you wrap both lines in the if conditional.

@akampjes akampjes force-pushed the akampjes:master branch Oct 28, 2014

@akampjes

This comment has been minimized.

Contributor

akampjes commented Oct 28, 2014

@seuros Done.

@carlosantoniodasilva Okay, I think I've done what you are saying, but I've dropped support for strings in the hash (to keep the code short, am I right in doing that?). Every usage of url_options has always used symbols.

@tgxworld

View changes

actionmailer/lib/action_mailer/base.rb Outdated
@@ -130,6 +130,8 @@ module ActionMailer
# will generate relative URLs by default when a <tt>:host</tt> option isn't explicitly provided, passing
# <tt>only_path: false</tt> will ensure that absolute URLs are generated.
#
# By default when <tt>config.force_ssl</tt> is set, URLs generated for hosts will use the HTTPS protocol.

This comment has been minimized.

@tgxworld

tgxworld Oct 30, 2014

Contributor

I would add By default when <tt>config.force_ssl</tt> is set to true to avoid any ambiguity 😄

@akampjes akampjes force-pushed the akampjes:master branch 2 times, most recently Oct 30, 2014

@akampjes

This comment has been minimized.

Contributor

akampjes commented Nov 5, 2014

Any further feedback or can this be merged by someone now?

@andyjeffries

This comment has been minimized.

Contributor

andyjeffries commented Nov 13, 2014

+1 Like it :-)

@robin850 robin850 added this to the 5.0.0 milestone Nov 17, 2014

@akampjes akampjes force-pushed the akampjes:master branch Dec 14, 2014

@akampjes akampjes force-pushed the akampjes:master branch Jan 2, 2015

@akampjes akampjes force-pushed the akampjes:master branch Feb 2, 2015

ActionMailer https on URL with force_ssl = true
`config.force_ssl = true` will set
config.action_mailer.default_url_options = { protocol: 'https' }

If you have turned on force_ssl, and then gone to the effort of setting
config.action_mailer.default_url_options = {host: 'example.com'} then
you are probably pointing people back to your current app and want
https on that too.

@akampjes akampjes force-pushed the akampjes:master branch to f0a3af2 Aug 13, 2015

@akampjes

This comment has been minimized.

Contributor

akampjes commented Aug 13, 2015

:shipit: 👍

@arthurnn

This comment has been minimized.

Member

arthurnn commented Oct 12, 2015

LGTM. thanks

arthurnn pushed a commit that referenced this pull request Oct 12, 2015

Arthur Nogueira Neves
Merge pull request #17388 from akampjes/master
ActionMailer https on URL with force_ssl = true

@arthurnn arthurnn merged commit 9674703 into rails:master Oct 12, 2015

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015

@rafaelfranca rafaelfranca modified the milestones: 5.0.0, 5.0.0 [temp] Dec 30, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment