veda-consulting-company / uk.co.vedaconsulting.mosaico

Other
39 stars 75 forks source link

Links getting butchered #165

Closed freedomlives closed 6 years ago

freedomlives commented 7 years ago

I have two problems actually, that are inter-related.
First problem: When sending with "track click throughs" checked, links get changed such that the full internal server path is used, e.g.: http://www.example.net//var/www/example.net/htdocs/sites/all/modules/civicrm/extern/url.php?u=720&qid=1724440 Other problem: If I turn off click-through tracking, then the url for e.g. a file on the same server ends up like this: ../../images/poster.jpg even though the url I put is: https://www.example.net/images/poster.jpg This is happening to links made to text in text blocks that link to a file on the same server. If, on the other hand, I insert a button block, the link set for that works fine. Or if I make a link in the text that links to another website, it also works. This problem occurs inside the editor-- I can see while editing-- I make a link, click ok, then "edit link" and it has been changed.

dptarrant commented 7 years ago

I have several instances with this same problem. It is not a Mosaico issue specifically as my examples are not using Mosaico

Here is the Jira report https://issues.civicrm.org/jira/projects/CRM/issues/CRM-20245

bhahumanists commented 7 years ago

Testing on WordPress / 4.7.17

--I can see the links break in the editor - we enter them correctly, then when we edit them again we see the ../ issue. But for us the links do actually work in the resulting email. This happens whether we have clickthrough turned on or not. --However, some of the links in the underlying template (so not ones we enter at the design phase) are coming through as ../. So it's definitely kicking in at some point.

I'll keep testing.

freedomlives commented 7 years ago

For me, if I put the images and pdfs on to a different server, the links don't get butchered in the editor, and the tracking urls all work fine (my solution for now). To me the solution would be perhaps in making sure that the editor just leaves URLs alone-- even if they reference a file on the same server. I'm using Drupal. How do the links when sent from WordPress without track through end up looking in the final email?

freedomlives commented 7 years ago

It also could be something to do with the Drupal upgrade. I don't remember when I last updated, but it may have been just in 2016 for both Drupal and CiviCRM. Unless there is a security issue, I tend to wait to upgrade when there is some really interesting feature.

bhahumanists commented 7 years ago

In WP the links without trackthrough work ok, despite appearing incorrectly in the editor.

freedomlives commented 7 years ago

Well, I thought everything was working, so I went ahead and sent. First I ran in to this problem, which required adjusting up the php memory limit. PHP Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 56749 bytes) in /var/www/example.net/htdocs/sites/default/files/civicrm/ext/uk.co.vedaconsulting.mosaico/CRM/Mosaico/UrlFilter.php on line 53 But things were fine at first-- we have a 27K subscriber email list. From testing, my emails are in the database under a few different contacts, so I usually get an email right at the beginning and then later as it gets further through the list. The first email as fine, everything worked. The later email again got this problem again with putting the full internal server path behind the domain name. Really boggles me, and now we've got to figure it out and re-send the email with an apology and working links. :-(

bhahumanists commented 7 years ago

I've made some progress with this, but I'm not there yet. The links are getting converted by TinyMCE, of all things. Mosaico seems to use this for text and link handling.

All the settings for this look to be buried within minified js files, but it seems like we should be able to disable link conversion when TinyMCE is initialized by adding a "convert_urls: false" option (see the links below). I can't work out where the TinyMCE initialization is happening at the moment, though.

I imagine people better at JS than me will be able to work this out pretty quickly!

Helpful links: https://www.tinymce.com/docs/demo/url-conversion/ and https://www.tinymce.com/docs/get-started/get-support/

bhahumanists commented 7 years ago

Ok, got it. TinyMCE is initialised in mosaico.min.js using tinymce.init(g). Just above that is a list of options for g, including hidden_input, toolbar1, toolbar2 etc. If you add "convert_urls:false" to this list, the link butchering problem goes away (remember to hard refresh the cache). It's pretty hard to read as it's all minified.

I'm not sure of the best way to proceed on this, as this is a mosaico core file. I'd submit a tentative pull request, but there doesn't seem to be a packages folder in this repo atm.

freedomlives commented 7 years ago

Excellent work figuring that out! My boss/wife asked me how many opened after our last mailing, and that reminded me that I don't have a clue, because I had to turn off tracking!

bhahumanists commented 7 years ago

Thanks! I still can't replicate your tracking problem here, but maybe it's CMS-dependent. The above seems to stop mosaico rewriting the URLs in the live editor, at least.

bryancn commented 7 years ago

It might be worth noting that, for me at least, the links worked perfectly well in the test mailing and then failed when the mailing was submitted. Are they using different TinyMCE configurations? Can the test mailings share (more) common code so that problems are more likely to be picked up in advance?

JKingsnorth commented 7 years ago

Interestingly, when the mailing is actually sent any of the 'relative-ised' links ../../this/that get turned back into absolute links mydomain.com/this/that. This In 2.x this happens regardless of whether tracking is enabled or not. The PR above just stops the links going to the ../../ format in the first place, based on teh suggestion from @bhahumanists =]

totten commented 6 years ago

There were several comments indicating that this was fixed in #177.

Note: I haven't tested this myself, but I've updated the TESTING.md procedure so that we'll re-check this whenever there's a major change.