Skip to content
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

Always use the site_url for generating urls in emails #6206

Closed
wants to merge 1 commit into from

Conversation

jbransen
Copy link
Contributor

Q A
Bug fix? Y
New feature? N
Automated tests included? N
Related user documentation PR URL -
Related developer documentation PR URL -
Issues addressed (#s or URLs) -
BC breaks? ?
Deprecations? ?

Description:

This change makes sure that all absolute url's put in emails (unsubscribe/webview links, themes, etc.) are based in the site_url. With the old version of the code this is based on the hostname of the user visiting the Mautic interface, but that breaks when there are different hostnames pointing to the same instance (for example one on a local network)..

Steps to reproduce the bug:

  1. Add a local hostname to access Mautic
  2. Use that hostname to send an e-mail
  3. See the local hostname appear in the email source, not the public one set in the settings

Steps to test this PR:

see above, the url's are then fine

List deprecations along with the new alternative:

List backwards compatibility breaks:

@dbhurley dbhurley added bug Issues or PR's relating to bugs ready-to-test PR's that are ready to test labels Jun 13, 2018
@jbransen
Copy link
Contributor Author

Sorry, after submitting the pull request I found that I forgot some cases. Will try to make a clean merge request later this week.

@jbransen
Copy link
Contributor Author

This is now ready for review, test and merge

@npracht npracht modified the milestone: 2.14.2 Sep 5, 2018
@npracht
Copy link
Member

npracht commented Oct 2, 2018

@jbransen this is great !
Could you extend that to landing pages and forms ?

Copy link
Member

@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried on Mautibox https://mautibox.com/6206/email/preview/2 and urls in emails are wrong:

image

@kuzmany kuzmany added pending-feedback PR's and issues that are awaiting feedback from the author and removed ready-to-test PR's that are ready to test labels Oct 11, 2018
@escopecz escopecz modified the milestones: 2.14.2, 2.15.0 Oct 16, 2018
@kuzmany
Copy link
Member

kuzmany commented Oct 17, 2018

@jbransen
I think this PR resolve your issue too #6752
Can you review it please?

@jbransen
Copy link
Contributor Author

That does not resolve my issue as the use case is different. It will replace all fully qualified url's by the one with the other hostname, so also links in the private admin interface. My use case is that I have a private (VPN access only) hostname for doing the admin, and a public one for hosting images/unsubscribe pages/etc. So I would like only url's that are send to users to contain the 'public' one, not all of them.

Unfortunately it took so long to reply to the request that it now conflicts, and indeed I did not include all parts of the code. I may fix this when I have some time, but this will most likely not happen soon. Also, my PR has the same problem as #6752 which I just reviewed.

@kuzmany
Copy link
Member

kuzmany commented Oct 19, 2018

Close in favor #6752

@kuzmany kuzmany closed this Oct 19, 2018
@escopecz escopecz removed this from the 2.15.0 milestone Oct 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs pending-feedback PR's and issues that are awaiting feedback from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants