Feature Request: Remove or replace *_path helpers for mailers

Here’s a change in behavior I would love on the mailers.

Backstory

When you send an email, you’ll likely need links. Those links must be the full path i.e. ‘http://example.com/foo’ instead of relative (just ‘/foo’). Unfortunately most devs are so used to using the *_path helpers, they use them in emails by mistake.

I made this mistake recently with http://www.codetriage.com, here’s the PR that fixes it (https://github.com/codetriage/codetriage/pull/257). The sad part is I didn’t realize there was a problem until a user got an email and couldn’t click on a link. Then they had to be nice enough to report it. If your business is running on Rails, this may take days. I’ve been doing this for years, and it still happens to me.

Feature

Here’s what I would like to see happen. Either raise an exception when *_path helpers are used in a mail template so my tests would have caught it, or likely just have *_path helpers resolve to the full URL by default. What do you think?

Totally in support of this as well. I've probably made this mistake more often
than I've done it correctly, and it's so easy to miss.

I've seen email templates go months without this being caught.

I'd probably prefer the exception route myself, just by undefining the methods
for ActionMailer.

Caleb

I have been bitten by this too, and agree with it.

I think resolving a full URL would be better. People sometimes reuse
normal views from Mailers, and would be better off using *_path
helpers there.

Exceptions would cause an issue. (A warning maybe?)
Vipul A.M.
+91-8149-204995

I’m in support of this as well.

As a solution i would prefer to resolve the full URL by default. Raising an exception seems to me a bit like rails telling the programmer „I know what you’re intending, and we both know the solution but you have to fix it on your own."

Agree with the feature request. I made this error too.

I’m in support of this as well.

As a solution i would prefer to resolve the full URL by default. Raising
an exception seems to me a bit like rails telling the programmer „I know
what you’re intending, and we both know the solution but you have to fix it
on your own."

Breaking the strong expectation that *_path generates a path is dubious to
me. Too clever, the API is kind of cheating in my mind.

I've been thinking about it and my only conclusion by now is that we agree
there's something to do here, but see cons in all proposals so far :).

Are there any valid uses for *_path in an email? Can you do reference links with ID elements in an email? We could make it configurable. Default it to non-hair-pulling behavior, but allow anyone with a legitimate *_path use to preserve functionality.


config.action_mailer.path_helper_behavior = :raise

config.action_mailer.path_helper_behavior = :url

config.action_mailer.path_helper_behavior = :make_me_lose_users_or_money # i.e. current behavior

Slight aside, but wouldn’t a simple test case catch such an issue? Whether that be an actual mailer test or reviewing a generated email?

Nope it wouldn’t. Rendering the emails don’t currently raise errors, so your tests will still pass. The only way to find out is to actually get the email and click on it. Even if you look at it in mail_view you get the right URL generated as the relative path will work.

Right now it’s all just :frowning:

I cannot think of a valid use case for *_path in mailer views, so after
thinking about it I personally would be inclined to remove them.

People cannot upgrade and have their mailers crashing in production, so I
think we have no option but to go through a deprecation cycle.

If someone had a rare need to generate a path and the helpers were gone,
they could still opt-in via *_url(only_path: true), which is something we
could document in small font :).

The mailer guide would need to reflect these changes.

What do you think?

I like that. Let’s deprecate *_path in Mailer in 4.2 and remove in 5.0

-Prem

Nice idea in my opinion.

If we go down this route, the error message should be crystal clear:

“The method user_path cannot be used in a mailer as relative links in emails do not work. Use user_url instead”

Also need to make sure that it raises when rendered with mail_view.

If we go down this route, the error message should be crystal clear:

"The method `user_path` cannot be used in a mailer as relative links in
emails do not work. Use `user_url` instead"

Also need to make sure that it raises when rendered with mail_view.

That'd be really awesome.

+1 from my side, never seen a use case for *_path on mailers.

For HTML emails, using a <base> tag allows you to use relative URLs safely.
It's easier to specify the base path in a single place and and use
relative URLs, as opposed to using an absolute path with the same base path
in every link (which is not very DRY).

Unfortunately, such an approach does not work for plain text email, so it
makes sense to deprecate them in the plain text case (if it is possible to
do so only in that case, but allow it for HTML email).

Unfortunately, for mail_view, I don't think the <base> tag approach will
work, unless you used a new window/frame for the HTML email.

Jeremy

I have never checked it myself (could be cargo culting), but my
understanding has always been that using the BASE tag is not a good
practice for portable HTML emails. See for example:

    http://stackoverflow.com/questions/14611225/html-base-tag-in-email

I personally believe preventing *_path calls that are plain wrong and a
common gotcha outweights supporting this edge-case that can still be
accomplished with only_path: true.

Here’s the working PR for those interested in following along: https://github.com/rails/rails/pull/15840

Merged. Thanks a lot Richard! <3

Thanks for the review and thanks to Sean, he did most of the driving on the initial pair. Happy to have this in :smiley: