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

Update i18n to make use of updated WP-CLI command #1329

Merged
merged 12 commits into from Aug 13, 2018

Conversation

Projects
None yet
2 participants
@westonruter
Copy link
Member

westonruter commented Aug 9, 2018

  • Do we need load_plugin_textdomain( 'amp', false, 'languages/amp.pot' );? Or is that unnecessary because of the Domain Path meta?
  • Is it no longer to pot-to-php anymore because the POT file now has all JS and PHP strings?

Fixes #1327

westonruter added some commits Aug 9, 2018

@westonruter westonruter added this to the v1.0 milestone Aug 9, 2018

@westonruter westonruter requested a review from swissspidy Aug 9, 2018

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Aug 9, 2018

@swissspidy feel more than free to amend this PR with additional commits as necessary.

@swissspidy

This comment has been minimized.

Copy link
Collaborator

swissspidy commented Aug 10, 2018

Do we need load_plugin_textdomain( 'amp', false, 'languages/amp.pot' );? Or is that unnecessary because of the Domain Path meta?

load_plugin_textdomain() is not needed when the following criteria is met:

  1. The plugin requires at least WordPress 4.6
  2. The plugin leverages the WordPress.org translation platform and ships with no other translations

In these cases, WordPress downloads translations from translate.wordpress.org to wp-content/languages/plugins.

Domain Path is irrelevant here. The domain path is used so that WordPress knows where to find the translation when the plugin is disabled. Only useful if the translations are located in a separate language folder and the criteria above is not met.

Looking at the readme (https://github.com/Automattic/amp-wp/blob/d2ad9d61fc0607334b6f282e7c14844330c8e1b2/readme.txt#L4) and the translation platform (https://translate.wordpress.org/projects/wp-plugins/amp), the criteria is met and the call to load_plugin_textdomain() can be removed.

Is it no longer necessary to run pot-to-php anymore because the POT file now has all JS and PHP strings?

Unfortunately, it's a bit more complicated than that. At least at the moment, as the necessary work in WordPress core and WordPress.org hasn't began yet.

So yeah, pot-to-php is still needed.

Here's a quick rundown of what's needed for plugins:

Plugin hosted solely on GitHub or other platform

  1. Use WP-CLI to extract all strings (from PHP and JS) into a single POT file.
  2. Create translations based on that POT file
  3. Call load_plugin_textdomain() to load translations
  4. Use gutenberg_get_jed_locale_data() to pass translations to JS

Plugin hosted on WordPress.org

  1. Use WP-CLI to extract only the JavaScript strings into a single POT file.
  2. Run pot-to-php to create a PHP file containing all these strings.
  3. Translate on WordPress.org and let WordPress handle the installation of these translations.
  4. Use gutenberg_get_jed_locale_data() to pass translations to JS.

That's quite a difference in steps needed. The reason is this:

Right now, WordPress.org only supports extracting strings from PHP files. JavaScript is ignored entirely. That's why we need to cheat a bit and use pot-to-php to create a PHP file WordPress.org can read.

As mentioned above, necessary work in WordPress core and WordPress.org to change this hasn't began yet.

In the future, pot-to-php should not be necessary anymore. Instead, WordPress.org will likely either

a) Use WP-CLI instead of the current library to extract strings from JavaScript files (see http://meta.trac.wordpress.org/ticket/3748)
b) Support reading a POT file provided by the plugin that contains the strings from JavaScript files


tl;dr: I'll try to find some time on Sunday/Monday to make the necessary changes to this PR, unless someone beats me to it :-)

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Aug 10, 2018

@swissspidy thank you. So it looks like most of what I did in 74f5f77 should be reverted. Also when calling wp i18n make-pot we should only look at JS files since the PHP files will already by accounted for on WordPress.org. And we'll need to restore the use of php-to-pot to take the JS POT file to create a PHP file with the translation strings, as this PR currently undoes.

swissspidy added some commits Aug 12, 2018

@swissspidy

This comment has been minimized.

Copy link
Collaborator

swissspidy commented Aug 12, 2018

Two non-blocking things I noticed:

  • The WP-CLI command also extracts the plugin info (name, description, etc.) from the main plugin file, even if we're only interested in the JS strings.

    Not really a problem since these strings shouldn't be duplicated on WordPress.org. It's just unnecessary. Perhaps GlotPress adds a wrong file reference to languages/amp-translations.php for these strings, but the file references are already wrong for all the JS strings anyway.

    I opened an issue about this nonetheless: wp-cli/i18n-command#76

  • The pot-to-php command (or maybe the underlying gettext-parser) generates an invalid PHP file because it doesn't properly handle comments inside the POT file.

    Not a big deal since the makepot.php script currently used on WordPress.org doesn't care about that.

@swissspidy

This comment has been minimized.

Copy link
Collaborator

swissspidy commented Aug 12, 2018

I just noticed that the plugin uses core's default text domain in multiple places.

While I get the intent to re-use existing translations in WordPress core, especially for things like AMP-enabled widgets, it's generally considered a bad practice. Plugins should not rely on the default WordPress text domain as it's not guaranteed that these strings will exist in core all the time. Also, it makes it harder for users to fully translate the plugin as they don't have control over these strings.

swissspidy and others added some commits Aug 12, 2018

@swissspidy swissspidy referenced this pull request Aug 13, 2018

Merged

Add .editorconfig file #1336

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Aug 13, 2018

I just noticed that the plugin uses core's default text domain in multiple places.

Thank you. That is good to keep in mind. So if we keep the default strings then we'd need to make sure they remain in place with each core release. I suppose we could make that check even part of the Travis build process to make sure that that persist in core. Or else like you say we could just switch to amp. It just seems a shame to replicate text which is already translated dozens/hundreds of times.

Your changes here look great. You can approve the PR (since I opened it originally) and then I'll merge it.

@swissspidy

This comment has been minimized.

Copy link
Collaborator

swissspidy commented Aug 13, 2018

We'd need to make sure they remain in place with each core release. I suppose we could make that check even part of the Travis build process to make sure that that persist in core.

That sounds like way more work than just using the correct text domain in the first place.

Given that only around a dozen or so strings are affected, this is not worth it IMO. The AMP plugin has around 120 strings and is only fully translated in a handful of languages. Assuming that the strings are unlikely to change soon, one could always export the translations from the WordPress core project and import them for the AMP plugin so that there's no additional burden for the translators.

@westonruter westonruter merged commit 14d6e4b into develop Aug 13, 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 update/i18n branch Aug 13, 2018

@westonruter westonruter referenced this pull request Aug 16, 2018

Merged

Fix deploy errors #1345

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.