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

Deprecate `*_path` methods in mailers #15840

Merged
merged 1 commit into from Jul 30, 2014

Conversation

Projects
None yet
@schneems
Member

schneems commented Jun 20, 2014

Email does not support relative links since there is no implicit host. Therefore all links inside of emails must be fully qualified URLs. All path helpers are now deprecated. When removed, the error will give early indication to developers to use *_url methods instead.

Currently if a developer uses a *_path helper, their tests and mail_view will not catch the mistake. The only way to see the error is by sending emails in production. Preventing sending out emails with non-working path's is the desired end goal of this PR.

Currently path helpers are mixed-in to controllers (the ActionMailer::Base acts as a controller). All *_url and *_path helpers are made available through the same module. This PR separates this behavior into two modules so we can extend the *_path methods to add a Deprecation to them. Once deprecated we can use this same area to raise a NoMethodError and add an informative message directing the developer to use *_url instead.

The module with warnings is only mixed in when a controller returns false from the newly added supports_relative_path?.

Paired @sgrif & @schneems

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Jun 20, 2014

Member

Is there a good place for an integration test with a mailer?

Member

sgrif commented Jun 20, 2014

Is there a good place for an integration test with a mailer?

@vipulnsward

This comment has been minimized.

Show comment
Hide comment
@vipulnsward

vipulnsward Jun 20, 2014

Member

I think deprecating the behaviour would cause some pain. I had mentioned people reuse views in emails.

Example: In one of my projects I used https://github.com/pokonski/public_activity, and render/reuse the same views from web/ update emails.

The problem is, developer is forced to either use multiple views in this case, add a hack, or start using *_url helpers in web views too, which might not be desirable in some cases.

Any way to overcome this?

Member

vipulnsward commented Jun 20, 2014

I think deprecating the behaviour would cause some pain. I had mentioned people reuse views in emails.

Example: In one of my projects I used https://github.com/pokonski/public_activity, and render/reuse the same views from web/ update emails.

The problem is, developer is forced to either use multiple views in this case, add a hack, or start using *_url helpers in web views too, which might not be desirable in some cases.

Any way to overcome this?

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jun 20, 2014

Member

@sgrif not sure about the best place. I added one here: https://github.com/rails/rails/pull/15840/files#diff-6cd2f2bc2d75ddac6c0d1e85983e983dR406 in mailer_previews_test

@vipulnsward are you on the rails-contributors mailing list? We've talked about this behavior a bit. Sharing views between emails and mailers should be fine, though right now if they're using _path in any of those views, those links won't work when rendered as an email. Is that their desired behavior?

Member

schneems commented Jun 20, 2014

@sgrif not sure about the best place. I added one here: https://github.com/rails/rails/pull/15840/files#diff-6cd2f2bc2d75ddac6c0d1e85983e983dR406 in mailer_previews_test

@vipulnsward are you on the rails-contributors mailing list? We've talked about this behavior a bit. Sharing views between emails and mailers should be fine, though right now if they're using _path in any of those views, those links won't work when rendered as an email. Is that their desired behavior?

Show outdated Hide outdated actionview/lib/action_view/rendering.rb
@@ -35,12 +35,13 @@ def process(*) #:nodoc:
module ClassMethods
def view_context_class
@view_context_class ||= begin
routes = respond_to?(:_routes) && _routes
include_path_helpers = self.supports_relative_path?

This comment has been minimized.

@sgrif

sgrif Jun 20, 2014

Member

I think we can drop the self here.

@sgrif

sgrif Jun 20, 2014

Member

I think we can drop the self here.

This comment has been minimized.

@schneems

schneems Jun 20, 2014

Member

done

@schneems
@vipulnsward

This comment has been minimized.

Show comment
Hide comment
@vipulnsward

vipulnsward Jun 20, 2014

Member

@schneems I missed the discussion. Will check.

Member

vipulnsward commented Jun 20, 2014

@schneems I missed the discussion. Will check.

@egilburg

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Jun 20, 2014

Contributor

