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

Flexmailer: Prevent broken urls containing hyphens when click tracking is enabled for plain text mailings #25149

Conversation

larssandergreen
Copy link
Contributor

Overview

Hyphens are allowed in URLs, but the regex for plain text mailings in Flexmailer did not include hyphens. URLs of the form example.org/wp-content/something were stored for click tracking as example.org/wp, breaking them.

Before

Hyphens not included in regex.

After

Hyphens included.

@civibot
Copy link

civibot bot commented Dec 9, 2022

(Standard links)

@civibot civibot bot added the master label Dec 9, 2022
@totten
Copy link
Member

totten commented Dec 10, 2022

Looks pretty sensible to me.

@seamuslee001
Copy link
Contributor

@larssandergreen would it be possible for you to add another case into here https://github.com/civicrm/civicrm-core/blob/master/ext/flexmailer/tests/phpunit/Civi/FlexMailer/ClickTracker/TextClickTrackerTest.php#L67 which covers the - to lock in this behaviour

@eileenmcnaughton
Copy link
Contributor

@larssandergreen I think we should target this patch against the 5.57 rc since we make flexmailer compulsory in that release

@larssandergreen larssandergreen force-pushed the allow-dashes-in-tracked-URLs-for-text-in-flexmailer branch from de6ee77 to ee55b52 Compare December 13, 2022 02:14
@larssandergreen larssandergreen force-pushed the allow-dashes-in-tracked-URLs-for-text-in-flexmailer branch from ee55b52 to 8b13a9f Compare December 13, 2022 02:36
@totten totten changed the base branch from master to 5.57 December 13, 2022 03:37
@civibot civibot bot added 5.57 and removed master labels Dec 13, 2022
@totten
Copy link
Member

totten commented Dec 13, 2022

Test looks good. To my eye, the current branch is based 5.57.alpha1, so it should apply on 5.57. Updated PR base-branch to match. MOP.

@yashodha
Copy link
Contributor

@larssandergreen @totten merging this. Thanks!

@yashodha yashodha merged commit 17ca2d1 into civicrm:5.57 Dec 13, 2022
@larssandergreen
Copy link
Contributor Author

larssandergreen commented Dec 13, 2022 via email

@eileenmcnaughton
Copy link
Contributor

Given we are making flexmailer required I have put up a port-to-stable for this #25166

@larssandergreen larssandergreen deleted the allow-dashes-in-tracked-URLs-for-text-in-flexmailer branch May 24, 2023 05:48
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.

5 participants