WordPress
Update escaping function
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: https://github.com/WordPress/create-block-theme/issues/682
How?
Replacing the escaping function.
How to test:
Follow the screencast in https://github.com/WordPress/create-block-theme/issues/682
@creativecoder I implemented your suggestions in the latest commits. Thanks for the review!
@matiasbenedetto I took a closer look at this to better understand the generated output in theme patterns and attempted an improvement in https://github.com/WordPress/create-block-theme/pull/683/commits/3e53a9aa90c378d4e4a9d8424c3dc9248c804832 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?
@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.
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>'
);
?>
In the latest commit (https://github.com/WordPress/create-block-theme/pull/683/commits/75cc3d2a2026fbb0608dfbf700051ee44a423274) I removed the wp_kses_post as a escaping function. With that commit this PR only changes the way the HTML attributes are escaped by using esc_attr_e and left the rest of the html contents escaped as before using esc_html_e. In this way we would be able to merge this and fix https://github.com/WordPress/create-block-theme/issues/691. Still https://github.com/WordPress/create-block-theme/issues/682 won't be fixed by this PR.
I updated the description. This PR should only fix the escaping of the HTML attributes. The text content of blocks will remain the same for now and it should be tackled in another PR.
Could you give it another review, please?