Skip to:
Content

BuddyPress.org

Opened 9 years ago

Last modified 6 years ago

#6921 reopened enhancement

Maybe introduce PHP Mustache parser for email templates

Reported by: djpaul's profile DJPaul Owned by:
Milestone: Awaiting Contributions Priority: normal
Severity: normal Version: 2.5.0
Component: Emails Keywords: trac-tidy-2018
Cc:

Description

From #6915

Idea is to be able to conditionally render emails tokens without having to mix PHP conditionals into the templates; would also allow more customisation ability in the email content.

@ray did some investigation which I'm going to quote here, and attach his patch:


I'd rather take the time to consider replacing the basic/simple token parsing logic with a real Mustache template parser.

I think this would be very cool and quite easy to integrate.

Only question is what Mustache parser we would want to use.

  • mustache.php is linked from the official mustache Github page. Could use Composer for this, but library is 176kb.
  • LightnCandy claims to be 2-7 times faster than mustache.php, but requires PHP 5.3 due to namespaces. Library is also 196kb.

For 6915.mustache.patch, I decided to pick a lesser-known library, php-mustache, that is only 40kb and licensed under the BSD. We can pick and choose any library, I just chose one that was relatively small.

To address the issue in this ticket, I've included some unit tests for bp_core_replace_tokens_in_text() and have changed the email template to use a mustache section for the unsubscribe block -- {{#unsubscribe}}. The 'unsubscribe' token is only added if the user is active. See the changes made to bp_email_set_default_tokens() and bp_is_user_active().

Now, the unsubscribe link will not show up in registration and activation emails.

Attachments (3)

6915.mustache.patch (38.7 KB) - added by DJPaul 9 years ago.
6921.02.patch (11.5 KB) - added by DJPaul 6 years ago.
6921.03.patch (12.2 KB) - added by r-a-y 6 years ago.

Download all attachments as: .zip

Change History (14)

#1 @DJPaul
9 years ago

  • Component changed from Component - Core to API - Emails

#2 @DJPaul
7 years ago

  • Keywords trac-tidy-2018 added

We're closing this ticket because it has not received any contribution or comments for at least two years. We have decided that it is better to close tickets that are good ideas, which have not gotten (or are unlikely to get) contributions, rather than keep things open indefinitely. This will help us share a more realistic roadmap for BuddyPress with you.

Everyone very much appreciates the time and effort that you spent sharing your idea with us. On behalf of the entire BuddyPress team, thank you.

If you feel strongly that this enhancement should still be added to BuddyPress, and you are able to contribute effort towards it, we encourage you to re-open the ticket, or start a discussion about it in our Slack channel. Please consider that time has proven that good ideas without contributions do not get built.

For more information, see https://bpdevel.wordpress.com/2018/01/21/our-awaiting-contributions-milestone-contains/
or find us on Slack, in the #buddypress channel: https://make.wordpress.org/chat/

#3 @DJPaul
7 years ago

  • Milestone Awaiting Contributions deleted
  • Resolution set to maybelater
  • Status changed from new to closed

#4 @r-a-y
7 years ago

  • Milestone set to Up Next
  • Resolution maybelater deleted
  • Status changed from closed to reopened
  • Version set to 2.5.0

I still think this is a good idea. Most of the prep work is done as there is a patch with unit tests. Just need to decide on a Mustache parser to use.

At the moment, I'm running up against the wall trying to write dynamic email content in an email template and can't because we do not support conditional email tokens. My workaround is creating several email posts for each different variation, but this isn't the greatest thing.

Moving to "Up Next" as I would like to tackle this in the next few releases, pending dev feedback.

#5 @DJPaul
7 years ago

Yep, this would be super sweet to have in, for sure. Do you need any help researching PHP Mustache parsers?

#6 @DJPaul
6 years ago

I looked into this. I found a benchmark suite from the developers behind lightncandy. See http://zordius.github.io/HandlebarsTest/ and https://github.com/zordius/HandlebarsTest#quick-conclusion

I vote to move ahead with using lightncandy, which we can pull in via Composer.

@DJPaul
6 years ago

#7 @DJPaul
6 years ago

I've done a partial refresh of the old patch, just to get something on the table. It is not tested at all, might even have typos.

Looking at the old original patch, the bp_is_user_active() change, as well as the tweak in bp_email_set_default_tokens(), seem unnecessary to be mixed in with this patch. We should just not send email to a spammed user? (not sure if handle that already or not) Either way, not in scope for this ticket. So I left it out.

On the token replacement, we need to check if the library supports callable values for tokens, otherwise we need to resolve those first.

We might also want to consider caching the compiled templates in object cache (I am concerned about these getting too large, but if they're only a few KB, it's acceptable. I don't know if caching these would help from a performance perspective enough to be worth the trade-off).

#8 @r-a-y
6 years ago

@djpaul - Thanks for the refresh.

I had a chance to play around with the LightnCandy patch and unfortunately, our usage of mustasche tokens are incompatible with the official Mustasche syntax:
https://zordius.github.io/HandlebarsCookbook/0013-dotnotation.html

For example, take the token, {{poster.name}}.

We pass on the token in BuddyPress like this:

array( 'poster.name' => 'foo' )

Mustasche / LightnCandy expects this:

array( 'poster => array( 'name' => 'foo' ) )

We could switch to the latter syntax internally, but we would also break all 3rd-party plugins using BP HTML and tokens using dot notation.

03.patch does some backward-compatibility token parsing to ensure we can support the older syntax up to one-level of dot notation (poster.name would convert to the new syntax, but poster.name.surname wouldn't as we haven't used tokens with two dots internally).

Patch passes existing unit tests.

Note: I haven't switched up all of our internal tokens to use the new token syntax yet.

Also, in the patch, I've removed the usage of the use keyword since I would rather this be used in namespaced classes. This isn't a sticking point for me though.


Forgot to mention that LightnCandy says that token parsing shouldn't be compiled in production environments!
https://github.com/zordius/lightncandy#suggested-handlebars-template-practices

I've quoted the appropriate section below:

For best performance, you should only use 'compile on demand' pattern when you are in development stage. Before you go to production, you can LightnCandy::compile() on all your templates, save all generated PHP codes, and deploy these generated files (You may need to maintain a build process for this) . DO NOT COMPILE ON PRODUCTION , it also a best practice for security. Adding cache for 'compile on demand' is not the best solution. If you want to build some library or framework based on LightnCandy, think about this scenario.

So... if we were to use LightnCandy as per our current patches, we wouldn't be using best practices here.

Last edited 6 years ago by r-a-y (previous) (diff)

@r-a-y
6 years ago

#9 @DJPaul
6 years ago

Interesting point about compiled templates.

A transient is the obvious solution but we have a lot of email templates and they are quite large when you render the HTML template into them, and I'm worried about that hurting performance on sites without dedicated object caches (making the options table too big, right?).

Another quick idea is we could use WP_Posts's post_content_filtered column for this...

#10 @DJPaul
6 years ago

I've been thinking about this. Downside of using post_content_filtered is somehow having to re-generate that cache each time the template is edited in wp-admin, or the customiser, or when the template file is added or modified on the file system.

I can't see any alternative (other than not doing this) to storing this in a transient, and making a go/no-go based on how large the new transient options are in wp_options table (for sites without an object cache).

Last edited 6 years ago by DJPaul (previous) (diff)

#11 @DJPaul
6 years ago

  • Milestone changed from Up Next to Awaiting Contributions
Note: See TracTickets for help on using tickets.