WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 3 months ago

Last modified 8 weeks ago

#49518 closed enhancement (fixed)

Consider adding domain-specific i18N filter hooks

Reported by: geminilabs Owned by: 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 7 months ago.
The Translation feature of Site Reviews
Function Call Count.png (78.2 KB) - added by geminilabs 7 months ago.
49518.diff (4.2 KB) - added by garrett-eclipse 7 months 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 7 months ago.
Updated patch to move domain specific filters after the more general filters

Download all attachments as: .zip

Change History (22)

@geminilabs
7 months ago

The Translation feature of Site Reviews

#2 @geminilabs
7 months 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.


7 months ago

#4 @garrett-eclipse
7 months ago

  • Keywords has-patch needs-testing added

#5 @davidbaumwald
7 months 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
7 months ago

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

#6 @garrett-eclipse
7 months 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
7 months 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
7 months ago

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

#8 @garrett-eclipse
7 months ago

Thanks @johnbillion good point, updated in 49518.2.diff

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


3 months ago

#10 @whyisjake
3 months ago

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

#11 @whyisjake
3 months 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
3 months ago

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

#13 in reply to: ↑ 12 @knutsp
3 months 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 3 months ago by knutsp (previous) (diff)

#14 @swissspidy
3 months 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
3 months 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
3 months 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.


2 months ago

#18 @justinahinon
8 weeks ago

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