Add theme.json i18n mechanism and JSON file specifying which theme.json paths are translatable #27380
Conversation
jorgefilipecosta
added
Internationalization (i18n)
jorgefilipecosta
requested review from
spacedmonkey and
TimothyBJacobs
as code owners
|
Size Change: 0 B Total Size: 1.28 MB ℹ️ View Unchanged
|
jorgefilipecosta
force-pushed
the
add/json-file-exposing-the-translatable-paths-and-a-theme-json-translation-mechanism
branch
from
08d698d to
8effe79
Compare
|
Is there a similar effort for |
lib/class-wp-theme-json-resolver.php
Outdated
| * @param string $domain Optional. Text domain. Unique identifier for retrieving translated strings. | ||
| * Default 'default'. | ||
| */ | ||
| private static function apply_theme_json_translations( &$theme_json_structure, $domain = 'default' ) { |
There was a problem hiding this comment.
Any specific reason for call by reference vs call by value?
There was a problem hiding this comment.
The only reason why I am passing by reference instead of returning a newly translated theme.json is to avoid copying the entire tree. I don't think it makes a big difference so if you think it is better to return I can change.
There is #23636 that still needs work. |
jorgefilipecosta
force-pushed
the
add/json-file-exposing-the-translatable-paths-and-a-theme-json-translation-mechanism
branch
from
0d9e815 to
949ecee
Compare
|
Hi @swissspidy, @nosolosw thank you a lot for providing feedback. It was applied. @swissspidy let me know if you prefer passing by value instead of reference and in that case, I will update the code. |
|
@jorgefilipecosta I've reviewed this and the cli issue and wanted to ask how do we manage the scenario where Gutenberg, the plugin, needs to add more presets to be translated? Is it ok to not allow translating the new ones until they are part of WordPress core? |
lib/class-wp-theme-json-resolver.php
Outdated
| * | ||
| * @return array An array of arrays each one containing a translatable path and an array of properties that are translatable. | ||
| */ | ||
| private static function theme_json_i18_file_structure_to_paths( $file_structure_partial, $current_path = array() ) { |
There was a problem hiding this comment.
This processing logic is tied to how presets are structured and can't be used for anything else. I don't anticipate that we want to translate any other thing in theme.json either. So, given this context, what do you think of making the naming of everything this PR adds more explicit: translate_presets instead of apply_theme_json_translations, get_presets_to_translate instead of get_theme_json_i18n, etc.
There was a problem hiding this comment.
Ideally, it isn't tied to themes.json as well. We need it for block.json 😄
There was a problem hiding this comment.
oh, so is it the idea that the file lib/experimental-i18n-theme.json and this code is used for both block.json and theme.json? If that's the case, it sounds like something independent from the WP_Theme_JSON_Resolver class.
There was a problem hiding this comment.
The definition with config can (or maybe even should) be in their own files for blocks and themes. However the logic that applies translation functions could be shared 😃
There was a problem hiding this comment.
Well, it is a fact that this code and even the structure is focused on presets.
All these functions were not implemented with block.json in consideration, but I guess we can in fact have a genetic data structure translation tool outside WP_Theme_JSON_Resolver (but used by WP_Theme_JSON_Resolver) this independent class could be used by both block.json and by theme.json.
We would need a specification format that can handle block.json and theme.json independent of presets I guess something like that would work
{
'*': {
"settings": {
"typography": {
"fontSizes": {
'*': [
'name'
]
}
}
}
}
}
But this is not very future proof as soon as we had nesting this specification would not work.
Being totally generic seems to bring more complexity to the specification structure. Maybe it is fine to have a set of functions focused on theme.json and presets and then we have something similar for block.json?
There was a problem hiding this comment.
It can be two different helpers if that makes it simpler 👍
There was a problem hiding this comment.
Hi @nosolosw, I renamed the functions as suggested.
jorgefilipecosta
force-pushed
the
add/json-file-exposing-the-translatable-paths-and-a-theme-json-translation-mechanism
branch
from
949ecee to
8bd502e
Compare
I agree this is an issue. I think it might be worth exploring having the .json file itself also be able to specify what paths are translatable. Perhaps using something like a |
Hi @nosolosw, @TimothyBJacobs, I think it is ok to not translate new ones until they are in the core. If we really need the new presets translated we can go for a temporary solution relying on filters&code as we are doing now, or we can simply update the file on WordPress trunk updating the JSON file of core does not seems like a big effort. |
I don't think I have enough context to understand whether this is an ok trade-off, so would like to hear thoughts from people more experienced, cc @mcsf @youknowriad @mtias If we have the greenlight on that specific point (people can't translate new presets that are added in the plugin OR we enable a mechanism for them to do it, etc), I can expedite a review to merge this PR. |
It is not that people can not translate new presets that are added in the plugin it is more. Core is the source of truth regarding what properties are translatable. We can always add more translatable properties to the core trunk. That is a simple change and makes things in the plugin also translatable. I guess if this proves to a be problem it would also be possible to add --gutenberg parameter to wp-cli extraction command that would make wp-cli use the translation file from the Gutenberg repository instead of the one from the core. |
|
I did some checks in the WordPress core to compare how translations are applied to the plugin's metadata. There is some more processing because translated strings are also escaped there. Do we need it here, too? The code snippet for reference: There is also |
There was a problem hiding this comment.
I've reviewed this and the cli issue and wanted to ask how do we manage the scenario where Gutenberg, the plugin, needs to add more presets to be translated? Is it ok to not allow translating the new ones until they are part of WordPress core?
If WP-CLI is going to use the JSON file from the trunk of wordpress-develop repository then it should be as simple as committing the change directly there. The only case where it wouldn't be possible is the RC phase of the WordPress release, at most 3 weeks, but in practice, I think the release branch is created earlier. In the plugin, we still would have to keep overrides between WordPress releases and that's why I asked in another comment whether we should add a filter to account for that.
| private static function get_presets_to_translate() { | ||
| static $theme_json_i18n = null; | ||
| if ( null === $theme_json_i18n ) { | ||
| $file_structure = self::get_from_file( __DIR__ . '/experimental-i18n-theme.json' ); |
There was a problem hiding this comment.
Is the plan to always keep a local copy of the file with the schema for translations? It usually gives us more flexibility when there is a hook introduced upfront. This way the filters let to add only new keys that need to be translated.
There was a problem hiding this comment.
Is the plan to always keep a local copy of the file with the schema for translations?
Yes, the plan is for the file to always exist locally.
It usually gives us more flexibility when there is a hook introduced upfront. This way the filters let to add only new keys that need to be translated.
If we had a hook that allowed to add new keys to be translatable, wp-cli would not be aware of the hook and would not extract the strings anyway, so the strings would not be translatable anyway.
The keys that are translatable are something static like in https://github.com/WordPress/wordpress-develop/blob/d2c8fae049e80f5726653c61361d467fad1a8a0b/src/wp-admin/includes/plugin.php#L149-L194 a plugin can not add other $plugin_data keys to be translatable.
If a plugin wants to inject translatable information in the theme.json structure, the plugin would use the filters that allow to change the structure and do something like $theme_json['settings'][mycustomPreset]= array( array( 'customPorperty' => __( 'translatable string' ) ) ); Given that it is done in code the string extraction would work as expected. So it would be possible for a plugin to inject translatable information using code not requiring to filter this file.
There was a problem hiding this comment.
I'm sure that some sort of filter is going to be necessary. Once you have the definition of translatable fields in the WordPress core, you might end up willing to expand it in Gutenberg when a new section is added to theme.json file. You can always simply override experimental-i18n-theme.json in Gutenberg and call it the day, but we should account for that when backporting logic to the WP core 😄
|
After a translation session in a WordCamp, I've just prepared a PR to add some context for translators in block's titles at #27933 I've used the I wonder how can we enable this kind of context/comments for the fields translated via theme.json? |
That's rather simple:
Note that context is more appropriate here than translator comments. |
…on translation mechanism.
Co-authored-by: Miguel Fonseca <[email protected]>
Co-authored-by: Pascal Birchler <[email protected]>
jorgefilipecosta
force-pushed
the
add/json-file-exposing-the-translatable-paths-and-a-theme-json-translation-mechanism
branch
from
8bd502e to
93c3c87
Compare
jorgefilipecosta
deleted the
add/json-file-exposing-the-translatable-paths-and-a-theme-json-translation-mechanism
branch
This PR exposes a JSON file with the theme.json paths that are translatable as suggested in wp-cli/i18n-command#224 (comment) by @schlessera.
For now this file is part of Gutenberg plugin but it should be part of core when we update the Gutenberg release core contains.
This file can be used by wp-cli to know which strings to extract from theme.json.
This PR also makes sure we call the WordPress translation functions on the theme.json structure from core, and themes. In order to have the strings translated.
To iterate on the tree and know the paths that are translatable we rely on the same JSON file.
How has this been tested?
I changed the translate function call to a hard-coded value that appends both parameters and I verified the "translation" was applied.