Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#49518 closed enhancement (fixed)

Consider adding domain-specific i18N filter hooks

Reported by: geminilabs's profile geminilabs Owned by: whyisjake's profile whyisjake
Milestone: 5.5 Priority: normal
Severity: normal Version: 5.3.2
Component: I18N Keywords: has-patch needs-testing 2nd-opinion has-dev-note
Focuses: coding-standards Cc:

Description

This would be useful in cases where plugins provide a search/replace option to translate plugin text. An example plugin is Site Reviews https://wordpress.org/plugins/site-reviews/ (which I develop and maintain). I optimise this process by calling a domain specific hook within each i18n filter hook, and perform any additional things inside these domain-specific hooks.

The problem with not having domain-specific i18n filter hooks is that the amount of times the i18n filter hooks are called are increased with every active plugin.

Even though there is little-to-no performance impact (by calling a domain specific hook within each i18n filter hook and only performing any additional things inside these domain-specific hooks), it still results in thousands, sometimes millions of function calls on every page load.

For example, on one system there were 14 million function calls on each page load due to this feature (even though the total combined execution time of these function calls was only ~<0.3s).

Here are the domain-specific i18n filter hooks I am proposing:

<?php
/**
 * Filters text with its translation for a domain.
 *
 * @since 5.4.0
 *
 * @param string $translation  Translated text.
 * @param string $text         Text to translate.
 */
$translation = apply_filters( 'gettext-' . $domain, $translation, $text );
<?php
/**
 * Filters text with its translation based on context information for a domain.
 *
 * @since 5.4.0
 *
 * @param string $translation  Translated text.
 * @param string $text         Text to translate.
 * @param string $context      Context information for the translators.
 */
$translation = apply_filters( 'gettext_with_context-' . $domain, $translation, $text, $context );
<?php
/**
 * Filters the singular or plural form of a string for a domain.
 *
 * @since 5.4.0
 *
 * @param string $translation Translated text.
 * @param string $single      The text to be used if the number is singular.
 * @param string $plural      The text to be used if the number is plural.
 * @param string $number      The number to compare against to use either the singular or plural form.
 */
$translation = apply_filters( 'ngettext-' . $domain, $translation, $single, $plural, $number );
<?php
/**
 * Filters the singular or plural form of a string with gettext context for a domain.
 *
 * @since 5.4.0
 *
 * @param string $translation Translated text.
 * @param string $single      The text to be used if the number is singular.
 * @param string $plural      The text to be used if the number is plural.
 * @param string $number      The number to compare against to use either the singular or plural form.
 * @param string $context     Context information for the translators.
 */
$translation = apply_filters( 'ngettext_with_context-' . $domain, $translation, $single, $plural, $number, $context );

Attachments (4)

screenshot-11.png (51.8 KB) - added by geminilabs 5 years ago.
The Translation feature of Site Reviews
Function Call Count.png (78.2 KB) - added by geminilabs 5 years ago.
49518.diff (4.2 KB) - added by garrett-eclipse 5 years ago.
Updated patch for Coding Standards (adding new lines around each block) and updating @since and adding to function definition
49518.2.diff (5.3 KB) - added by garrett-eclipse 5 years ago.
Updated patch to move domain specific filters after the more general filters

Download all attachments as: .zip

Change History (22)

@geminilabs
5 years ago

The Translation feature of Site Reviews

#2 @geminilabs
5 years ago

Typo correction: I meant to say, "14 million function calls each hour due to this feature"

This ticket was mentioned in Slack in #core by pryley. View the logs.


5 years ago

#4 @garrett-eclipse
5 years ago

  • Keywords has-patch needs-testing added

#5 @davidbaumwald
5 years ago

  • Keywords needs-dev-note added

Marking this for a Dev Note as well. Deserves at least a small call-out on the Misc Dev Note.

@garrett-eclipse
5 years ago

Updated patch for Coding Standards (adding new lines around each block) and updating @since and adding to function definition

#6 @garrett-eclipse
5 years ago

  • Milestone changed from Awaiting Review to 5.5

Thanks for the ticket @geminilabs and the initial patch @pryley.
In 49518.diff I made a minor update for coding standards and to include a @since in the wrapping function docblock. Also updated @since's to 5.5, and with that I'm going to milestone this as it looks good, just needs more review/testing.

#7 @johnbillion
5 years ago

