Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make i18n functions filter their return values #27966

Merged
merged 17 commits into from
Jan 28, 2021
Merged

Make i18n functions filter their return values #27966

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
f9b484a
Make i18n functions filter their return values
leewillis77 Dec 31, 2020
99d3466
@wordpress/hooks local dependency
leewillis77 Jan 4, 2021
5176c06
Support for domain-specific filters as per PHP
leewillis77 Jan 4, 2021
1efa483
Move responsibility for creating applyFilters to default-i18n
leewillis77 Jan 5, 2021
4a3c2d7
Add tests
leewillis77 Jan 5, 2021
2866eb9
Merge branch 'master' into add/i18n-filters-2
leewillis77 Jan 6, 2021
883bc53
Merge branch 'master' into add/i18n-filters-2
leewillis77 Jan 7, 2021
a0a0a73
Flag dependency in tsconfig.json
leewillis77 Jan 12, 2021
e955c4c
package-lock changes
leewillis77 Jan 12, 2021
40cb141
Coding standards fix
leewillis77 Jan 14, 2021
a320295
Introduce i18n-filters test
leewillis77 Jan 21, 2021
815a422
Include i18n-filters test in CI task
leewillis77 Jan 21, 2021
f8eca3a
More consistent (not perfect) hook attachment
leewillis77 Jan 22, 2021
d6ae239
Test fix
leewillis77 Jan 23, 2021
8ab3436
Nit: type names start with a capital letter
leewillis77 Jan 26, 2021
f34fcf0
Add default i18n tests
leewillis77 Jan 26, 2021
f9650fe
Use type annotations rather than explicit conversion
leewillis77 Jan 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/client-assets.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,11 @@ function gutenberg_override_script( $scripts, $handle, $src, $deps = array(), $v
* `WP_Dependencies::set_translations` will fall over on itself if setting
* translations on the `wp-i18n` handle, since it internally adds `wp-i18n`
* as a dependency of itself, exhausting memory. The same applies for the
* polyfill script, which is a dependency _of_ `wp-i18n`.
* polyfill and hooks scripts, which are dependencies _of_ `wp-i18n`.
*
* See: https://core.trac.wordpress.org/ticket/46089
*/
if ( 'wp-i18n' !== $handle && 'wp-polyfill' !== $handle ) {
if ( ! in_array( $handle, [ 'wp-i18n', 'wp-polyfill', 'wp-hooks'], true ) ) {
sirreal marked this conversation as resolved.
Show resolved Hide resolved
$scripts->set_translations( $handle, 'default' );
}
}
Expand Down
1 change: 1 addition & 0 deletions packages/i18n/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
},
"dependencies": {
"@babel/runtime": "^7.11.2",
"@wordpress/hooks": "file:../hooks",
"gettext-parser": "^1.3.1",
"lodash": "^4.17.19",
"memize": "^1.1.0",
Expand Down
92 changes: 88 additions & 4 deletions packages/i18n/src/create-i18n.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* WordPress dependencies
*/
import { applyFilters } from '@wordpress/hooks';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The createI18n function shouldn't always use the global applyFilters function. It should allow for supplying a custom instance of the Hooks object returned by createHooks.

The hooks and i18n packages have a similar structure:

  • there is a createI18n or createHooks function that creates a I18n or Hooks object that has methods like __ or applyFilters
  • the program that uses these libraries can create multiple instances of these objects that are fully independent from each other and don't share any data. Imagine a Node.js server that returns server-generated pages, serving multiple users, and handling the requests in parallel. Each request has its own, isolated context.
  • both libraries create a "default" instance of their respective objects, and exports their methods as functions. That's what __ and applyFilters are -- methods of one specific object instance that are used as functions.

To keep the separation between default singletons and custom instances, createI18n should take a new hooks parameter:

createI18n( initialData, initialDomain, hooks )

use this hooks parameter to get to the right applyFilters:

return String( hooks.applyFilters( ... ) );

and the default-i18n.js module should configure it with the default hooks:

import { applyFilters } from '@wordpress/hooks';

// if the hooks package exported the `Hooks` object itself, we would pass it as is and
// wouldn't have to create a 'fake' one with `{ applyFilters }`
const i18n = createI18n( null, null, { applyFilters } );

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsnajdr I've updated the PR to follow this approach. The hooks arg is optional, and no filters will be called if it is not provided. default-i18n provides a copy of applyFilters from @wordpress/hooks when creating the default instance. Feedback welcome.


/**
* External dependencies
*/
Expand Down Expand Up @@ -149,22 +154,101 @@ export const createI18n = ( initialData, initialDomain ) => {

/** @type {__} */
const __ = ( text, domain ) => {
return dcnpgettext( domain, undefined, text );
const translation = dcnpgettext( domain, undefined, text );
/**
* Filters text with its translation.
*
* @param {string} translation Translated text.
* @param {string} text Text to translate.
* @param {string} domain Text domain. Unique identifier for retrieving translated strings.
*/
return String(
applyFilters( 'i18n.gettext', translation, text, domain )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Core recently added also domain-specific filters named i18n.gettext_${ domain }:

https://core.trac.wordpress.org/ticket/49518

Can we add them here, too, to keep the APIs as similar as possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsnajdr I'm happy to update to include that, however there is a slight difference here in implementations. In core "domain" is defaulted to "default" if not provided, but the JS methods don't do that (leaving "domain" as undefined). Given your comments previously about re-use of the resulting object I'm not sure whether we should:
a) update the __() etc. method signatures to set domain to "default" if not provided,
b) call a "default" filter if typeof domain === 'undefined'

Any thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leewillis77 The b) option looks better to me as it achieves the goal without changing the library too much. But I'm not really sure. I noticed the new gettext_${ domain } hooks when comparing the hooks in this PRs to what's already in Core/PHP. Anyway, adding these extra hooks is not a blocker and can be done at any later time.

);
};

/** @type {_x} */
const _x = ( text, context, domain ) => {
return dcnpgettext( domain, context, text );
const translation = dcnpgettext( domain, context, text );
/**
* Filters text with its translation based on context information.
*
* @param {string} translation Translated text.
* @param {string} text Text to translate.
* @param {string} context Context information for the translators.
* @param {string} domain Text domain. Unique identifier for retrieving translated strings.
*/
return String(
applyFilters(
'i18n.gettext_with_context',
translation,
text,
context,
domain
)
);
};

/** @type {_n} */
const _n = ( single, plural, number, domain ) => {
return dcnpgettext( domain, undefined, single, plural, number );
const translation = dcnpgettext(
domain,
undefined,
single,
plural,
number
);
/**
* Filters the singular or plural form of a string.
*
* @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} domain Text domain. Unique identifier for retrieving translated strings.
*/
return String(
applyFilters(
'i18n.ngettext',
translation,
single,
plural,
number,
domain
)
);
};

/** @type {_nx} */
const _nx = ( single, plural, number, context, domain ) => {
return dcnpgettext( domain, context, single, plural, number );
const translation = dcnpgettext(
domain,
context,
single,
plural,
number
);
/**
* Filters the singular or plural form of a string with gettext context.
*
* @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.
* @param {string} domain Text domain. Unique identifier for retrieving translated strings.
*/
return String(
applyFilters(
'i18n.ngettext_with_context',
translation,
single,
plural,
number,
context,
domain
)
);
};

/** @type {IsRtl} */
Expand Down