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

Eliminate obsolete i18n code #1789

Merged
merged 5 commits into from
Jan 2, 2019
Merged

Eliminate obsolete i18n code #1789

merged 5 commits into from
Jan 2, 2019

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Dec 31, 2018

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
Gruntfile.js Outdated
@@ -38,7 +38,8 @@ module.exports = function( grunt ) {
command: 'npm run pot-to-php && php -l languages/amp-translations.php'
},
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
Member Author

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
Collaborator

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
Member Author

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

@swissspidy
Copy link
Collaborator

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
Member Author

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

@swissspidy
Copy link
Collaborator

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
Member Author

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

So given this example:

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
Collaborator

swissspidy commented Jan 2, 2019

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
Member Author

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants