Skip expensive smarty Processing when nothing to see here

[eileenmcnaughton]

Overview

This addresses a speed & disk use issue by not calling smarty in greeting processing when there are no tokens to process. It is a performance fix and a disk usage fix

The call to Smarty occurs AFTER civi tokens are processed - so in addition to parsing the tokens that will be present on most sites there are a small number of sites that implement actual smarty tokens like {if} in their address greetings. This is a feature that was implemented back in 4.2 73d64eb
to support German greetings.

Before

Smarty called on every contact create, edit, ?delete? to get tokens.

After

Smarty only called when there is a '{' present - otherwise we can assume it has nothing to do

No change in results

Technical Details

On digging into speed & disk use issues I find that significant disk space is being
used on smartyProcessing of greetings. The same also seems to be true of display names. This is
a first shot across the bows but I'm also investigating

1. also doing an early return before replaceGreetingTemplate
2. changing the smarty ->fetch call to pass 'eval' rather than string.
   Despite the gut-reaction the docs identify this as correct
   https://www.smarty.net/docs/en/resources.string.tpl and eval is not eval

From my local profiling this code also kicks in on delete & it's likely we will actually
see a decrease in overall test time on this

Comments

[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).

[eileenmcnaughton]

Ok - I added a sentence to the test docblock

[mattwire]

I have not run this but looks ok from r-code perspective

[eileenmcnaughton]

test this please

[eileenmcnaughton]

Steps to test are pretty much to edit a contact & check greetings look OK

[eileenmcnaughton]

Note we deployed this in production. It has made a significant difference to how many files are now in the templates c directory

[jaapjansma]

test this please

[jaapjansma]

Betty and I are reviewing PRs and we came across this one.

Steps to reproduce

1. Go to Administer --> Communications --> E-mail greetings and add the following greetings:

• Dear which is a greeting without a token.
• Dear {contact.first_name} which is a greeting with a token
• Dear {contact.first_name} {if '{contact.first_name}' === 'Tim'} The Wise{/if} which is greeting with a token and smarty functionality.

2. Now create a new contact with a first name Tim
3. Change the e-mail greeting and see whether the e-mail greetings are changed.(repeat this for every greeting given above).
4. Now create a new contact with a first name John and repeat step 3 to see whether the e-mail greetings are changed.

Review

• General standards
  • Explain (r-explain)
    • FAIL: We missed what problem this PR solves. Is it that the smarty greetings did not work before, or is it performance? If it is only performance, we think it is a useless addition as in most installations we assume the greetings contains tokens and are processed by smarty.
    • FAIL: We missed steps to reproduce this issue. We have figured it out and see the steps above.
    • COMMENT: We also miss a related issue on lab.civicrm.org but that is a minor detail.
  • User impact (r-user)
    • PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
  • Documentation (r-doc)
    • ISSUE: The user documentation should be updated. We miss a section with examples of different kind of greetings, including the one with a smarty template.
  • Run it (r-run)
    • FAIL: We could not save the greeting {contact.first_name} {if '{contact.first_name}' === 'Tim'} The Wise{/if}. We got a DB Error.
• Developer standards
  • Technical impact (r-tech)
    • PASS: The change potentially affects compatibility, but the risks have been sufficiently managed.
  • Code quality (r-code)
    • PASS: The functionality, purpose, and style of the code seems clear+sensible.
  • Maintainability (r-maint)
    • PASS: The change sufficiently improves test coverage.
    • COMMENTS: The tests are passing however the greeting available in test could not be entered by the user because we got a DB error. So the question is whether the automate tests are of a good enough quality as I would have expected them to fail as well.
  • Test results (r-test)
    • PASS: The test results have failures, but these have been individually inspected and found to be irrelevant.
    • COMMENTS: We do think the failing test is unrelated but we are not completely in a position to judge. The test is failing on the API v4 Conformance Test (and we don't know what that test means).

@eileenmcnaughton we think you need to do some additional work on this issue as could not enter the {contact.first_name} {if '{contact.first_name}' === 'Tim'} The Wise{/if} greeting.
We also think it might be good if you update the user documentation.

[eileenmcnaughton]

test this please

[eileenmcnaughton]

@jaapjansma I have updated the PR comments a bit - this is really about mitigating the performance and disk use impact of an old change to add an undocumented feature - ie 73d64eb

I don't really want to get into documenting that feature given that it is not created by this PR, or solving the fact that it can't be configured via the UI which is mostly part of that feature - but I can propose a variant that can be saved via the UI - {contact.first_name} {if 1 === 1} The Wise{/if}

Note that the 'normal' tokens like {contact.first_name} are CiviCRM tokens NOT smarty tokens so by the time the early return is hit they have already been removed. It is only sites that have had a site builder configure smarty

The goal here is to make it so supporting the smarty parsing feature doesn't cause negative impact on other sites that don't use it. The result of parsing smarty tokens here is that every contact you edit will create a file on disk with their name just to parse smarty tokens. Perhaps 2% of sites actually have Smarty tokens here. For sites that process 10s of thousands of contacts a day that is 10s of thousands of files created in templates_c

[mattwire]

Agree this is now ok