WordPress.org

Make WordPress Core

#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 20 months ago.
Replace HTML with placeholders

Download all attachments as: .zip

Change History (12)

mordauk20 months ago

Replace HTML with placeholders

comment:1 SergeyBiryukov20 months 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 mordauk20 months 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: scribu20 months ago

  • Keywords close added

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.

Last edited 20 months ago by scribu (previous) (diff)

comment:4 in reply to: ↑ 3 mordauk20 months 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 SergeyBiryukov20 months 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 mordauk20 months 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: scribu20 months 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 scribu20 months 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 SergeyBiryukov20 months 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 mordauk20 months ago

Makes sense to me.

comment:11 ocean9020 months ago

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