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.

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

1171: Ensure translatable strings in blocks can actually be translated #1173

Merged
merged 5 commits into from May 29, 2018

Conversation

Projects
None yet
3 participants
@miina
Copy link
Collaborator

commented May 24, 2018

Add creating amp.pot file from blocks' JS files and add to inline script for making sure that the strings can be translated.

Fixes #1171.

miina added some commits May 24, 2018

msgid ""
msgstr ""
"Content-Type: text/plain; charset=utf-8\n"
"X-Generator: babel-plugin-makepot\n"

This comment has been minimized.

Copy link
@miina

miina May 24, 2018

Author Collaborator

@westonruter Do you think that this file should be fully under version control or created when building? Looks like the file has to exist at least being empty for not creating missing file error when building, currently it's just an almost empty file, should we add all the strings under version control?

This comment has been minimized.

Copy link
@westonruter

westonruter May 24, 2018

Member

I think we should follow Gutenberg's lead here. The POT file is not committed to version control: https://github.com/WordPress/gutenberg/tree/master/languages

The POT file generation should be done as part of the build.

wp_add_inline_script( 'amp-editor-blocks', sprintf( 'ampEditorBlocks.boot();' ) );
wp_add_inline_script(
'amp-editor-blocks',
'wp.i18n.setLocaleData( ' . wp_json_encode( gutenberg_get_jed_locale_data( 'amp' ) ) . ', "amp" );' . sprintf( 'ampEditorBlocks.boot();' ),

This comment has been minimized.

Copy link
@miina

miina May 24, 2018

Author Collaborator

At this moment it's just including the amp.pot file created by babel, not including the PHP file translations, wondering if we should create a separate domain for the blocks and load only that specific file within the blocks instead, e.g. amp-js?

This comment has been minimized.

Copy link
@westonruter

westonruter May 24, 2018

Member

That's one of the questions raised in https://make.wordpress.org/core/2018/05/01/javascript-internationalization-the-missing-pieces/ under “Loading only specific set of translations”.

Use a different text domain per JavaScript module. Export a JSON file per text domain. This is appealing, but has to be ruled out quickly: the text domain is not known to GlotPress and is not stored in the database or anything.

If Babel is just creating an amp.pot for the JS strings, then we can just use that as-is and ignore the amp.pot for all other purposes. In other words, maybe there could be an alternative to gutenberg_get_jed_locale_data() which allows you to pass a path to a POT file instead (or maybe there is an underlying function that could be used instead). And then this POT file would not be used for translating the plugin as a whole. Instead, Babel should generate a dummy PHP file that contains all of the strings in it for translation by GlotPress. You can see that Gutenberg does this by generating a languages/gutenberg-translations.php file in addition to the languages/gutenberg.pot.

Not sure if this is possible, but it's what I'd be looking into.

This comment has been minimized.

Copy link
@miina

miina May 24, 2018

Author Collaborator

If I understand correctly then the following is true on using different text domain per JS module, however, it should be possible for separating the editor blocks translation file for all the blocks from the rest in case of this plugin since babel is creating the .pot file based on the block directory files only.

Use a different text domain per JavaScript module. Export a JSON file per text domain. This is appealing, but has to be ruled out quickly: the text domain is not known to GlotPress and is not stored in the database or anything.

Added a commit where amp-js locale is registered for editor blocks JS translation and is creating a separate amp-js.pot file for translation of the blocks, leaving amp.pot available for PHP and any other translations outside of blocks directory: 8ac7759. At least it seems to be working locally when translating some block-related strings within amp-js-et.po and strings from PHP files within amp-et.po.

Do you think this could be suitable setup for the plugin?

This comment has been minimized.

Copy link
@westonruter

westonruter May 24, 2018

Member

I don't know how well this will work with GlotPress on translate.wordpress.org. My understanding is that the POT files are not read and that only the strings used in PHP files are parsed and made available for translation. How would GlotPress be told of the POT files in the first place? I could very well be wrong. @swissspidy Do you have insights to share on the approach?

This comment has been minimized.

Copy link
@swissspidy

swissspidy May 24, 2018

Collaborator

Essentially:

  • as of now translate.wordpress.org ignores any POT files in a plugin and only scans PHP files
  • Whatever strings you extract from JS files and save in a POT file, you need to write to a PHP file so translate.wordpress.org finds them. Just like Gutenberg does it.
  • For that PHP file you use the same text domain as in the rest of the plugin („amp“)
  • The text domain (not locale! That‘s a completely different thing) in the JS file basically doesn‘t matter because you only need it for creating that PHP file. For consistency I would still use „amp“
  • JS I18N in WordPress is being shaped/defined right now. Loading only some strings for a particular JS module might make sense for a large plugin like Gutenberg, but not for this one IMO. It‘s better to wait for official recommendations and implementation.
  • Even if creating different PO files works locally, it doesn‘t matter if your plugin is hosted on WordPress.org. WordPress.org provides a single translation for all of your plugin‘s strings.
  • Again, this stuff is being figured out at the moment, so please join us in our efforts to standardize this for core and plugins.

This comment has been minimized.

Copy link
@swissspidy

swissspidy May 24, 2018

Collaborator

Regarding telling about POT files: it‘s one of the possible features for JS 18N, see our latest chat summary.

This comment has been minimized.

Copy link
@swissspidy

This comment has been minimized.

Copy link
@miina

miina May 25, 2018

Author Collaborator

Good to know that the JS i18n defining is happening right now, taking a look at the chat summary. Thanks for the clarification about the limitations of the current system.

@swissspidy

This comment has been minimized.

Copy link
Collaborator

commented on 8ac7759 May 24, 2018

Just for correct understanding: text domain != locale.

@swissspidy
Copy link
Collaborator

left a comment

Overall this looks good to me. As mentioned, JS I18N is pretty much in flux, so there will be some changes and improvements in the future. For now, this is probably as good as one can do in WP :-)

