WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#21698 closed enhancement (wontfix)

Remove HTML from __() in load.php and wp_set_wpdb_vars()

Reported by: mordauk Owned by:
Milestone: Priority: normal
Severity: minor Version:
Component: I18N Keywords: close
Focuses: Cc:

Description

When WP checks to see if there is an error with the DB prefix, it outputs a localized error with HTML in it:

wp_die( __( '<strong>ERROR</strong>: <code>$table_prefix</code> in <code>wp-config.php</code> can only contain numbers, letters, and underscores.' ) );

It'd be better to use sprintf() and add the HTML after localization via placeholders:

wp_die( sprintf( __( '%sERROR%s: %s$table_prefix%s in %swp-config.php%s can only contain numbers, letters, and underscores.', '<strong>', '</strong>', '<code>', '</code>', '<code>', '</code>' ) ) );

Attachments (1)

prefix_error.patch (630 bytes) - added by mordauk 3 years ago.
Replace HTML with placeholders

Download all attachments as: .zip

Change History (12)

@mordauk3 years ago

Replace HTML with placeholders

comment:1 @SergeyBiryukov3 years ago

Why would that be better?

<strong>ERROR</strong> pattern is used in 11 more files. Replacing the tags with placeholders in this one string would be inconsistent and seems less readable to me.

comment:2 @mordauk3 years ago

My main reason was following what Otto says here: http://ottopress.com/2012/internationalization-youre-probably-doing-it-wrong/

If it gets replaced here, I would replace it everywhere to be consistent, I just happened to notice it here while I was browsing the file.

comment:3 follow-up: @scribu3 years ago

  • Keywords close added

Otto says:

Markup should be eliminated from your translated strings wherever possible.

It's always "possible". I'm pretty sure he meant "wherever it makes sense".

I don't think it makes sense here.

Version 0, edited 3 years ago by scribu (next)

comment:4 in reply to: ↑ 3 @mordauk3 years ago

Replying to scribu:

Otto says:

Markup should be eliminated from your translated strings wherever possible.

It's always possible, technically. I'm pretty sure he meant "wherever it makes sense".

I don't think it makes sense here.

Okay I can see that. I suppose it comes down to a matter of preference as well, I just prefer to never have HTML in a translated string.

comment:5 @SergeyBiryukov3 years ago

I think it makes more sense to leave out HTML tags when there's no need for sprintf(), e.g. like <p> tags in help text (which is not the case here):
http://core.trac.wordpress.org/browser/tags/3.4.1/wp-admin/custom-header.php#L104

Otto's example is essentially the same. It uses sprintf() for $number argument, not for HTML tags.

comment:6 @mordauk3 years ago

Line 139 of custom-header.php is:

'<p>' . __( '<a href="http://codex.wordpress.org/Appearance_Header_Screen" target="_blank">Documentation on Custom Header</a>' ) . '</p>'

That seems like a perfect place to use sprintf(). I can definitely agree that my original patch might be unnecessary, but how do you feel about this?

comment:7 follow-up: @scribu3 years ago

Yes, that makes more sense. And if we want to enable translators to change the Codex URL, we can add it as a separate string.

comment:8 @scribu3 years ago

We don't even need sprintf():

'<p><a href="' . __( 'http://codex.wordpress.org/Appearance_Header_Screen' ) . '" target="_blank">' . __( 'Documentation on Custom Header' ) . </a></p>'

but it does seem cleaner:

sprintf( '<p><a href="%s" target="blank">%s</a></p>',
  __( 'http://codex.wordpress.org/Appearance_Header_Screen' ),
  __( 'Documentation on Custom Header' )
);

comment:9 in reply to: ↑ 7 @SergeyBiryukov3 years ago

The Codex links are intended to be localizable.

Splitting those strings in two would result in more strings to translate and would require more memory. Localized installs are already close to 32 MB memory limit (ticket:19831:14, ticket:19852:21, #19832).

I'd prefer to leave those strings as is, at least until gettext performance is improved (#17128, #17268).

If the links were to some standard non-localizable location (like the one in line 498), it would be fine:
http://core.trac.wordpress.org/browser/tags/3.4.1/wp-admin/custom-header.php#L498

comment:10 @mordauk3 years ago

Makes sense to me.

comment:11 @ocean903 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.