CRM-21472 - Allow FlexMailer to take over token validation

[totten]

Overview

By default, CiviMail checks for tokens (such as {action.unsubscribeUrl}) whose presence would help address anti-spam regulations.

FlexMailer (generally) and Mosaico (in particular) allow customization of the email template language. But tokens look different in those environments, which leads the validation to malfunction. (eg veda-consulting-company/uk.co.vedaconsulting.mosaico#143 )

The logic for checking tokens should be hookable/alterable so that different template handlers can enforce different variants on this logic.

In keeping with the 'leap-by-extension', the refactoring/enhancement is primarily done in an extension. However, it requires a few small, conservative entry points.

Before

Token validation is entirely hard-coded.

After

Token validation can overloaded by FlexMailer. FlexMailer provides APIs/documentation enabling extensions to weigh-in on the token-validation.

Technical Details

Documentation and utilities can be reviewed in civicrm/org.civicrm.flexmailer#9

The existing behaviors remain unchanged if you have a vanilla (non-FlexMailer) deployment.

---

• CRM-21472: Make CiviMail token validation extensible

[eileenmcnaughton]

At the risk of giving this PR the kiss of death by commenting .... I have some concerns about this.

This doesn't seem to provide any path to deprecate the code that we are trying to grandfather out, or any entry / option for other extensions to engage in a meaningful way. I expect the LeXIM approach to involve figuring out how to make it so 'any extension' can hook into core - rather than hacking it for specific ones.

[mattwire]

I agree with @eileenmcnaughton here. The changes you are proposing seem quite hacky and would (in my opinion) be better implemented as hooks that can be used by any extension.

How about:

1. Create a new api function mailing.getrequiredtokens which calls CRM_Utils_Token::getRequiredTokens()
2. Add a hook to CRM_Utils_Token::getRequiredTokens() which allows overriding/replacing what that function returns.
3. CRM_Mailing_BAO_Mailing::create() Add a hook to alter params passed in? This one is a little bit harder.. and it might be nice to be able to hook/override checkSendable directly. Perhaps even by an overrideable class and base method?

[seamuslee001]

I think one of the other things to remember also is that flexmailer is using pretty much all symphony events for its stuff which may explain the hack but i agree with Eileen and Matt that i think it probably needs to be done i a better format such that its not just a hack but something other extensions can build onto

[eileenmcnaughton]

re the symphony events - I tried using those in an extension last week - but there is no clear statement in the documentation as to whether using them from outside of core is supported - the docs implied that they were for internal purposes. I realise that is an aside - but I also think it's important to clarify what our 'api' is

[totten]

Just to be clear, there is an optics problem on this PR -- the PR by itself looks hard-code-y. But the reality is that it's small piece in a bigger picture, and the bigger picture is more extensibility, more documentation, more test coverage.

Docs for FlexMailer APIs are generally published in prettier format at https://docs.civicrm.org/flexmailer/en/latest/ . If you haven't read that in the past couple weeks, please do. Questions/comments/suggestions towards improving those are welcome. (If you want to talk about the coding style of events and hooks, please check out civicrm/org.civicrm.flexmailer#7 . If you walk away from that and still feel some kind of attachment to "hook", then let's continue that conversation on PR 7.)

For this issue of token validation, the main PR is civicrm/org.civicrm.flexmailer#9 . That's where we add new interfaces, new test coverage, and new docs. This PR 11316 is just the ugly thunk that allows that to happen without exposing existing users on the stable version to any of the design or implementation risks. This is absolutely a project aimed at letting any extension (esp Mosaico, which has a funky bug) tap into token-validation.

Keep in mind: The aim here is to cleanup the CiviMail engine and expose more interfaces -- while doing more "LEx" and less "IM". The path to phasing-out the old code was planned as:
* Refactor code for CiviMail engine. Improve extensibility.
* Put the new code in an extension, where (a) it doesn't pose risk to existing installations or customizations and (b) it can deployed on alpha/beta basis and (c) it can be changed more quickly if we encounter issues.
* Stabilize.
* As adoption/stabilization improves, bundle with core.
* After it's bundled with core for a while, delete the old code.

[mattwire]

@totten I did actually look at the changes in org.civicrm.flexmailer and they do look clean, well thought-out and well documented. It's just these bits going into core that are really "hacky".
If you can throw a load of comments round the core changes explaining why they are there, and that they should not be used by other extensions/that they will change in the future then I think it could be merged, so it doesn't hold up the bigger picture.

[totten]

@mattwire Good point about comments. I've added more.

[eileenmcnaughton]

I guess what would make me feel better is if the classes & functions that will eventually be replaced by flexmailier are marked as deprecated (with reference to flexmailer). As it stands I have no idea when I see a PR to review if the code is actually on it's way out and I want to be able to simply say 'switch to flexmailer, no more patches accepted on this code'

[totten]

Adding those notes is a good idea. I'll try to do a bit of that in the next few days.

Did a quick test, and it seems that our phpcs rules (civilint) accept notations like @deprecated <message>, so we can have an explanatory note about why it's deprecated.

[totten]

@eileenmcnaughton I've added deprecation notices to a bunch of methods. (This is split between two commits - one for methods made redundant originally, and one for the changes in CRM-21472.)

[eileenmcnaughton]

I'm feeling disappointed to see so few functions tagged :-(

However, I think this is good to go now.

[eileenmcnaughton]

As an aside we should probably have a protocol around rejecting changes to deprecated code

[eileenmcnaughton]

unrelated fail