No (useful) content in mail, if there are shortcodes

Erenor commented 6 years ago

When using this plugin (version 3.8.1) together with others like BP Activity Plus, the mails users receive contain the exact activity's content, for example:

[bpfb_images] 1_0-48931900-1520595052_big-kudu-1.jpg [/bpfb_images]

This, of course, gives no idea about the Activity contents. A solution would be to use the function do_shortcode() to "evaluate" content before using it in mails, to give users a preview of the Activity.

boonebgorges commented 6 years ago

Thanks for the report.

It feels to me like blanket evaluation of all shortcodes is probably not a great idea. Many shortcodes are used to embed iframes or other interactive content that won't work in various ways in emails.

Does it seem reasonable to have a filter that would allow site owners to toggle the processing of shortcodes? Or maybe we can have a whitelist of shortcodes that are processed by default, with a filter to allow more? (Not sure off the top of my head whether the latter is technically easy to do.) @r-a-y Do you have thoughts about an approach?

Erenor commented 6 years ago

The latter (whitelist of shortcodes) would involve a bit of work when managing the content string.

I do like the idea of a flag in BPGES' settings to allow site owners activate/deactivate the parsing of the content. Of course, this should be an "on/off" option, to avoid too many computation with the shortcodes whitelist (regex and so on). As you said, I got some issues with the "video" upload in BPAP plugin, when the mail application tries to render the iframe and the Javascript code, but I feel it is a "minor" issue when compared to the "blank" output it currently provides.

r-a-y commented 6 years ago

Does it seem reasonable to have a filter that would allow site owners to toggle the processing of shortcodes? Or maybe we can have a whitelist of shortcodes that are processed by default, with a filter to allow more? (Not sure off the top of my head whether the latter is technically easy to do.) @r-a-y Do you have thoughts about an approach?

We could enable do_shortcode() to run, and then we could apply our own version of KSES that will run afterwards so it would strip stuff like <iframe> tags.

I guess we'd need to decide what tags are supported. A good elements list to look at would be bbPress' KSES filter: https://bbpress.trac.wordpress.org/browser/tags/2.5.14/includes/common/formatting.php#L15

We'd also need to test how images would look in the default BP email template. I'm not sure how responsive the template is.

boonebgorges commented 6 years ago

We could enable do_shortcode() to run, and then we could apply our own version of KSES that will run afterwards so it would strip stuff like tags.</p> </blockquote> <p>This seems really complicated. I found a couple guides around the web that give lists of HTML tags and CSS properties that are "universally" supported by clients. But even this is probably not enough to do the necessary parsing. For example, <code><img></code> tags work in general, but they won't work if the <code>src</code> is relative rather than absolute. Another example: HTML that depends on enqueued scripts or styles to work/look good will not work in email. So I think that the parsing would have to be on a per-shortcode basis, rather than per-tag. And then we could probably enable some built-in shortcodes that we are pretty confident will work (like maybe WP's <code>caption</code> or some shortcodes from common plugins like bbPress and BPAP), and put a filter in place for site owners to add more. </p> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/r-a-y"><img src="https://avatars.githubusercontent.com/u/505921?v=4" />r-a-y</a> commented <strong> 6 years ago</strong> </div> <div class="markdown-body"> <p>If we were to go with the per-shortcode approach, we would still need an email KSES regardless.</p> <p>For example, @Erenor notes that BP Activity Plus has a video upload option that converts their shortcode into an <code><iframe></code> element. Obviously, that would not work for email and we would need to strip that. That's where our version of KSES would kick in.</p> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/boonebgorges"><img src="https://avatars.githubusercontent.com/u/246627?v=4" />boonebgorges</a> commented <strong> 6 years ago</strong> </div> <div class="markdown-body"> <p>Yes, I guess that's good for extra safety. But if shortcode processing is <em>opt-in</em> on a per-shortcode basis, then the onus of verifying that the shortcode output is email-compatible is on the person who's adding the shortcode to the whitelist. So the BPAP video shortcode should <em>not</em> be added.</p> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/r-a-y"><img src="https://avatars.githubusercontent.com/u/505921?v=4" />r-a-y</a> commented <strong> 6 years ago</strong> </div> <div class="markdown-body"> <p>I was under the impression that BP Activity Plus had only one shortcode, but if it uses two different shortcodes for images and video, then sure.</p> <p>I still think limiting the shortcodes doesn't make much sense because if you implement a few shortcodes and KSES, you're basically halfway there. Why not just remove the shortcode restriction?</p> <p>Parsing just individual shortcodes would also be additional work for us as WordPress doesn't have a function to run just individual shortcodes.</p> </div> </div> <div class="page-bar-simple"> </div> <div class="footer"> <ul class="body"> <li>© <script> document.write(new Date().getFullYear()) </script> Githubissues.</li> <li>Githubissues is a development platform for aggregating issues.</li> </ul> </div> <script src="https://cdn.jsdelivr.net/npm/jquery@3.5.1/dist/jquery.min.js"></script> <script src="/githubissues/assets/js.js"></script> <script src="/githubissues/assets/markdown.js"></script> <script src="https://cdn.jsdelivr.net/gh/highlightjs/cdn-release@11.4.0/build/highlight.min.js"></script> <script src="https://cdn.jsdelivr.net/gh/highlightjs/cdn-release@11.4.0/build/languages/go.min.js"></script> <script> hljs.highlightAll(); </script> </body> </html>

boonebgorges / buddypress-group-email-subscription

No (useful) content in mail, if there are shortcodes #139