Making WordPress.org

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#3876 closed enhancement (fixed)

Update wporg-gp-customizations to generate translation files for each JS file.

Reported by: herregroen's profile herregroen Owned by: ocean90's profile ocean90
Milestone: Priority: normal
Component: Translate Site & Plugins Keywords: has-patch
Cc:

Description (last modified by herregroen)

In order to support seperate javascript translation files and allow these to be selectively loaded as described in #3875 wordpress.org will need to be able to generate translation files for each JS file.

Currently a single translation file is generated ( in both po and mo format ) in the Language_Pack class during the build_language_packs function.

Translation entries are retrieved from GlotPress and all of these are always added to the PO file.

This should be changed so these translation entries are split according to their references.

Before building PO files all translation entries should be looped over and a mapping should be created of filenames to an array of translation entries. In this mapping there should be a single entry for all translations that occur in PHP files and an entry for every single JS file present in the translation references containing the appropriate translations.

For each entry in this mapping a different translation file should be created. These should all be added to the same ZIP the current PO and MO files are added to.

The filenames of the generated files for each JS file should be in the format of {$domain}-{$path}-{$locale}. Where $path is the full path to the JS file with path separators replaced by dashes. For example, the file js/src/script.js containing translations in the example domain would resolve to the filename example-js-src-script-nl_NL for translations of the nl_NL locale.

The filenames of the generated files for each JS file should be in the format of {$domain}-{$locale}-{$md5_of_path}. Where $md5_of_path is based on the full path to the JS file. For example, the file js/src/script.js containing translations in the example domain would resolve to the filename example-nl_NL-c3d772dafc117eeef45013ceb21629ed for translations of the nl_NL locale.

Doing this will ensure all translation files are ready to be included selectively in WordPress and only translations that are needed will be loaded.

Attachments (10)

3876.diff (5.2 KB) - added by herregroen 6 years ago.
3876.2.diff (5.2 KB) - added by herregroen 6 years ago.
3876.3.diff (6.0 KB) - added by herregroen 6 years ago.
3876.4.diff (236.9 KB) - added by herregroen 6 years ago.
3876.5.diff (240.0 KB) - added by herregroen 6 years ago.
3876.6.diff (6.1 KB) - added by herregroen 6 years ago.
core-language-pack.sh (1.6 KB) - added by ocean90 6 years ago.
3876.7.diff (11.8 KB) - added by herregroen 6 years ago.
3876.8.diff (11.8 KB) - added by ocean90 6 years ago.
3876.9.diff (7.3 KB) - added by ocean90 6 years ago.

Download all attachments as: .zip

Change History (45)

#1 @herregroen
6 years ago

  • Keywords needs-patch added

#2 @ocean90
6 years ago

As already mentioned serval times and to avoid confusions: There will still be only *one* language pack for core and each plugin/theme. The current build process will only be extended to include the JSON file(s) for the translations used in JavaScript files.

The filenames of the generated files for each JS file should be in the format of {$domain}-{$path}-{$locale}. Where $path is the full path to the JS file with path separators replaced by dashes

Does this really work for systems which have a file path limit?

#3 @swissspidy
6 years ago

I'd move the domain part to the middle of the file name. Very hard to read otherwise.

Could the path perhaps be a simple md5 hash of the filename instead?

Example: my-awesome-plugin-de_DE-098F6BCD4621D373CADE4E832627B4F6.json

What if a plugin renames their file names between releases? Then a user needs to re-download language packs, otherwise translations are missing, correct?

This ticket was mentioned in Slack in #meta by tellyworth. View the logs.


6 years ago

@herregroen
6 years ago

#5 @herregroen
6 years ago

I've added a patch for my initial attempt at generate translation files for JS translations.

As suggested they're in JSON format and using the md5 hash of the file instead of replacing / with -. That should ensure we don't run into file path limits.

@ocean90 @swissspidy I'd appreciate if you could take a look and see if this is the direction we want to go.

#6 follow-up: @ocean90
6 years ago

At first glance this is looking good. Shouldn't it use the jed.json format? I'm wondering if wp_get_installed_translations() in core needs any changes or if we can ignore that since there will always a po/mo file? It's mainly used to parse the revision date for updates.