@westonruter westonruter added this to the v1.0 milestone May 29, 2018

@@ -30,7 +32,9 @@
},
"main": "blocks/index.js",
"scripts": {
"build": "cross-env BABEL_ENV=production webpack; grunt build",
"pot-to-php": "pot-to-php languages/amp-js.pot languages/amp-translations.php amp",
"makepot": "wp i18n make-pot .",

This comment has been minimized.

Copy link
@westonruter

westonruter May 29, 2018

Member

I'm having a very difficult time getting this to run. I had to update my WP-CLI to install the i18n package, but now running it exits with error code 137. This seems to indicate that the VM runs out of memory while running. So I had to bump VVV's memory usage from 2048MB to 4096MB. Then I can get it to run, but it takes an agonizing 1+ minute to run. @swissspidy Is this normal?

Make sure translations are included in release builds
* Avoid running translation logic when creating normal builds.
* Add Grunt task for invoking Webpack.
* Add build-release command for creating a ZIP with translations.

@westonruter westonruter merged commit 76b929c into develop May 29, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/1171-support_blocks_string_translation branch May 29, 2018

npm run build-release
```

Note that this will currently take much longer than a regular build because it generates the files required for translation. You also must have WP-CLI installed with the `i18n` package.

This comment has been minimized.

Copy link
@swissspidy

swissspidy May 29, 2018

Collaborator

This comment has been minimized.

Copy link
@swissspidy

swissspidy May 29, 2018

Collaborator

Regarding the performance: a quick guess is that the command currently scans every folder looking for PHP files to parse—including node_modules. We might wanna exclude this directory (and others) by default. See https://github.com/wp-cli/i18n-command/blob/90f3f8e80a8e7db71e3b1e2d2b26140d5dd9e9fe/src/WordPressCodeExtractor.php#L115-L135 and wp-cli/i18n-command#27.

If you try running wp i18n make-pot without a node_modules folder being there, it should definitely be very fast. If not, it's something else.

This comment has been minimized.

Copy link
@westonruter

westonruter May 29, 2018

Member

@swissspidy I tried moving vendor and node_modules out of the directory without much improvement. But when I moved .git out of the directory, then it became much faster. It seems that it's scanning all the files inside the .git directory as well. So yeah, excluding those directories seems like the solution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.