Skip to content

Eliminate obsolete i18n code#1789

Merged
westonruter merged 5 commits into1.0from
improve/i18n
Jan 2, 2019
Merged

Eliminate obsolete i18n code#1789
westonruter merged 5 commits into1.0from
improve/i18n

Conversation

@westonruter
Copy link
Copy Markdown
Member

@westonruter westonruter commented Dec 31, 2018 •
edited
Loading

WordPress no longer needs POT files or generated PHP files for translation strings to be discovered. So we can eliminate all of that code.

This PR also adds a missing translators comment.

Without this --exclude argument, running time lando grunt shell:makepot takes 73 seconds to complete. What's more is that the generated amp-js.pot file includes references to files in the build/ directory even though this directory is not included in the --include argument. When the --exclude is included, the time to run time lando grunt shell:makepot cuts down to 41 seconds. Even though this is entirely too long to run (which is likely due to a Lando problem), it shows that adding the --exclude is cutting down on processing files unnecessarily.

This PR also uses Composer to explicitly mark the wp-cli/i18n-command dependency with a pinned version.

@westonruter westonruter added this to the v 1.0.2 milestone Dec 31, 2018
@googlebot googlebot added the cla: yes Signed the Google CLA label Dec 31, 2018
westonruter
westonruter commented Dec 31, 2018
View reviewed changes
Gruntfile.js Outdated
},
makepot: {
command: 'wp i18n make-pot . languages/amp-js.pot --include="*.js" --file-comment="*/null/*"' // The --file-comment is a temporary workaround for <https://github.com/ampproject/amp-wp/issues/1416>.
// TODO: The exclude argument to make-pot does not seem to actually exclude. Without including the --exclude argument, the generated amp-js.pot still includes references to the build/ directory.
Copy link
Copy Markdown
Member Author

@westonruter westonruter Dec 31, 2018

Choose a reason for hiding this comment

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

@swissspidy Can you advise regarding this?

Copy link
Copy Markdown
Collaborator

@swissspidy swissspidy Jan 2, 2019

Choose a reason for hiding this comment

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

build was never excluded by default, so if you don't need that, you have to pass --exclude.

Note: The latest version of the command changed the way includes and excludes are handled based on scores, see wp-cli/i18n-command#104.

@westonruter
Copy link
Copy Markdown
Member Author

westonruter commented Dec 31, 2018

@felixarntz Does running lando grunt shell:makepot on your machine similarly take a long time to complete?

@swissspidy
Copy link
Copy Markdown
Collaborator

swissspidy commented Jan 1, 2019

Quick question: Does the plugin still support WordPress 4.9 with the Gutenberg plugin or is Gutenberg support aimed at WordPress 5.0 only?

@westonruter
Copy link
Copy Markdown
Member Author

westonruter commented Jan 1, 2019

Support is intended for 4.9 without Gutenberg as well. For example #1781

@swissspidy
Copy link
Copy Markdown
Collaborator

swissspidy commented Jan 2, 2019

WordPress.org should handle string extraction of JavaScript files by now, as you can see here: https://translate.wordpress.org/projects/wp-plugins/amp/dev/en-gb/default?filters%5Boriginal_id%5D=7004670

That also means WordPress.org ships language packs with JSON translation files in them since WordPress 5.0. So there's no need to manually build POT files anymore.

Note: I published an in-depth guideline for WordPress JS I18N on my website: https://pascalbirchler.com/internationalization-in-wordpress-5-0/

wp_set_script_translations handles loading these JSON files for you.

wp_get_jed_locale_data only existed in the 5.0 Betas but has been removed after that. Any checks for that function should be removed as it does not exist.

gutenberg_get_jed_locale_data is for older versions of Gutenberg running on WordPress 4.9. It loads the translations from the PO files. IIRC WordPress.org also adds the JS translations to PO files, so that should work for these people.

@westonruter
Copy link
Copy Markdown
Member Author

westonruter commented Jan 2, 2019

@swissspidy ok, thank you. So we can eliminate makepot entirely?

So given this example:

amp-wp/includes/admin/class-amp-editor-blocks.php

Lines 162 to 171 in 8f7a60a

if ( function_exists( 'wp_set_script_translations' ) ) {
wp_set_script_translations( 'amp-editor-blocks', 'amp' );
} elseif ( function_exists( 'wp_get_jed_locale_data' ) || function_exists( 'gutenberg_get_jed_locale_data' ) ) {
$locale_data = function_exists( 'wp_get_jed_locale_data' ) ? wp_get_jed_locale_data( 'amp' ) : gutenberg_get_jed_locale_data( 'amp' );
wp_add_inline_script(
'wp-i18n',
'wp.i18n.setLocaleData( ' . wp_json_encode( $locale_data ) . ', "amp" );',
'after'
);
}

Are you saying that we don't need the elseif at all anymore? But I see that wp_set_script_translations() is not part of Gutenberg, so would 4.9 installs using Gutenberg just not be able to have translated strings?

Also, will wp_set_script_translations() take the entire plugin language pack for the locale and export it to JSON? It doesn't seem there is a way to subset it for what is actually needed in JS? So this means that PHP-only strings would be exported as well, right?

@swissspidy
Copy link
Copy Markdown
Collaborator

swissspidy commented Jan 2, 2019 •
edited
Loading

Yep, there's no need for that anymore. WordPress.org handles the make-pot part nowadays.

Are you saying that we don't need the elseif at all anymore? But I see that wp_set_script_translations() is not part of Gutenberg, so would 4.9 installs using Gutenberg just not be able to have translated strings?

4.9 + Gutenberg should still have gutenberg_get_jed_locale_data().

Also, will wp_set_script_translations() take the entire plugin language pack for the locale and export it to JSON?

No :-) That's the whole point of this new function. Everything is split up into multiple little JSON files that will be read and passed to the JS libraries in your plugin. See https://make.wordpress.org/core/2018/11/09/new-javascript-i18n-support-in-wordpress/ for more info.

However, gutenberg_get_jed_locale_data() does that.

You could shim wp_set_script_translations() in the plugin though in that case.

@westonruter
Copy link
Copy Markdown
Member Author

westonruter commented Jan 2, 2019

@swissspidy thank you very much. I've amended the PR then to eliminate POT generation entirely. The result is that there is that there is no longer a languages/amp-translations.php in the build which will be committed to WordPress.org. Can you confirm if the PR looks good now?

@westonruter westonruter changed the title Improve performance of make-pot call Eliminate obsolete make-pot calls Jan 2, 2019
@westonruter westonruter changed the title Eliminate obsolete make-pot calls Eliminate obsolete language translation logic Jan 2, 2019
@westonruter westonruter changed the title Eliminate obsolete language translation logic Eliminate obsolete i18n code Jan 2, 2019
@westonruter westonruter merged commit 4ad849f into 1.0 Jan 2, 2019
@westonruter westonruter deleted the improve/i18n branch January 2, 2019 20:37
@westonruter westonruter mentioned this pull request Jan 2, 2019
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@swissspidy swissspidy swissspidy approved these changes

@felixarntz felixarntz Awaiting requested review from felixarntz

Assignees

No one assigned

Labels

cla: yes Signed the Google CLA

Projects

None yet

Milestone

v1.0.2

Development

Successfully merging this pull request may close these issues.

3 participants

@westonruter @swissspidy @googlebot