Opened 9 months ago

Closed 9 months ago

#21698 closed enhancement (wontfix)

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

Reported by: mordauk Owned by:
Priority: normal Milestone:
Component: I18N Version:
Severity: minor Keywords: close
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 9 months ago.
Replace HTML with placeholders

Download all attachments as: .zip

Change History (12)

Replace HTML with placeholders

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.

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: ↓ 4   scribu9 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 9 months ago by scribu (previous) (diff)

comment:4 in reply to: ↑ 3   mordauk9 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.

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.

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: ↓ 9   scribu9 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.

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   SergeyBiryukov9 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

Makes sense to me.

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