Skip to content

Ensure correct translation loading when loading assets via CDN #14797

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.

Sign up for GitHub

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

Jump to bottom
Merged
jeherve merged 3 commits into from
Mar 5, 2020
Merged

Ensure correct translation loading when loading assets via CDN #14797

jeherve merged 3 commits into from
Mar 5, 2020

Conversation

@akirk
Copy link
Member

@akirk akirk commented Feb 25, 2020 •
edited
Loading

When activating the CDN, the Jetpack blocks don't load their translations properly. This is because Core language loading assumes local files. This fixes the paths under the hood before the JSONs are loaded.

Testing instructions:

  1. Change your UI language to something other than English (with sufficient translation percentage, e.g. German, Spanish, or French).
  2. Activate Jetpack CDN in the Jetpack Settings.
  3. Create a new post with the Gutenberg editor.
  4. Click "Add New Block" and scroll down to the Jetpack blocks. Block names and descriptions should be translated.

Proposed changelog entry for your changes:

  • Fixed Translation loading of Gutenberg blocks with activated CDN.

@akirk akirk added the [Focus] i18n Internationalization / i18n, adaptation to different languages label Feb 25, 2020
@akirk akirk requested review from jeherve and zinigor February 25, 2020 17:13
@akirk akirk self-assigned this Feb 25, 2020
akirk
akirk commented Feb 25, 2020
View reviewed changes
modules/photon-cdn.php Outdated
public static function fix_local_script_translation_path( $file, $handle, $domain ) {
switch ( $handle ) {
case 'jetpack-blocks-editor':
return str_replace( 'wp-content/languages/jetpack', 'wp-content/languages/plugins/jetpack', $file );
Copy link
Member Author

@akirk akirk Feb 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hairy thing here is that we need to rewrite only the plugin translations. It looks like core assumes that these CDN-ed plugins have their translations in the core folder and not in the plugins folder.

@jetpackbot
Copy link
Collaborator

jetpackbot commented Feb 25, 2020 •
edited
Loading

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: March 3, 2020.
Scheduled code freeze: February 25, 2020

Generated by 🚫 dangerJS against 434cf50

@akirk akirk added the [Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins label Feb 25, 2020
@akirk akirk added the [Status] Needs Review This PR is ready for review. label Feb 26, 2020
@akirk akirk marked this pull request as ready for review February 26, 2020 09:18
@akirk akirk requested a review from a team February 26, 2020 09:18
@kraftbj kraftbj added [Feature] Asset CDN and removed [Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins [Status] Needs Review This PR is ready for review. labels Mar 5, 2020
kraftbj
kraftbj approved these changes Mar 5, 2020
View reviewed changes
Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This worked well for me. If testing, be sure to have your translations packages fully updated and you're dropping the PHP on a stable version of the plugin so the CDN would fire for the assets.

@kraftbj kraftbj added the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 5, 2020
@kraftbj kraftbj added this to the 8.4 milestone Mar 5, 2020
jeherve
jeherve approved these changes Mar 5, 2020
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well on my end as well. Merging.

@jeherve jeherve merged commit 7988d52 into master Mar 5, 2020
@jeherve jeherve deleted the fix/cdn-script-translations-loading branch March 5, 2020 17:45
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 5, 2020
jeherve added a commit that referenced this pull request Mar 20, 2020
@jeherve
Changelog;: add #14797
2cfea13
jeherve added a commit that referenced this pull request Mar 31, 2020
@jeherve
8.4 Changelog (#15075)
0eb0b35 
* Initial changelog entry

* Changelog: add #14904

* Changelog: add #14910

* Changelog: add #14913

* Changelog: add #14916

* Changelog: add #14922

* Changelog: add #14924

* Changelog: add #14925

* Changelog: add #14928

* Changelog: add #14840

* Changelog: add #14841

* Changelog: add #14842

* Changelog: add #14826

* Changelog: add #14835

* Changelog: add #14859

* Changelog: add #14884

* Changelog: add #14888

* Changelog: add #14817

* Changelog: add #14814

* Changelog: add #14819

* Changelog;: add #14797

* Changelog: add #14798

* Changelog: add #14802

* Changelog: add #13676

* Changelog: add #13744

* Changelog: add #13777

* Changelog: add #14446

* Changelog: add #14739

* Changelog: add #14770

* Changelog: add #14784

* Changelog: add #14897

* Changelog: add #14898

* Changelog: add #14968

* Changelog: add #14985

* Changelog: add #15044

* Changelog: add #15052

* Update to remove Podcast since it remains in Beta

* Changelog: add #14803

* Changelog: add #15028

* Changelog: add #15065

* Changelog:add #14886

* Changelog: add #15118

* Changelog: add #14990

* Changelog: add #14528

* Changelog: add #15120

* Changelog: add #15126

* Changelog: add #15049

* Chanegelog: add #14852

* Changelog: add #15090

* Changelog: add #15138

* Changelog: add #15124

* Changelog:add #15055

* Changelog: add #15017

* Changelog: add #15109

* Changelog: add #15145

* Changelog:add #15096

* Changelog:add #15153

* Changelog: add #15133

* Changelog: add #14960

* Changelog: add #15127

* Changelog: add #15056

* Copy current changelog to changelog archive.

* Clarify changelog description
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kraftbj kraftbj kraftbj approved these changes

@jeherve jeherve jeherve approved these changes

@zinigor zinigor Awaiting requested review from zinigor

Assignees

@akirk akirk

Labels

[Feature] Asset CDN [Focus] i18n Internationalization / i18n, adaptation to different languages

Projects

None yet

Milestone

8.4

Development

Successfully merging this pull request may close these issues.

6 participants

@akirk @jetpackbot @kraftbj @jeherve @matticbot