Make WordPress Core

Opened 6 years ago

Closed 6 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:


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 6 years ago.
Replace HTML with placeholders

Download all attachments as: .zip

Change History (12)

6 years ago

Replace HTML with placeholders

#1 @SergeyBiryukov
6 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.

#2 @mordauk
6 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.

#3 follow-up: @scribu
6 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 6 years ago by scribu (next)

#4 in reply to: ↑ 3 @mordauk
6 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.

#5 @SergeyBiryukov
6 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):

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

#6 @mordauk
6 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?

#7 follow-up: @scribu
6 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.

#8 @scribu
6 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' )

#9 in reply to: ↑ 7 @SergeyBiryukov
6 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

#10 @mordauk
6 years ago

Makes sense to me.

#11 @ocean90
6 years ago

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