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

Do not create smarty cached templates for processed greetings #16733

Merged
merged 1 commit into from
Apr 26, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Mar 10, 2020

Overview

Currently whenever a greeting is parsed through smarty a smarty template file is stored to disk. This has
impacts on disk use, cache clearing time and performance. Where we know that the template will be of low value so this uses the {eval} (not the evil eval) to only create one cache file for all singleUseStrings

Before

For every edited contact smarty files are created for each greeting - note post #16731 this only applies to contacts with smarty in greeting fields - eg. alter postal_greeting under administer->communications to
{contact.first_name}{if 1===1} the bold {/if}

After editing a contact their postal greeting should have 'the bold' (good) AND there will be a file created somewhere in templates_c with the compiled template for that contact (bad)

After

The commits that are also in
#16731 make it so smarty is only parsed if there are smarty tags. If it is parsed only one smarty template is created for ALL the contact greeting strings.

User experience should be the same but going into templates_c & doing ls -R * | grep .php should show no new created files

Technical Details

This tries to replicate the smarty3 approach somewhat in Smarty2

Smarty3 which we are alas not using directly allows specification of 'eval' which means 'like a string but don't cache' https://www.smarty.net/docs/en/resources.string.tpl

In order to replicate that in Smarty2 I'm using {eval} per
https://www.smarty.net/docsv2/en/language.function.eval.tpl#id2820446
From the above:

    • Evaluated variables are treated the same as templates. They follow the same escapement and security features just as if they were templates.
    • Evaluated variables are compiled on every invocation, the compiled versions are not saved! However if you have caching enabled, the output
  • will be cached with the rest of the template.
Our set up does not have caching enabled and my testing suggests this still works fine with it
enabled so turning it off before running this is out of caution based on the above.

When this function is run only one template file is created (for the eval) tag no matter how
many times it is run. This compares to it otherwise creating one file for every parsed string.

Comments

Test already added in #16731

I started out working through this
civicrm/civicrm-packages#234 by @sunilpawar
but I was uncomfortable on 2 fronts

  1. the obvious - it's patching a package
  2. I felt able to say the string I was working with and testing should not be compiled but I wasn't sure that NO string should be compiled . For the use case in clear smarty cache generated from string input civicrm-packages#234 to use this it would require a further change to call this function. I don't have my head around how clear cut it is in that case as yet

@civibot
Copy link

civibot bot commented Mar 10, 2020

(Standard links)

@civibot civibot bot added the master label Mar 10, 2020
@eileenmcnaughton eileenmcnaughton force-pushed the smarty branch 2 times, most recently from 859ceeb to 22879e7 Compare March 17, 2020 00:52
@eileenmcnaughton
Copy link
Contributor Author

Added has-test as it was pre-added in #16731

Currrently whenever a greeting is parsed through smarty a smarty template file is stored to disk. This has
impacts on disk use, cache clearing time and performance. Where we know that the template will be of low value
we are better not to cache it. Smarty provides eval (not that eval) for this

The technical details are in the code comment block
@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 @pfigel is one of you up for reviewing this? I feel like I'll struggle to find anyone else with their head around it - I think it might have been @andrew-cormick-dockery who originally complained about unnecessary files with personal information created in the templates_c dir

@andrew-cormick-dockery
Copy link
Contributor

I don't recall saying anything about files in the templates_c directory. Although I have raised issues in the past with plain text credit card details in logs. But that was a long time ago, and has been long fixed. And the only issue I can recall that I have raised with Smarty is regarding performance, but once again that was a long time ago. I'm not sure I can assist with the current ticket.

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.

3 participants