Fix errors when smarty is enabled in settings

[jitendrapurohit]

Smarty error is shown on viewing a mailing which includes css in html. This only gets triggered when CIVICRM_MAIL_SMARTY is enabled in settings. To reproduce -

• Create a mailing using versafix-1 template.
• Submit it and view the mailing page.

---

Issue raised in #233

[deepak-srivastava]

@totten looks fine to me. This is how we tackled it in v1. Or you got some other ideas?

[mattwire]

@totten Any thoughts on this - it looks like a straightforward bugfix and would be good in the main release

[totten]

Short answer: no, patch civicrm-core and/or flexmailer instead.

Long answer, primary critique: One of the original observations leading to Flexmailer is that CIVICRM_SMARTY is problematic and we need a path to migrate away from it. The basic idea ("let's use a real template language for mailings") is good, but the contract is subtly broken (it chains together two rich but different templating languages in a way that is hard to secure and impossible to optimize). Fixing the issues with CIVICRM_SMARTY requires a major compatibility break with existing templates/user-data, so you can't easily patch it. The aim with template_type is that each template_type has its own clean/safe space -- free from CIVICRM_SMARTY (et al). This allows you to transition away (e.g. some old content can continue using template_type=traditional and CIVICRM_SMARTY; new content can use other processes; and the two can co-exist).

In the case of Mosaico (template_type=mosaico), the clean/safe space where we can specify the template handling logic is CRM_Mosaico_MosaicoComposer. That class borrows some functions from traditional mailings, but it specifically disables Smarty processing. Smarty should have zero involvement with Mosaico mailings. It's one of the main differences between template_type=traditional and template_type=mosaico.

To my eye, the error reported at the top indicates a problem with the "Find Mailings" UI (or other core UI). The resolution -- figure out why that screen is using Smarty. In the presence of Flexmailer/Mosaico, it shouldn't be using Smarty. it shouldn't be hard-coded to use the traditional composition process. If there's any rendering to be done, it should be delegating that work. The listeners (like CRM_Mosaico_MosaicoComposer) should be responsible for any template-rendering.

Long answer, tangential critique: Adding function mosaico_civicrm_alterMailContent() in this context is a code-smell. We already have _mosaico_civicrm_alterMailContent(). From where I'm sitting, I haven't definitively traced it to a problem, but any of these seem possible:

1. Overly broad. The patch will apply to all mailings -- whether for mosaico, traditional, Smarty, or something else. That's overreach -- e.g. if there were an extension for writing Mustache templates, no one would expect code in the Mosaico extension to change the behavior of the Mustache templates.

2. Simple lack of awareness (i.e. didn't notice that _mosaico_civicrm_alterMailContent() existed).

3. Inconsistent contract. If some use-case works only when patching mosaico_civicrm_alterMailContent() and not when patching _mosaico_civicrm_alterMailContent(), then that's a problem because it means that _mosaico_civicrm_alterMailContent() is not getting called. The use-case is going to miss out on other important bits of logic (like token-aliasing). We should only define this logic once.

I include the tangential critique in the interest of generally sharing perspective. However, I'm -1 on this approach because of the primary critique. Addressing that should fix the symptom for Mosaico users and also provide an architecture where we can incrementally get away from mistakes of the past.

[jitendrapurohit]

Thanks for the detailed review @totten

To my eye, the error reported at the top indicates a problem with the "Find Mailings" UI (or other core UI)....

Mosaico templates adds a web version link("View in your browser") when it is set to show pre-header. It navigates to civicrm/mailing/view?id=<mailing_id>&reset=1 which is completely managed by CiviMail and doesn't consider CRM_Mosaico_MosaicoComposer to render its content.

This page is set to public, hence there is no error displayed on this link. The Smarty error shows up when any core UI is loaded after mailing is viewed.

Thought either we need to override this link and call mailing.preview API(uses mosaico composer) to render its content OR add a new alterMailContent hook to adjust the smarty enabled settings. Anyway closing this PR as I think the former should be the one to proceed. Thanks :-)

[totten]

... call mailing.preview API(uses mosaico composer) ...

Ahh, that smells like a good idea! This way we can use existing interfaces.

... override this link (civicrm/mailing/view?id=<mailing_id>&reset=1) ....

That sounds fine. Just thinking/digesting out loud a bit... it seems reasonable to either:

• (a) Update flexmailer to use hook_civicrm_alterMenu and override civicrm/mailing/view. The updated version would use mailing.preview API.
• (b) Update civicrm-core to change the mechanics of civicrm/mailing/view. The updated version would use mailing.preview API.

I initially thought to update civicrm-core (e.g. (b)), but you're probably right about overriding (e.g. (a)). If there's any risk in the new implementation of civicrm/mailing/view, then it's safer to put those in the Flexmailer alpha/beta -- where potential bugs would impact the self-appointed guinea-pigs. It also has the side-benefit of being deployable sooner...

[jitendrapurohit]

okay, proceeded with (b) and created a PR at civicrm/org.civicrm.flexmailer#21. For (a), maybe, this part in core needs to be replaced by the similar API call.

[TomCrawshaw]

OK, I understand that Smarty logic can't be used with Mosaico, but not being able to use conditional text in emails is a real issue, and limits the use of Mosaico for us to a few basic PR emails. I've found some hints that Mosaico has a "low-level template language" (https://github.com/voidlabs/mosaico/wiki/Template-language), but with no usable documentation this is not a replacement for the (relatively) simple Smarty approach. Please has anyone got any hints about how to include text conditional on a CiviCRM field or token?

[MegaphoneJon]

@TomCrawshaw https://civicrm.org/blog/colemanw/create-your-own-tokens-for-fun-and-profit

I'm hoping to get funding in 2021 to build a token creator atop Search Kit, which would make creating many complex tokens possible for end users.