Even if this doesn't have an implicit host, why does _path must be deprecated? The partial path should still work and it's up to user if and how they want to use it.

Contributor

egilburg commented Jun 20, 2014

Even if this doesn't have an implicit host, why does _path must be deprecated? The partial path should still work and it's up to user if and how they want to use it.

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jun 21, 2014

Member

The partial path should still work

How so? You'll send an email with an unclickable link. If you need this functionality you can use a URL helper and pass in only_path: true.

Member

schneems commented Jun 21, 2014

The partial path should still work

How so? You'll send an email with an unclickable link. If you need this functionality you can use a URL helper and pass in only_path: true.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Jun 21, 2014

Member

What about forcing :only_path to be false for all URL helpers in the context of a mailer view?

Member

pixeltrix commented Jun 21, 2014

What about forcing :only_path to be false for all URL helpers in the context of a mailer view?

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jun 24, 2014

Member

The *_path helpers are expected to return relative paths. Having them return the full path would fix the problem but break expectations.

Any comments or questions on implementation?

Member

schneems commented Jun 24, 2014

The *_path helpers are expected to return relative paths. Having them return the full path would fix the problem but break expectations.

Any comments or questions on implementation?

@guilleiguaran

This comment has been minimized.

Show comment
Hide comment
@guilleiguaran
Member

guilleiguaran commented Jun 28, 2014

👍

Show outdated Hide outdated actionpack/lib/action_dispatch/routing/route_set.rb
# Rails.application.routes.url_helpers.url_for(args)
@_routes = routes
class << self
delegate :url_for, :optimize_routes_generation?, :to => '@_routes'

This comment has been minimized.

@robin850

robin850 Jun 29, 2014

Member

Nitpick but could we use 1.9 hash syntax here please ? :-) Thanks for the patch so far! 👍

@robin850

robin850 Jun 29, 2014

Member

Nitpick but could we use 1.9 hash syntax here please ? :-) Thanks for the patch so far! 👍

This comment has been minimized.

@schneems

schneems Jul 8, 2014

Member

done, i also updated the hashes in define_named_route_methods for consistency

@schneems

schneems Jul 8, 2014

Member

done, i also updated the hashes in define_named_route_methods for consistency

@robin850 robin850 added this to the 4.2.0 milestone Jun 29, 2014

@andyjeffries

This comment has been minimized.

Show comment
Hide comment
@andyjeffries

andyjeffries Jul 9, 2014

Contributor

I agree with this PR, I think it's a non-obvious thing that newer Rails devs may get caught by and not understand why things aren't working for their end users.

Contributor

andyjeffries commented Jul 9, 2014

I agree with this PR, I think it's a non-obvious thing that newer Rails devs may get caught by and not understand why things aren't working for their end users.

@schuetzm

This comment has been minimized.

Show comment
Hide comment
@schuetzm

schuetzm Jul 9, 2014

Contributor

I think there should still be a way to get at the bare paths. For example:
<%= link_to login_url(return_to: user_settings_path) %>
But it's fine if the _path helpers would require an explicit only_path: true.

Contributor

schuetzm commented Jul 9, 2014

I think there should still be a way to get at the bare paths. For example:
<%= link_to login_url(return_to: user_settings_path) %>
But it's fine if the _path helpers would require an explicit only_path: true.

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Jul 16, 2014

Member

@schneems thanks for the patch, will try to review soon!

Member

fxn commented Jul 16, 2014

@schneems thanks for the patch, will try to review soon!

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Jul 24, 2014

Member

Hi @schneems!

I had a look at the patch. Looks good to me, I like the overall idea of separating URL and path modules.

