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

Improve language handling when creating JSON files #133

Merged
merged 2 commits into from Jan 12, 2019

Conversation

@swissspidy
Copy link
Member

commented Jan 9, 2019

Turns out \Gettext\Translations::setLanguage() throws an exception when trying to pass a language that's not in the list provided by https://packagist.org/packages/gettext/languages (CLDR).

That means it fails when passing de_DE, because it expects de-DE.

This PR does two things:

  • Replaces underscore with hyphens
  • Catches exceptions and falls back to "en" in that case.

@schlessera schlessera added this to the 2.1.1 milestone Jan 12, 2019

@schlessera schlessera merged commit 905672d into master Jan 12, 2019

1 check passed

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

@delete-merged-branch delete-merged-branch bot deleted the improve-language-handling branch Jan 12, 2019

@alialaa

This comment has been minimized.

Copy link

commented Feb 7, 2019

Hello, I still get this error even after replacing underscores with dashes. When I run the make-json command, I get absolutely nothing. No errors, no success messages and no files created. I went to users/myuser/.wp-cli/packages/vendor/wp-cli/i18n-command to debug and I var_dumped the error in this handler. And it says: string(33) "The language "de-DE" is not valid" apparently this error is thrown no matter what string is passed there. So even $mapping[ $source ]->setLanguage( 'en' ); in the error handler will throw an error. I tried hardcoding different strings and none worked.

@swissspidy

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

@alialaa Are you using the latest version of the command that includes the fixes from this PR?

@alialaa

This comment has been minimized.

Copy link

commented Feb 7, 2019

I guess so I ran php -d memory_limit=512M "$(which wp)" package install git@github.com:wp-cli/i18n-command.git and I am trying the code on this repository.

@alialaa

This comment has been minimized.

Copy link

commented Feb 7, 2019

Also in users/myuser/.wp-cli/packages/vendor/wp-cli/i18n-command/src/MakeJsonCommand I have this line $mapping[ $source ]->setLanguage( str_replace( '_', '-', $translations->getLanguage() ) );. I guess that means I am having the latest version?

@swissspidy

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

Ah yes, that looks correct.

Looks like the issue is that the language list (https://github.com/mlocati/cldr-to-gettext-plural-rules/blob/master/src/cldr-data/main/en-US/languages.json) includes de but not de-DE. However, it does include en-US and en.

The upstream library we use leverages the language information solely for the purpose of adding plural forms: https://github.com/oscarotero/Gettext/blob/545f716f08338030d2d0d35a1835d04a9d4f18ed/src/Translations.php#L352-L361

However, we already do that ourselves anyway:

$plural_forms = $translations->getPluralForms();
if ( $plural_forms ) {
list( $count, $rule ) = $plural_forms;
$mapping[ $source ]->setPluralForms( $count, $rule );
}

So I guess we can just silently ignore that exception...

@alialaa

This comment has been minimized.

Copy link

commented Feb 7, 2019

The thing is if I hard code 'en' like so in the try catch block, I also get The languane 'en' is not valid:

try {
   var_dump(Language::getAll());
   $mapping[ $source ]->setLanguage( 'en' );
} catch ( \InvalidArgumentException $e ) {
   // The locale had an invalid format.
   var_dump($e->getMessage());
   $mapping[ $source ]->setLanguage( 'en' );
}

Also not sure if this is related but Language::getAll() that I am dumping returns an empty array. Languages is from use Gettext\Languages\Language;

@swissspidy

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

Weird, I'm pretty sure that worked in tests. But not really a problem. With my suggestion (created #142 for it) we don't need that list anyway.

@alialaa

This comment has been minimized.

Copy link

commented Feb 7, 2019

So I can just remove that whole try, catch block if I need to test it now, correct?

@swissspidy

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

You could certainly do that for testing, yes, but that's not really the solution. We definitely should set the language for the translations.

The correct solution would probably be more like this:

try {
	$mapping[ $source ]->setLanguage( $translations->getLanguage() );
} catch ( \InvalidArgumentException $e ) {
	// Do nothing.
}

Or perhaps

$mapping[ $source ]->setHeader( \Gettext\Translations::HEADER_LANGUAGE, $translations->getLanguage() );
@alialaa

This comment has been minimized.

Copy link

commented Feb 7, 2019

Thanks, I will wait for your fix. I just tried this and it worked but I have some notes that maybe could be helpful to you:

  1. The js strings are removed from the PO files after generating the JSON. I know this is intended but what if I wanted to update the PO file later, how will I get the old values that are now gone?
  2. Also a weird thing happened with me. Not sure if it's intentional but I had multiple json files generated. 1 for each JS source file in the PO file.
  3. Also the output file should be called {pluginName}-{locale}-{handler}.json (where handler is the first arg in wp_set_script_translations) in order for the translations to work. So maybe you can use that instead of the hash?

Thanks for your help!

@swissspidy

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

@alialaa

  1. Use the --no-purge flag to skip this, as per the documentation.
  2. That is intentional. Please read the documentation and this longer explanation.
  3. No. Or to be more precise: It depends. {pluginName}-{locale}-{handle}.json and {pluginName}-{locale}-{hash}.json both work. The problem is, WP-CLI does not know your script handle. Neither does WordPress.org. That's why we need to use the hash variant. However, for WP-CLI we might need to figure out how you could inform WP-CLI of the script handles in order to use the other variant. See #127 for this.
@alialaa

This comment has been minimized.

Copy link

commented Feb 7, 2019

Thanks a lot :)

@swissspidy swissspidy referenced this pull request Feb 18, 2019
@michaelzangl

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2019

I enocuntered the same issue. This is what I do to fix the wp-cli.phar

  • Remove Symfony\Component\Finder\
  • Replace $mapping[ $source ]->setLanguage( $translations->getLanguage() ); by $mapping[ $source ]->setHeader( \\Gettext\\Translations::HEADER_LANGUAGE, $translations->getLanguage() );
$phar = new Phar('.....wp-cli.phar'); $phar['vendor/wp-cli/i18n-command/src/MakeJsonCommand.php'] = str_replace( ['Symfony\\Component\\Finder\\', '$mapping[ $source ]->setLanguage( $translations->getLanguage() );'], ['', '$mapping[ $source ]->setHeader( \\Gettext\\Translations::HEADER_LANGUAGE, $translations->getLanguage() );'], $phar['vendor/wp-cli/i18n-command/src/MakeJsonCommand.php']->getContent());
@cbratschi

This comment has been minimized.

Copy link

commented Mar 25, 2019

Which WP-CLI version will include this fix?

@swissspidy

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2019

@cbratschi The next one? :-)

Once #150 is merged, you can install the latest version of this package over what's included in WP-CLI by running this:

wp package install git@github.com:wp-cli/i18n-command.git

This way you don't have to wait for the next WP-CLI release.

@cbratschi

This comment has been minimized.

Copy link

commented Mar 31, 2019

@swissspidy Thanks a lot, it works now fine.

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