#7 in reply to: ↑ 6 @herregroen
6 years ago

Replying to ocean90:

At first glance this is looking good. Shouldn't it use the jed.json format? I'm wondering if wp_get_installed_translations() in core needs any changes or if we can ignore that since there will always a po/mo file? It's mainly used to parse the revision date for updates.

That was my first idea as well, however looking at GlotPress it seems that the exported file will always contain 'domain' => 'messages' rather than the plugin domain which is why I decided against it.

I've left the po/mo file alone for that reason, since it will contain all translations the revision date should also be accurate for the JSON files. The downside is that if an update is needed all files would have to be replaced. But implementing partial updates may not be worth the effort, especially for the initial implementation.

#8 @herregroen
6 years ago

  • Keywords has-patch added; needs-patch removed
  • Summary changed from Update wporg-gp-customizations to generate language packs for each JS file. to Update wporg-gp-customizations to generate language files for each JS file.

#9 @herregroen
6 years ago

  • Description modified (diff)
  • Summary changed from Update wporg-gp-customizations to generate language files for each JS file. to Update wporg-gp-customizations to generate translation files for each JS file.

#10 @herregroen
6 years ago

Cleaned up the description and summary a bit to avoid confusion mentioned earlier.

#11 @swissspidy
6 years ago

That was my first idea as well, however looking at GlotPress it seems that the exported file will always contain 'domain' => 'messages' rather than the plugin domain which is why I decided against it.

That's correct, because GlotPress technically doesn't know the domain, but it's not a problem per se. We know the domain in core. See WP_Scripts:: load_translation_file() in https://core.trac.wordpress.org/attachment/ticket/20491/20491.7.diff for how we made it work in #core20491. We can do something similar in #core45103.

#12 @ocean90
6 years ago

I expect that many of the new strings will land in the front-end project. To avoid unnecessary memory usage, a string that's only used in a JavaScript file should not be part of the main po/mo file.

@herregroen
6 years ago

@herregroen
6 years ago

#13 @herregroen
6 years ago

Final patch includes both changes to export in the JED format as was discussed in #core45103 ( also in the second patch ) and excludes JS-only translations from the PO file.

@herregroen
6 years ago

#14 @herregroen
6 years ago

As core now includes unminified JS files as well I've added a minor update that unifies translations of both the minified and unminified files into a single translations file. I will update #core45103 accordingly.

#15 @swissspidy
6 years ago

3876.4.diff contains lots of unrelated code 🤔

@herregroen
6 years ago

@herregroen
6 years ago

#16 @ocean90
6 years ago

We now have the JavaScript strings in the core projects. Worth nothing the Language_Pack command is currently *not* used for core. I have attached a snippet of the current build script in core-language-pack.sh. It uses wp glotpress translation-set export for the export. Instead of trying to do the JS handling in bash we should probably add a WP-CLI command for this.

To avoid unnecessary memory usage, a string that's only used in a JavaScript file should not be part of the main po/mo file.

That's something we have to ignore for core for now because of https://github.com/wp-cli/i18n-command/issues/105.

@herregroen
6 years ago

#17 @herregroen
6 years ago

@ocean90 Just updated the patch to expand the language-pack command to also generate packs for core as is done for plugins and themes.

I think I have everything in the correct format to match https://i18n.svn.wordpress.org/ but could really use a second pair of eyes.

$GP_VER would be passed as the $slug. $GP_LOCALE as --locale=$GP_LOCALE. $GP_TS_SLUG as --locale-slug=$GP_TS_SLUG. Status is always current.

The build_language_packs function does update SVN, but considering the files are there I've left this in place for core as I imagine there's another shell script that does this already which could be replaced like this. Let me know if the process here is different and I can add an exception.

Filtering on $GP_TS_SLUG will only happen if $GP_LOCALE is passed, same as for plugins and themes.

All translation entries are now added to the PO file. I've chosen to keep this consistent between core, plugins and themes but can add an exception if desirable.

#18 @ocean90
6 years ago

  • Owner set to ocean90
  • Status changed from new to reviewing