Regarding the implementation I only have some minor remarks:

  • In principle I don't see a need for representing this support in a predicate like supports_relative_path?. I believe it is enough to leave everything as is today, except that the AM railtie would pass a hard-coded false flag. This is not something we want to be overridden and we have a railtie for each component anyway.
  • About terminology: "/posts" is an absolute path. There are some places where "relative paths" are mentioned. Indeed, since the context is so clear (naming is "url" vs just "path" everywhere), we could either refer to "path helpers", or "relative URLs", leaning probably on the former in my opinion.
  • What do you think about passing a flag to path_helpers_module instead of a lambda that gives you a message? In principle I'd prefer to simplify that and have the message just down in the code that issues the deprecation. I believe path_helpers_module can be responsible for the deprecation message.
  • In master define_named_route_methods has changed a little bit, there's now a constant that indicates whether you want a URL or a path. Since define_url_helper is the one that defines stuff in @module, I guess this patch could now branch depending on FULL/PATH instead of being passed the target module. (Off the top of my head, let's see if it looks good once written.)
  • The Action Mailer guide could mention this.
  • Let's register this in the CHANGELOG.

Let's iterate a little bit and done!

Member

fxn commented Jul 24, 2014

Hi @schneems!

I had a look at the patch. Looks good to me, I like the overall idea of separating URL and path modules.

Regarding the implementation I only have some minor remarks:

  • In principle I don't see a need for representing this support in a predicate like supports_relative_path?. I believe it is enough to leave everything as is today, except that the AM railtie would pass a hard-coded false flag. This is not something we want to be overridden and we have a railtie for each component anyway.
  • About terminology: "/posts" is an absolute path. There are some places where "relative paths" are mentioned. Indeed, since the context is so clear (naming is "url" vs just "path" everywhere), we could either refer to "path helpers", or "relative URLs", leaning probably on the former in my opinion.
  • What do you think about passing a flag to path_helpers_module instead of a lambda that gives you a message? In principle I'd prefer to simplify that and have the message just down in the code that issues the deprecation. I believe path_helpers_module can be responsible for the deprecation message.
  • In master define_named_route_methods has changed a little bit, there's now a constant that indicates whether you want a URL or a path. Since define_url_helper is the one that defines stuff in @module, I guess this patch could now branch depending on FULL/PATH instead of being passed the target module. (Off the top of my head, let's see if it looks good once written.)
  • The Action Mailer guide could mention this.
  • Let's register this in the CHANGELOG.

Let's iterate a little bit and done!

Show outdated Hide outdated actionpack/lib/action_dispatch/routing/route_set.rb
if include_path_helpers
path_helpers = routes.named_routes.path_helpers_module
else
warning_block = ->(name) {"The method `#{name}` cannot be used here as relative links are not supported. Use `#{name.sub(/_path$/, '_url')}` instead"}

This comment has been minimized.

@schuetzm

schuetzm Jul 25, 2014

Contributor

