Twenty Fifteen: Unnecessary use of esc_html()

[ocean90]

Since #30651 I had the attached patch on my disk but never created a ticket, until now. Will do a refresh in the next hours.

[ocean90]

In 30896:

Twenty Fifteen: Don't escape translated strings.

Replace every unnecessary instance of esc_html__() and esc_html_e() with just __() and _e().

see #30724.

[ocean90]

In 30897:

Twenty Fifteen: Remove esc_html() from blog description.

The blog description gets esc_html()'d *into* the DB. It's also escaped because the filter for get_bloginfo() is set to 'display'.

see #30724.

[ocean90]

In 30899:

Twenty Fifteen: Don't escape translated strings.

Replace every unnecessary instance of esc_html_x() with just _x().

see #30724.

[ocean90]

In 30901:

Twenty Fifteen: Remove esc_html() from get_the_author().

The display name is sanitized through sanitize_text_field, wp_filter_kses, and _wp_specialchars.

see #30724.

[ocean90]

In 30905:

Twenty Fifteen: Remove unnecessary esc_html() from get_the_date() and get_the_modified_date().

see #30724.

[adamsilverstein]

Replying to ocean90:

In 30905:

Twenty Fifteen: Remove unnecessary esc_html() from get_the_date() and get_the_modified_date().

see #30724.

@ocean90: I'm curious about your removal of escaping from translations - can you briefly explain/point me to the the logic? I am always suspicious of translations as a malicious script injection vector, is this not the case?

[TomasM]

Is there anywhere a clear guidance for the theme developers on when to use escaping?

I follow the development of _s and default WP themes to make my themes better and recently in Twenty Fifteen and _s there was escaping promoted. Now, when I applied those changes to my theme I will have to revert :-/

Is there any harm for having escaping in place?

[ocean90]

Replying to adamsilverstein:

@ocean90: I'm curious about your removal of escaping from translations - can you briefly explain/point me to the the logic?

That's simple. If we don't trust translations anymore we should do it directly in the function. function __() { return esc_html(….

Replying to TomasM:

I follow the development of _s and default WP themes to make my themes better and recently in Twenty Fifteen and _s there was escaping promoted. Now, when I applied those changes to my theme I will have to revert :-/

That over-escaping seems to come from .com which is something I (and nacin, I talked with him before this ticket) don't want to support, especially not for a default theme where we have a type of review process for translations.

Is there any harm for having escaping in place?

No harm.

BTW: Twenty Fifteen wasn't even consistent with this.

[adamsilverstein]

Thanks for the clarification, much appreciated. [edited for brevity]

[sboisvert]

One problem with not escaping translations is that some plugins that filter translations will allow end users to push translations in the back end.
Depending on where these go they can break the code because characters are not escaped properly and the end users doing the translations won't understand what broke.
This doesn't even take under account that you may not be able to trust the end users doing the translations.

I feel that escaping protects against user error and potentially malicious users especially with translation plugins with little cost / negative repercussions.

Thanks!

[johnbillion]

Replying to sboisvert:

This doesn't even take under account that you may not be able to trust the end users doing the translations.

Translations are inherently trusted. The __() family of functions are used thousands of times and they don't escape output. If we're not trusting translations then we have a big problem.

[nacin]

I support all changes here. What _s does has been dictated by Automattic's zealous escaping rules for VIPs and ​http://theme.wordpress.com/join/. It is not dictated by general best practices.

[johnbillion]

In 30924:

Twenty Fifteen: Don't escape translated strings.

Replace every unnecessary instance of esc_html__() and esc_html_e() with just __() and _e().

Merges [30896] to the 4.1 branch.

see #30724.

[johnbillion]

In 30925:

Twenty Fifteen: Remove esc_html() from blog description.

The blog description gets esc_html()'d *into* the DB. It's also escaped because the filter for get_bloginfo() is set to 'display'.

Merges [30897] to the 4.1 branch.

see #30724.

[johnbillion]

In 30926:

Twenty Fifteen: Remove various unnecessary esc_html() calls and replace various unnecessary esc_html_x() calls with _x().

Merges [30899], [30901], and [30905] into 4.1.

Props ocean90
Fixes #30724

[williampatton]

There is a case for arguing that, while we can trust the translation system to not do bad things, other languages may contain entities that we do not expect to be there and as such escaping them is still a valuable thing.