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

only add a closing quote if it is not already present #12182

Merged
merged 1 commit into from
May 22, 2018

Conversation

michaelmcandrew
Copy link
Contributor

Overview

CiviMail was adding extra closing quotes to some tokens

Before

On dmaster, create a new mailing with the following content:

<p><a href="https://example.org/communication-preferences?cid1={contact.contact_id}&amp;{contact.checksum}">Click here to select your communication preferences</a></p>

Send a test mailing

Note that the output is something like

<p><a href="https://example.org/communication-preferences?cid1=2&cs=cbfcef29ea6a8f4022b3d34e8259294d_1527000439_168&reset=1"">Click here to select your communication preferences</a></p>

This happens because the code assumes that the trailing quote is not present and that it needs to be added.

Some (it seems a minority) of email clients cannot handle the extra quote and the email is corrupted as a result.

After

Instead of assuming that the trailing quote is not present, we check to see if it is present and only add it if it is not present.

Comments

I did not want to mess with / try and understand why the code was gobbling up the trailing quote is some cases in the first place. TBH, I would have done the extra work normally but did not think it was worth it in this case seeings as we are moving to flexmailer TM (and this problem does not occur in flexmailer).

This fixes the problem in a minimally invasive way and meant that I did not have to try and understand the ins and outs of why the trailing quote is / is not being gobbled.

@colemanw
Copy link
Member

Ok, I tested this out on https://regex101.com/ and I'm satisfied the new regexes work.

@monishdeb
Copy link
Member

Merging as per tag

@monishdeb monishdeb merged commit 6747931 into civicrm:master May 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants