dev/core#2836 install flexmailer by default on new installs [token]

[eileenmcnaughton]

Overview

dev/core#2836 install flexmailer by default on new installs

Before

Flexmailer not installed by default on new installs

After

Wow - look at that

Technical Details

Comments

https://lab.civicrm.org/dev/core/-/issues/2836

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[mlutfy]

Test errors seem to relate:

    api_v3_MailingTest.testMailerSubmit with data set #5
    api_v3_MailingTest.testMailerPreview
    api_v3_MailingTest.testMailerPreviewUnknownContact
    api_v3_MailingTest.testTrackableURLWithUnicodeSign

[eileenmcnaughton]

This one
api_v3_MailingTest.testTrackableURLWithUnicodeSign

seems to relate to something that works in normal mail but not flexmailer

[totten]

Rebased. api_v3_MailingTest.testTrackableURLWithUnicodeSign should pass this time... 🤞

[eileenmcnaughton]

#11316 (comment)

[eileenmcnaughton]

Stacktrace
api_v3_MailingTest::testMailerPreview
Failed asserting that '

Forward this emailForward this email with no protocol</p

This is Mr. Anthony Anderson II.

CiviCRM.org

http://core-21522-95seo.test-3.civicrm.org:8001/index.php?q=civicrm/mailing/optout&amp;amp;reset=1&amp;jid=&amp;qid=&amp;h=fakehash

\n

' contains "Forward this email with no protocol".

/home/jenkins/bknix-dfl/build/core-21522-95seo/web/sites/all/modules/civicrm/tests/phpunit/api/v3/MailingTest.php:295
/home/jenkins/bknix-dfl/build/core-21522-95seo/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php:260
/home/jenkins/bknix-dfl/extern/phpunit8/phpunit8.phar:671

[eileenmcnaughton]

	Test Result (3 failures / -4)api_v3_MailingTest.testMailerSubmit with data set #5api_v3_MailingTest.testMailerPreviewapi_v3_MailingTest.testMailerPreviewUnknownContact

[totten]

api_v3_MailingTest.testMailerPreviewUnknownContact

Right, following #21715, both testMailerPreview and testMailerPreviewUnknownContact need to be restored back to their original assertions. The prior revision restored one assertion. Force-pushed to restore the second assertion.

api_v3_MailingTest.testMailerSubmit with data set #5

Added a bit to relax this assertion. The error text is very slightly different (and imho is maybe a little more on-point in the new variant).

api_v3_MailingTest.testMailerPreview

This has some other thing going on with regard to mocking of contact-hashes... don't fully grok it yet...

[totten]

api_v3_MailingTest.testMailerPreview

This has some other thing going on with regard to mocking of contact-hashes... don't fully grok it yet...

OK, adding some new-lines to the example bits makes the issues a bit clearer:

== Template:
1: <link href='https://fonts.googleapis.com/css?family=Roboto+Condensed:400,700|Zilla+Slab:500,700' rel='stylesheet' type='text/css'>
2: <p><a href="http://{action.forward}">Forward this email</a>
3: <a href="{action.forward}">Forward this email with no protocol</a></p
4: <p>This is {contact.display_name}.</p>
5: <p><a href='https://civicrm.org/'>CiviCRM.org</a></p>
6: <p>{domain.address}{action.optOutUrl}</p>

== Actual:
1: <link href='https://fonts.googleapis.com/css?family=Roboto+Condensed:400,700|Zilla+Slab:500,700' rel='stylesheet' type='text/css'>
2: <p><a href="http://http://dmaster.127.0.0.1.nip.io:8001/index.php?q=civicrm/mailing/forward&amp;amp;reset=1&amp;jid=&amp;qid=&amp;h=fakehash">Forward this email</a>
3: <a href="http://dmaster.127.0.0.1.nip.io:8001/index.php?q=civicrm/mailing/forward&amp;amp;reset=1&amp;jid=&amp;qid=&amp;h=fakehash">Forward this email with no protocol</a></p
4: <p>This is Mr. Anthony Anderson II.</p>
5: <p><a href='https://civicrm.org/'>CiviCRM.org</a></p>
6: <p>http://dmaster.127.0.0.1.nip.io:8001/index.php?q=civicrm/mailing/optout&amp;amp;reset=1&amp;jid=&amp;qid=&amp;h=fakehash</p>
7: <img src="http://dmaster.127.0.0.1.nip.io:8001/index.php?q=civicrm/mailing/open&amp;qid=" width='1' height='1' alt='' border='0'>'

== Expect Substring:
<a href="http://dmaster.127.0.0.1.nip.io:8001/index.php?q=civicrm/mailing/forward&amp;amp;reset=1&amp;jid=&amp;qid=&amp;h=">Forward this email with no protocol</a>

== Export NOT Substring:
http://http://

A few things going on here:

• line 2: double http://: CiviMail BAO treats {action.forward} and http://{action.forward} as the same thing. IIRC, that's a convenient work-around for usability issues when editing HREFs in CKEditor. That's probably still an issue with CKEditor... so I guess that means we should extend the same forgiveness to TokenProcessor?
• line 3: 'h=fakehash' vs h= (empty-string): In preview mode, we don't get contact-hashes. But BAO and Flexmailer differ in what nonsense we do get (ie BAO's '' vs Flexmailer's 'fakehash'). I feel like it's easier to investigate with a string like 'fakehash', but if anyone thinks continuity is better, then it's easy to search/replace fakehash and make Flexmailer match.
• line 3 </p doesn't close correctly: I don't think this meant to test invalid HTML. We should fix it.

[eileenmcnaughton]

@totten so my general take is these things seems so trivial to the point where no-one seems to have noticed when switching to flexmailer so far - so I'm fine with just updating the tests

[totten]

@eileenmcnaughton I rebased, regenerated the SQL dump, and updated the MailingTests for preview. Tests are passing now.

For the http://{action.*} discrepancy...

• I didn't quite want to remove the tests for http://{action.forward} since that's still meaningful on the BAO-renderer. So I split off a narrower test and used some trickery to only apply it to BAO-renderer.
• We had discussed out-of-band and figured that this PR focuses on new sites which don't have the same continuity expectation. The problem is really salient on upgraded sites. Split off a Gitlab issue: https://lab.civicrm.org/dev/core/-/issues/2898

[eileenmcnaughton]

yay - thanks @totten