WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 5 months ago

#33073 new enhancement

Some strings need "no HTML entities" translator comments

Reported by: xibe Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: I18N Keywords: needs-patch
Focuses: Cc:
PR Number:

Description

This is a renewal of #10005, which was apparently my first patch to WP 6 years ago, but never got accepted. #10005 was closed as fixed, but it truth it was not (see my last comment there).

So, I'm opening a new ticket about this... and updating the description and patch from 6 years :)

Here goes!

Several strings need to have an indication warning translators against the inclusion of HTML entities within their translation, because of where the strings are user (RSS feeds, e-mails, JavaScript...).

Case in point: in French, there should be a non-breaking space ( ) before any double sign (; : ! ?). So, translators would turn "URL: %s" into "Adresse web : %s". But since it is used in e-mails, it is displayed as-is. And for strings that are used in feeds, it breaks them...

Suggestions comment: "Do not add HTML entities ( , etc): used in [context]".

Patch fixes all strings that I could find.

I do hope it is a simple enough patch, and not too close to RC, that it could make it into 4.3.

Thanks!

Attachments (1)

33073.diff (18.4 KB) - added by xibe 4 years ago.
Patch for upgrade.php, feed-atom-comments.php, feed-rss2-comments.php, pluggable.php

Download all attachments as: .zip

Change History (16)

@xibe
4 years ago

Patch for upgrade.php, feed-atom-comments.php, feed-rss2-comments.php, pluggable.php

#1 @xibe
4 years ago

  • Keywords has-patch added

#2 @atomicjack
4 years ago

  • Keywords needs-testing added

This ticket was mentioned in Slack in #polyglots by xavier. View the logs.


4 years ago

#4 @fxbenard
4 years ago

+1 Making it for 4.3 will be a nice treat for all French translators (new comers - old elephants)

#5 follow-up: @xibe
4 years ago

Note: for some strings, I chose to expand the code block.

For instance:

if ( EMPTY_TRASH_DAYS )
	$notify_message .= sprintf( __('Trash it: %s'), admin_url("comment.php?action=trash&c=$comment_id") ) . "\r\n";
else
	$notify_message .= sprintf( __('Delete it: %s'), admin_url("comment.php?action=delete&c=$comment_id") ) . "\r\n";

...is turned into:

if ( EMPTY_TRASH_DAYS ) {
	/* translators: Do not use HTML entities ( , etc.): this string is used in e-mails. */
	$notify_message .= sprintf( __('Trash it: %s'), admin_url("comment.php?action=trash&c=$comment_id") ) . "\r\n";
}
else {
	/* translators: Do not use HTML entities ( , etc.): this string is used in e-mails. */
	$notify_message .= sprintf( __('Delete it: %s'), admin_url("comment.php?action=delete&c=$comment_id") ) . "\r\n";
}

The line I add is a comment, so I'm unsure if it's necessary to add curly braces to mark the code block, but I want to play it safe.

#6 @SergeyBiryukov
4 years ago

#10005 was marked as a duplicate.

#7 in reply to: ↑ 5 ; follow-up: @SergeyBiryukov
4 years ago

Replying to xibe:

The line I add is a comment, so I'm unsure if it's necessary to add curly braces to mark the code block, but I want to play it safe.

Good call, this usage of braces was added to the coding standards some time ago.

#8 @zodiac1978
4 years ago

+1

This a often a question on the i18n-table at contributor days.

Could/Should we use entities or not? - Depends on the context.

This would be good solution to add translator comments!

#9 in reply to: ↑ 7 ; follow-up: @xibe
4 years ago

Good call, this usage of braces was added to the coding standards some time ago.

So I see! I'm guessing the PSR2 Fixer has not been run on the whole codebase (with the braces option)? (I know we do not use PSR-2, but the fixer could be useful in this case). But that's for another ticket...

Last edited 4 years ago by xibe (previous) (diff)

#10 follow-up: @xibe
4 years ago

A note by @nbachiyski from #10005:

We have computers on our side and we can do better than telling the translators not to use entities. In all cases we can properly escape entities if they aren't allowed. It can wait until next version, though.

Still, nothing happened in 6 years, so I'm submitting my patch idea. If anything better comes up, I'd be happy to forget it :)

#11 in reply to: ↑ 10 ; follow-up: @azaozz
4 years ago

Replying to xibe:

Would it work if we run html_entity_decode() when using these strings? This is the case for all strings that are outputted for use in JS and as far as I know there aren't any problems with entities there.

#12 in reply to: ↑ 9 @netweb
4 years ago

Replying to xibe:

So I see! I'm guessing the PSR2 Fixer has not been run on the whole codebase (with the braces option)? (I know we do not use PSR-2, but the fixer could be useful in this case). But that's for another ticket...

See https://make.wordpress.org/core/2011/03/23/code-refactoring/, but the gist of that doc is "Code refactoring should not be done just because we can."

#13 in reply to: ↑ 11 @xibe
4 years ago

  • Keywords has-patch needs-testing removed

Replying to azaozz:

Would it work if we run html_entity_decode() when using these strings? This is the case for all strings that are outputted for use in JS and as far as I know there aren't any problems with entities there.

Good call! Actually, I see that the feed-atom-comments.php and feed-rss2-comments.php strings are already enclosed in ent2ncr() -- and the feeds don't break with "By: %s" translated to "Par : %s", so it seems we're good here.

For the mail related strings, I guess html_entity_decode() is indeed the better way, since it converts the entities to their applicable characters, whereas ent2ncr() "only" switches named entities to their numbered counterparts.

If so, I can take care of the patch.

#14 @johnbillion
4 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Version 4.3 deleted

This ticket was mentioned in Slack in #core-i18n by xavier. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.