The message also needs to tell the user what to do when he indeed needs only the path. For example, xxx_url only_path: true (but please check if that actually works, didn't test it).

@schuetzm

schuetzm Jul 25, 2014

Contributor

The message also needs to tell the user what to do when he indeed needs only the path. For example, xxx_url only_path: true (but please check if that actually works, didn't test it).

This comment has been minimized.

@schneems

schneems Jul 25, 2014

Member

I'm 👎 on this suggestion. Error messages are to point out what is wrong, and tell of a fix. If this use case was as likely as the unintentional use of _path then I would agree, but here it's such a minority it will distract and confuse. The statement is factually correct, tells how to get rid of the error/deprecation and does not assume how it will be used. Pointing someone to a _url is still correct. The error message is not a complete substitute for docs.

The 1% of time people need this non working url, it's in the docs for _url helpers and is extremely google-able.

@schneems

schneems Jul 25, 2014

Member

I'm 👎 on this suggestion. Error messages are to point out what is wrong, and tell of a fix. If this use case was as likely as the unintentional use of _path then I would agree, but here it's such a minority it will distract and confuse. The statement is factually correct, tells how to get rid of the error/deprecation and does not assume how it will be used. Pointing someone to a _url is still correct. The error message is not a complete substitute for docs.

The 1% of time people need this non working url, it's in the docs for _url helpers and is extremely google-able.

This comment has been minimized.

@schuetzm

schuetzm Jul 25, 2014

Contributor

Ok, but does only_path actually still work? That's what I'm most concerned about.

@schuetzm

schuetzm Jul 25, 2014

Contributor

Ok, but does only_path actually still work? That's what I'm most concerned about.

This comment has been minimized.

@schneems

schneems Jul 25, 2014

Member

yes, this doesn't touch the implementation of *_url only *_path

@schneems

schneems Jul 25, 2014

Member

yes, this doesn't touch the implementation of *_url only *_path

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jul 25, 2014

Member

In principle I don't see a need for representing this support in a predicate like supports_relative_path?. I believe it is enough to leave everything as is today, except that the AM railtie would pass a hard-coded false flag. This is not something we want to be overridden and we have a railtie for each component anyway.

Moving this logic to the railtie is the only component i'm having problems with. The behavior is deeply nested inside of action view. We need to get this state information to https://github.com/rails/rails/blob/master/actionview/lib/action_view/rendering.rb#L43-L44 somehow. This is included in ActionMailer directly through ActionView::Layouts https://github.com/rails/rails/blob/master/actionmailer/lib/action_mailer/base.rb#L416

This seems like a likely place to add logic, but this doesn't affect path helpers in the view (not actually sure what it is doing): https://github.com/rails/rails/blob/master/actionmailer/lib/action_mailer/railtie.rb#L33

Do you have any ideas or thoughts on how to move this logic to the railtie? cc @sgrif

About terminology: "/posts" is an absolute path. There are some places where "relative paths" are mentioned. Indeed, since the context is so clear (naming is "url" vs just "path" everywhere), we could either refer to "path helpers", or "relative URLs", leaning probably on the former in my opinion.

Updated to proper naming

What do you think about passing a flag to path_helpers_module instead of a lambda that gives you a message? In principle I'd prefer to simplify that and have the message just down in the code that issues the deprecation. I believe path_helpers_module can be responsible for the deprecation message.

I'm fine with this, changed to a boolean

In master define_named_route_methods has changed a little bit, there's now a constant that indicates whether you want a URL or a path. Since define_url_helper is the one that defines stuff in @module, I guess this patch could now branch depending on FULL/PATH instead of being passed the target module. (Off the top of my head, let's see if it looks good once written.)

Updated to use this technique instead of module injection.

The Action Mailer guide could mention this.

Updated to mention this behavior

Let's register this in the CHANGELOG.

Done

Member

schneems commented Jul 25, 2014

In principle I don't see a need for representing this support in a predicate like supports_relative_path?. I believe it is enough to leave everything as is today, except that the AM railtie would pass a hard-coded false flag. This is not something we want to be overridden and we have a railtie for each component anyway.

Moving this logic to the railtie is the only component i'm having problems with. The behavior is deeply nested inside of action view. We need to get this state information to https://github.com/rails/rails/blob/master/actionview/lib/action_view/rendering.rb#L43-L44 somehow. This is included in ActionMailer directly through ActionView::Layouts https://github.com/rails/rails/blob/master/actionmailer/lib/action_mailer/base.rb#L416

This seems like a likely place to add logic, but this doesn't affect path helpers in the view (not actually sure what it is doing): https://github.com/rails/rails/blob/master/actionmailer/lib/action_mailer/railtie.rb#L33

Do you have any ideas or thoughts on how to move this logic to the railtie? cc @sgrif

About terminology: "/posts" is an absolute path. There are some places where "relative paths" are mentioned. Indeed, since the context is so clear (naming is "url" vs just "path" everywhere), we could either refer to "path helpers", or "relative URLs", leaning probably on the former in my opinion.

Updated to proper naming

What do you think about passing a flag to path_helpers_module instead of a lambda that gives you a message? In principle I'd prefer to simplify that and have the message just down in the code that issues the deprecation. I believe path_helpers_module can be responsible for the deprecation message.

I'm fine with this, changed to a boolean

In master define_named_route_methods has changed a little bit, there's now a constant that indicates whether you want a URL or a path. Since define_url_helper is the one that defines stuff in @module, I guess this patch could now branch depending on FULL/PATH instead of being passed the target module. (Off the top of my head, let's see if it looks good once written.)

Updated to use this technique instead of module injection.