I think these new domain-specific filters should get applied after the existing more generic filters, not before. This is a pattern that allows for the return value of more specific filters to override that of the more generic ones (see for example the plugin_action_links and plugin_action_links_{$plugin_file} filters).

@garrett-eclipse
5 years ago

Updated patch to move domain specific filters after the more general filters

#8 @garrett-eclipse
5 years ago

Thanks @johnbillion good point, updated in 49518.2.diff

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 years ago

#10 @whyisjake
4 years ago

  • Owner set to whyisjake
  • Status changed from new to accepted

#11 @whyisjake
4 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 48131:

I18N: Introduce domain specific i18n gettext hooks.

Adding context to allow for more specific targeting. New hooks:

  • gettext-{$domain}
  • gettext_with_context-{$domain}
  • ngettext-{$domain}
  • ngettext_with_context-{$domain}

Fixes #49518.

Props geminilabs, garrett-eclipse, davidbaumwald, johnbillion, whyisjake.

#12 follow-up: @swissspidy
4 years ago

Do we use dashes in hook names elsewhere? Seems a bit uncommon. Same with the concatenation.

#13 in reply to: ↑ 12 @knutsp
4 years ago

  • Focuses coding-standards added
  • Keywords 2nd-opinion added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to swissspidy:

Do we use dashes in hook names elsewhere? Seems a bit uncommon. Same with the concatenation.

Concatenated (dynamic?) filters and action strings seems to use "_" to connect to the variable part, except manage_edit-${post_type}_columns, where "edit-${post_type}" seems to be a special case inherited from $current_screen.

Normal examples: option_{$option name}, pre_update_option_{$option name}, create_{$taxonomy} and save_post_{$post->post_type}.

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

#14 @swissspidy
4 years ago

Scrolled through Slack to find the same question.

One answer was:

The domain with_context would collide with another filter without the dash.

However, I'd say that's extremely unlikely. Even more generally speaking, the norm is that the text domains matches the plugin slug, which uses dashes anyway, not underscores.


More importantly though, the filter names to not adhere to coding standards:

https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#interpolation-for-naming-dynamic-hooks

#15 @SergeyBiryukov
4 years ago

This introduces WPCS issues as is:

FILE: src\wp-includes\l10n.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 5 LINES
----------------------------------------------------------------------
 198 | WARNING | Words in hook names should be separated using
     |         | underscores. Expected: 'gettext_' . $domain, but
     |         | found: 'gettext-' . $domain.
     |         | (WordPress.NamingConventions.ValidHookName.UseUnderscores)
 262 | WARNING | Words in hook names should be separated using
     |         | underscores. Expected: 'gettext_with_context_' .
     |         | $domain, but found: 'gettext_with_context-' .
     |         | $domain.
     |         | (WordPress.NamingConventions.ValidHookName.UseUnderscores)
 482 | WARNING | Words in hook names should be separated using
     |         | underscores. Expected: 'ngettext_' . $domain, but
     |         | found: 'ngettext-' . $domain.
     |         | (WordPress.NamingConventions.ValidHookName.UseUnderscores)
 538 | WARNING | Words in hook names should be separated using
     |         | underscores. Expected: 'ngettext_with_context_' .
     |         | $domain, but found: 'ngettext_with_context-' .
     |         | $domain.
     |         | (WordPress.NamingConventions.ValidHookName.UseUnderscores)
 694 | WARNING | Found: ==. Use strict comparisons (=== or !==).
     |         | (WordPress.PHP.StrictComparisons.LooseComparison)
----------------------------------------------------------------------

#16 @SergeyBiryukov
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 48136:

I18N: Use underscores instead of dashes and interpolation instead of concatenation in domain-specific gettext hooks, per the coding standards:

  • gettext_{$domain}
  • gettext_with_context_{$domain}
  • ngettext_{$domain}
  • ngettext_with_context_{$domain}

Additionally:

  • Pass $domain parameter to the new hooks, for consistency with their pre-existing counterparts.
  • Update documentation to explain the dynamic portion of the filter, for consistency with other dynamic hooks in core.

Follow-up to [48131].

Props swissspidy, knutsp, TimothyBlynJacobs, SergeyBiryukov.
Fixes #49518.

This ticket was mentioned in Slack in #core by justinahinon. View the logs.


4 years ago

#18 @justinahinon
4 years ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.