-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix regression mailer preview path #1388
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
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,11 +14,25 @@ def comment_lines(path, flag, *args) | |
|
||
initializer 'action_mailer.rb', <<-CODE | ||
if ENV['DEFAULT_URL'] | ||
Rails.application.configure do | ||
config.action_mailer.default_url_options = { :host => ENV['DEFAULT_URL'] } | ||
if ::Rails::VERSION::STRING < '4.1' | ||
ExampleApp::Application.configure do | ||
config.action_mailer.default_url_options = { :host => ENV['DEFAULT_URL'] } | ||
end | ||
else | ||
Rails.application.configure do | ||
config.action_mailer.default_url_options = { :host => ENV['DEFAULT_URL'] } | ||
end | ||
end | ||
end | ||
|
||
if defined?(ActionMailer) | ||
# This will force the loading of ActionMailer settings | ||
ActionMailer::Base.smtp_settings | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the same on all versions of Rails? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From what I can tell. |
||
end | ||
CODE | ||
gsub_file 'config/initializers/action_mailer.rb', | ||
/ExampleApp/, | ||
Rails.application.class.parent.to_s | ||
|
||
copy_file 'spec/support/default_preview_path' | ||
chmod 'spec/support/default_preview_path', 0755 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,19 +29,12 @@ class Railtie < ::Rails::Railtie | |
private | ||
|
||
def setup_preview_path(app) | ||
# If the action mailer railtie isn't loaded the config will not respond | ||
return unless supports_action_mailer_previews?(app.config) | ||
options = app.config.action_mailer | ||
config_default_preview_path(options) if config_preview_path?(options) | ||
end | ||
|
||
def config_preview_path?(options) | ||
# This string version check avoids loading the ActionMailer class, as | ||
# would happen using `defined?`. This is necessary because the | ||
# ActionMailer class only loads it's settings once, at load time. If we | ||
# load the class now any settings declared in a config block in an | ||
# initializer will be ignored. | ||
# | ||
# We cannot use `respond_to?(:show_previews)` here as it will always | ||
# return `true`. | ||
if ::Rails::VERSION::STRING < '4.2' | ||
|
@@ -59,8 +52,18 @@ def config_default_preview_path(options) | |
end | ||
|
||
def supports_action_mailer_previews?(config) | ||
config.respond_to?(:action_mailer) && | ||
config.action_mailer.respond_to?(:preview_path) | ||
# If the action mailer railtie isn't loaded the config will not | ||
# respond. | ||
# | ||
# This string version check avoids loading the ActionMailer class, as | ||
# would happen using `defined?`. This is necessary because the | ||
# ActionMailer class only loads it's settings once, at load time. If we | ||
# load the class now any settings declared in a config block in an | ||
# initializer will be ignored. | ||
# | ||
# We cannot use `config.action_mailer.respond_to?(:preview_path)` here | ||
# as it will always return `true`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rails is a ghetto |
||
config.respond_to?(:action_mailer) && ::Rails::VERSION::STRING > '4.1' | ||
end | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious why this change is related to this PR? Shouldn't we always have been doing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should always have been there. Rails changed how they provided access to the configuration in 4.1. Incidentally the mailer specs / script never executed with a code path testing a default url option on Rails < 4.1. Which is also partially related to why this issue wasn't noticed sooner.
After fixing the mailer specs to run better coverage for Rails < 4.1, reproducing the issue, more things started failing as this code path was actually run. Thus, a fix is included here. The commit 9522842 has a long explanation of several related things which got fixed in the spec suite.