Update escaping function

[matiasbenedetto]

What?

This PR avoid that the HTML inside text blacks is escaped when printed to the page.

Why?

It seems like esc_html_e escapes HTML characters, which means it converts characters like < and > into their HTML entity equivalents (< and >). This prevents the <br> tags from being interpreted as HTML and instead displays them as plain text.

Fixes: #682

How?

Replacing the escaping function.

How to test:

Follow the screencast in #682

[creativecoder]

Using kses rather than an escaping function seems like the right solution to the linked issue.

However I think this needs a more nuanced change, based on how the escape_string method is used

• ✅ escaping block content--kses is appropriate
• ❌ For escaping attributes, we should be using esc_attr (or esc_attr_e)

What do you think of the following as a fix?

• Renaming escape_string to escape_attr, replacing esc_html_e with esc_attr_e, then using it below on line 128
• Adding kses_for_block_content (or similar name) that wraps wp_kses_post and using it below on line 112

Also, maybe these methods should be protected/private, since they aren't used outside the class.

[matiasbenedetto]

@creativecoder I implemented your suggestions in the latest commits.
Thanks for the review!

[creativecoder]

@matiasbenedetto I took a closer look at this to better understand the generated output in theme patterns and attempted an improvement in 3e53a9a to use wp_kses_post only when necessary.

My understanding is that wp_kses (and related kses functions) aren't meant to be escaping functions, though sometimes they can be used that way. That said, I don't think we should be using wp_kses_post on translated strings in theme patterns in place of esc_html.

This opens the door to translated strings containing a wide variety of html, including things like a tags. For themes released in the directory, I can imagine a kind of injection attack where translated versions of sites show injected html links, audio, videos, etc.

Potentially we could use wp_kses with a limited list of rich text tags (em, strong, br, code, etc) but having that as inline PHP seems like it would clutter pattern files considerably.

The best solution would be parsing HTML content and generating sprintf functions with placeholders for the relevant html in translated strings, but that seems very complex to get right.

What do you think?

[matiasbenedetto]

@creativecoder thanks for the detailed review. I have a few comments:

I don't think we should be using wp_kses_post on translated strings in theme patterns in place of esc_html.

I see that in 3e53a9a (#683)
wp_kses_post it is still used. If there's a potential security concern it should be removed completely, right?

The best solution would be parsing HTML content and generating sprintf functions with placeholders for the relevant html in translated strings, but that seems very complex to get right.

Yes, this seems to be too complex to worth it.

I'm not sure what would be the best way to move this forward.

[creativecoder]

I see that in 3e53a9a (#683)
wp_kses_post it is still used. If there's a potential security concern it should be removed completely, right?

Yes, I think so. That was my attempt to resolve the issue, but I don't think it's good enough.

I'm not sure what would be the best way to move this forward.

Same for me. For now, I think this needs to be worked around by manually editing the string in the theme template.

In the case of the video in the linked issue, splitting the string on the line breaks seems appropriate.

And for other inline html, using placeholders, for example:

<?php
/* translators: %1$s: start of bold text (<strong>), %2$s: end of bold text (</strong>
) */
sprintf(
    __( 'Some text %1$sthat is bold.%2$s', 'my-theme-textdomain' ),
    '<strong>',
    '</strong>'
);
?>