-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add API route to download ZIP files of translations #1
Conversation
'action' => 'traduttore_zip_generated', | ||
'description' => __( 'When a new translation ZIP files is built', 'traduttore' ), | ||
'message' => function( $success, GP_Translation_Set $translation_set ) { | ||
if ( ! $success ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be an undefined variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why's that?
This should be the variable passed by the traduttore_zip_generated
hook on line 71.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I only saw the variable once I commented. I thought I deleted the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good.
To generate the zip files from WP-CLI. The value --0=1
is the translation ID. I am not sure how to easily get a list of ID's.
wp cron event schedule traduttore_generate_zip --0=1 --url=translate.required.ch
wp cron event run traduttore_generate_zip --url=translate.required.ch
inc/TranslationApiRoute.php
Outdated
'english_name' => $locale->english_name, | ||
'native_name' => $locale->native_name, | ||
'package' => file_exists( $zip_provider->get_zip_path() ) ? $zip_provider->get_zip_url() : false, | ||
'iso' => array_filter( [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we want to align the values?
} | ||
|
||
// Make sure the cache directory exists. | ||
if ( ! @is_dir( $this->get_cache_dir() ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is @
needed? The result should either true or false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I copied that from core to suppress errors. Not a biggie imho
"keywords": [ | ||
"wordpress", | ||
"glotpress" | ||
], | ||
"require": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a
"suggest": {
"wpackagist-plugin/slack": "Send Slack notification on generation ZIP completion"
}
inc/TranslationApiRoute.php
Outdated
|
||
$zip_provider = new ZipProvider( $set ); | ||
|
||
$result[] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the result only show if a zip file exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's a benefit in that. When the URL isn't set, it's more obvious that the ZIP doesn't exist and WordPress will fail to download it anyway.
Regarding WP-CLI, I could totally add a separate command for this to make generation easier. Or perhaps the 15 minute timeframe needs to be adjusted, I don't know :) |
@ocean90 Can we merge this now? |
I wasn’t able to test the registry yet so I didn’t approve it. |
I've just added a new CLI command that might be useful for testing during development. Example usage: wp @dev --url=https://translate.required.local traduttore generate_zip klingler/klingler-theme
wp @dev --url=https://translate.required.local traduttore generate_zip 12345 Regarding the registry, just note that the GlotPress API needs to be available to external users. Check the Restricted Site Access settings or temporarily disable the plugin altogether. For production we obviously need a better solution that only grants access to the API (and the ZIP files) |
For local development you also have to disable SSL verification on the API request. |
Right. I reported this in wp-cli/language-command#30 as you can't easily do that when calling the API via WP-CLI. |
We can add the IP addresses of the servers to the whitelist. |
That's what I meant with the Restricted Site Access settings :-) |
With this PR, translation information for projects will be available available over
https://translate.required.local/api/translations/<path>
, e.g.https://translate.required.local/api/translations/klingler/klingler-theme
.The format is similar to the one returned by the WordPress.org translations API. This way, WordPress can easily consume this additional data source.
Whenever translations are updated, a cron event is scheduled to generate a ZIP file containing both the PO and MO file for the project.
In the API, this looks as follows:
Our own plugins and themes can use this in combination with https://github.com/wearerequired/traduttore-registry. It would look like this:
After that, when you run
wp language core update
on that site, plugin translations will be loaded from our GlotPress site (given that the IP address is whitelisted or Restrict Site Access is disabled on the GP site).Note that the code is written in such a way that it could be eventually open sourced eventually.
Also note that the registry library doesn't yet fully support themes. The theme transient needs to be filtered first.
I've only really tested this locally and then copied everything over into that registry plugin. So it's definitely possible that something's not working correctly yet :-)