@herregroen Thank you! The SVN update is fine, it's currently a few lines below the PO/MO part. I'll give this a test tomorrow.

@ocean90
6 years ago

#19 @ocean90
6 years ago

3876.8.diff includes some small fixes.
The remaining issue is that all core files need to be bundled into one ZIP file. I think we can solve this by passing an array with all the data to build_language_packs(). The keys for core would be default, admin, admin-network, and continents-cities. Plugins and themes only have default. This would also allow us to continue using default for $data->domain for core which is required by get_active_language_packs() or insert_language_pack().

@ocean90
6 years ago

#20 @ocean90
6 years ago

Due to the annoying complexity I'm going with 3876.9.diff now which will be used as a simple replacement for core-language-pack.sh.

This ticket was mentioned in Slack in #core-js by omarreiss. View the logs.


6 years ago

This ticket was mentioned in Slack in #core by omarreiss. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-js by herregroen. View the logs.


6 years ago

#24 @herregroen
5 years ago

@ocean90 as translatable strings are now being generated for themes and plugins with https://meta.trac.wordpress.org/changeset/7893 and https://meta.trac.wordpress.org/changeset/7895 I think https://meta.trac.wordpress.org/attachment/ticket/3876/3876.6.diff is the final step to ensure the appropriate JED JSON files are also being server for plugins and themes.

Let me know if I'm missing something or you encounter any issues with the patch and I'll be happy to help however I can.

#25 @dd32
5 years ago

Just noting that #3976 is an outstanding request for this feature.

#26 @herregroen
5 years ago

@ocean90 Do you know if anything beyond https://meta.trac.wordpress.org/attachment/ticket/3876/3876.6.diff is needed to ensure the JSON files are included in the language pack zips?

I don't think they're currently being included yet for plugins and themes. Finishing that should be enough to both close this ticket and #3875.

#27 @timwhitlock
5 years ago

Hi. I don't know if this the right place to comment, but prior to JS translations being available for plugins, would you be open to some comments?

In short: I wonder if some concessions can be made for plugins and themes that only really need a single JSON file.

The combined size of the default text domain's 11 JSON files is ~45K and I'd expect Core to be more complex and have more JS than the vast majority of plugins. (My own uses 33 strings, totalling ~8K)

The new system creates complexities which may be ok for Core development, but many plugin and theme developers looking to use the new system will struggle to prepare their sources. I know this, because I'm already beginning to hear from them.

Ensuring file references match production code will cause some people problems. (e.g. src/library/component.js won't match dist/prod.js). I know this has been considered because WP handles .min.js, but there are many common practices that won't produce that result, and using command line tools to generate PHP files with /* references: */ seems like unnecessary wrangling for simple cases.

I agree that following WordPress standards is a fair requirement for being GlotPress-compatible, and I support that. However, in this case I feel like the requirements are unnecessarily restrictive and error-prone.

Would it be possible to allow plugins and themes to use a single {$slug}-{$locale}.json file? Perhaps electing to do so through some meta declaration, or simply on account of the number, or size of strings? I know that loading such file would also require a change to load_script_translation_file, but would you be against the idea from the GlotPress side?

Last edited 5 years ago by timwhitlock (previous) (diff)

#28 @ocean90
5 years ago

There's is #core45467 and #core45468 which should get fixed first. I also noticed that we create JSON files for source files, files which have /src/ in their path. Those should be excluded for now.

Also related: #3976

#29 @mobby2561
5 years ago

I'm also not sure if this is the right place to comment, but during manual WordPress release package generation, the localized JSON files are not being generated.

This ticket was mentioned in Slack in #core-js by swissspidy. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-js by herregroen. View the logs.


5 years ago

#32 @ocean90
5 years ago

In 8337:

Plugin Directory: Set code_touched for changes in JavaScript files.

See #3876.

#33 @ocean90
5 years ago

In 8338:

Translate: Include JSON translations in language packs.

Props herregroen, ocean90.
See #3876, #3976.

#34 @ocean90
5 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

This ticket was mentioned in Slack in #meta by dd32. View the logs.


5 years ago

Note: See TracTickets for help on using tickets.