The Action Mailer guide could mention this.

Updated to mention this behavior

Let's register this in the CHANGELOG.

Done

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Jul 30, 2014

Member

For the archives, we've discussed by email adding the same behavior to mailer instances (in addition to their templates).

@schneems I believe we are close. Would you mind doing a couple of last small edits?

  • In the CHANGELOG entry there's a double "instead" in the last sentence.
  • In the hunk for the guide, the link_to examples need a comma, and the last paragraph does not read well.

Could do that myself after pushing, but if you didn't mind the commit would be more round.

Member

fxn commented Jul 30, 2014

For the archives, we've discussed by email adding the same behavior to mailer instances (in addition to their templates).

@schneems I believe we are close. Would you mind doing a couple of last small edits?

  • In the CHANGELOG entry there's a double "instead" in the last sentence.
  • In the hunk for the guide, the link_to examples need a comma, and the last paragraph does not read well.

Could do that myself after pushing, but if you didn't mind the commit would be more round.

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Jul 30, 2014

Member

LGTM

Member

arthurnn commented Jul 30, 2014

LGTM

Deprecate `*_path` methods in mailers
Email does not support relative links since there is no implicit host. Therefore all links inside of emails must be fully qualified URLs. All path helpers are now deprecated. When removed, the error will give early indication to developers to use `*_url` methods instead.

Currently if a developer uses a `*_path` helper, their tests and `mail_view` will not catch the mistake. The only way to see the error is by sending emails in production. Preventing sending out emails with non-working path's is the desired end goal of this PR.

Currently path helpers are mixed-in to controllers (the ActionMailer::Base acts as a controller). All `*_url` and `*_path` helpers are made available through the same module. This PR separates this behavior into two modules so we can extend the `*_path` methods to add a Deprecation to them. Once deprecated we can use this same area to raise a NoMethodError and add an informative message directing the developer to use `*_url` instead.

The module with warnings is only mixed in when a controller returns false from the newly added `supports_relative_path?`.

Paired @sgrif & @schneems
@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jul 30, 2014

Member

There were a ton of merge conflicts introduced in the last 24 hours (https://github.com/rails/rails/commits/master/actionpack/lib/action_dispatch/routing/route_set.rb). I fixed those. Removed double "instead". Wrote a different last paragraph for the guide. Also had to modify a test that was asserting *_url methods cannot be used directly in mailers. Test introduced 4 years ago, I don't believe the behavior it was asserting was valid.

Looks like build is now green.

Member

schneems commented Jul 30, 2014

There were a ton of merge conflicts introduced in the last 24 hours (https://github.com/rails/rails/commits/master/actionpack/lib/action_dispatch/routing/route_set.rb). I fixed those. Removed double "instead". Wrote a different last paragraph for the guide. Also had to modify a test that was asserting *_url methods cannot be used directly in mailers. Test introduced 4 years ago, I don't believe the behavior it was asserting was valid.

Looks like build is now green.

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Jul 30, 2014

Member

Looking good!

@schneems you've devoted a lot of time to this PR, since the first discussions in the mailing list to the (not easy) patch and later revisions. That was awesome and really appreciated, thanks so much for working on this. ❤️

Member

fxn commented Jul 30, 2014

Looking good!

@schneems you've devoted a lot of time to this PR, since the first discussions in the mailing list to the (not easy) patch and later revisions. That was awesome and really appreciated, thanks so much for working on this. ❤️

fxn added a commit that referenced this pull request Jul 30, 2014

Merge pull request #15840 from schneems/schneems/deprecate-mailer_pat…
…h_methods

Deprecate `*_path` methods in mailers

@fxn fxn merged commit bd944e0 into rails:master Jul 30, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@calebthompson

This comment has been minimized.

Show comment
Hide comment
@calebthompson
Contributor

calebthompson commented Jul 30, 2014

I'm so excited!

seanlinsley added a commit to drapergem/draper that referenced this pull request Jul 4, 2016

seanlinsley added a commit to drapergem/draper that referenced this pull request Jul 4, 2016

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