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

Adapt Jetpack to use WordPress JS i18n, fix Gutenberg block i18n. #11225

Merged
merged 9 commits into from
Feb 1, 2019

Conversation

zinigor
Copy link
Member

@zinigor zinigor commented Jan 30, 2019

Changes proposed in this Pull Request:

  • Adapts JSON language files to be usable in WordPress client-side i18n.
  • Rebuilds JSON language files.
  • Adds a mechanism to integrate POT files that result from block building into jetpack-strings.php. The whole PHP file generation pipeline now consists of one i18n-calypso run, one msgcat run, and one msgexec run that executes a bash script for every translation.
  • Paves the way for legacy i18n support and makes initial steps to move towards wp.i18n.

Testing instructions:

Currently blocks don't have any strings translated yet in the JSON files because the strings themselves are not present in the code. After merging this code we should be able to re-run language builds and get translated strings in blocks.

  • Make sure the existing i18n works properly, including existing wp-admin Settings translations.

Proposed changelog entry for your changes:

  • Adapted existing Jetpack translations to be used in WordPress i18n mechanisms.

@zinigor zinigor added [Type] Bug When a feature is broken and / or not performing as intended [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Focus] i18n Internationalization / i18n, adaptation to different languages labels Jan 30, 2019
@zinigor zinigor added this to the 7.0 milestone Jan 30, 2019
@zinigor zinigor requested a review from a team January 30, 2019 12:38
@jetpackbot
Copy link

jetpackbot commented Jan 30, 2019

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: February 5, 2019.
Scheduled code freeze: January 29, 2019

Generated by 🚫 dangerJS against 021a130

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

I tried to test this, but could not get it to work. Maybe I did not test well.


  • I seem to get the following error when I run yarn build-languages, afer deleting the existing language files in languages/json to start from scratch.

./tools/msgexec-callback.sh: line 20: conditional binary operator expected

In the end, my jetpack-strings.php file was empty for some reason. Should I have tested somehow differently?

  • Should we clean up the 2 .pot files in the build folder to make sure they do not get shipped?
  • Would there be a way for us not to commit all those json files, but instead only commit them on built branches?

_inc/lib/admin-pages/class.jetpack-react-page.php Outdated Show resolved Hide resolved
_inc/jetpack-strings.php Outdated Show resolved Hide resolved
@zinigor
Copy link
Member Author

zinigor commented Jan 30, 2019

Thanks for trying to test, the updated version should be testable for you because I made it compatible with older bash versions. The incompatibility was the reason why your jetpack-strings.php file ended up empty.

Unfortunately, we'll have to commit the JSON files because they both change their name, and change their data format - existing Jetpack translations that are in master will not get picked up by the new system.

@jeherve jeherve force-pushed the try/wp-client-i18n branch from 5ca50ea to f7e1614 Compare January 31, 2019 14:37
@jeherve
Copy link
Member

jeherve commented Jan 31, 2019

It seems to work well for me now, and I like the approach of this. I have rebased the PR so it can be merged.

@zinigor What would you think about deleting the pot files before shipping, with something like this:

diff --git a/gulpfile.babel.js b/gulpfile.babel.js
index 5f7ee321a..c64537c73 100644
--- a/gulpfile.babel.js
+++ b/gulpfile.babel.js
@@ -196,6 +196,15 @@ gulp.task( 'languages:cleanup', function( done ) {
 				} );
 				done();
 			} );
+
+			log( 'Cleaning up POT files.' );
+			del( [
+				'_inc/build/blocks/*.pot',
+				'./_inc/build/jetpack-strings.pot',
+				'./_inc/build/merged-strings.pot',
+			] ).then( function() {
+				done();
+			} );
 		}
 	);
 } );

__( "SEO preview tools", "jetpack" ), // _inc/client/components/jetpack-disconnect-dialog/index.jsx:96
__( "Site stats, related content, and sharing tools", "jetpack" ), // _inc/client/components/jetpack-disconnect-dialog/index.jsx:103
__( "Brute force attack protection and downtime monitoring", "jetpack" ), // _inc/client/components/jetpack-disconnect-dialog/index.jsx:107
__( "Unlimited, high-spee
Copy link
Member

Choose a reason for hiding this comment

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

For some reason we seem to have less strings here now than in the old jetpack-strings.php file:
https://github.com/Automattic/jetpack/blob/branch-7.0/_inc/jetpack-strings.php#L724

On my end, and that seems even weirder: if I run yarn build-languages on this branch locally, my jetpack-strings.php file only contains 633 lines.

What could explain all those differences?

Copy link
Member Author

Choose a reason for hiding this comment

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

We have several strings in the old jetpack-strings.php file that are repeated several times, for example strings like "At A Glance" or "Connected" can be seen used two or three times. The new POT merging method using msgcat merges these into entries that are only used once. I didn't compare the diff string by string, but I'm betting that this is the reason.

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This seems to work well. I'll merge, and put this to good use today :)

@jeherve jeherve removed the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Feb 1, 2019
@jeherve jeherve merged commit 7a964a2 into master Feb 1, 2019
@jeherve jeherve deleted the try/wp-client-i18n branch February 1, 2019 16:33
jeherve added a commit that referenced this pull request Feb 1, 2019
…1225)

<!--- Provide a general summary of your changes in the Title above -->

* Adapts JSON language files to be usable in WordPress client-side i18n.
* Rebuilds JSON language files.
* Adds a mechanism to integrate POT files that result from [block building](Automattic/wp-calypso#30303) into `jetpack-strings.php`. The whole PHP file generation pipeline now consists of one i18n-calypso run, one msgcat run, and one msgexec run that executes a bash script for every translation.
* Paves the way for legacy i18n support and makes initial steps to move towards wp.i18n.

Currently blocks don't have any strings translated yet in the JSON files because the strings themselves are not present in the code. After merging this code we should be able to re-run language builds and get translated strings in blocks.
* Make sure the existing i18n works properly, including existing wp-admin Settings translations.

* Adapted existing Jetpack translations to be used in WordPress i18n mechanisms.

Co-authored-by: Jeremy Herve <[email protected]>
@jeherve
Copy link
Member

jeherve commented Feb 1, 2019

Cherry-picked to branch-7.0 in 7c224ea

@jeherve
Copy link
Member

jeherve commented Feb 1, 2019

Here is the first run in the release branch: 0c16a17

It seems the strings from the blocks disappeared from the jetpack-strings.php file. I tried running yarn build-languages again after building the blocks from Automattic/wp-calypso#30303, but that did not help for some reason.

I did, however, notice this error during the run:

[18:16:24] Starting 'languages:phpize'...
[18:16:24] msgexec: warning: Locale charset "ASCII" is different from

[18:16:24]                   input file charset "UTF-8".
                  Output of 'msgexec' might be incorrect.
                  Possible workarounds are:
                  - Set LC_ALL to a locale with encoding UTF-8.
                  - Convert the translation catalog to ASCII using 'msgconv',
                    then apply 'msgexec',
                    then convert back to UTF-8 using 'msgconv'.

@jeherve
Copy link
Member

jeherve commented Feb 4, 2019

After some chat, we managed to get the strings in 19d5f61.

We're going to need #11258 as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] i18n Internationalization / i18n, adaptation to different languages [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants