Skip to content

Asset CDN: Fix json translation loading#10982

Merged
jeherve merged 4 commits intomasterfrom
fix/cdn-broken-json-translation-loading
Dec 18, 2018
Merged

Asset CDN: Fix json translation loading#10982
jeherve merged 4 commits intomasterfrom
fix/cdn-broken-json-translation-loading

Conversation

@akirk
Copy link
Copy Markdown
Member

@akirk akirk commented Dec 16, 2018

When the static asset CDN of Jetpack is loaded, the translations for Gutenberg don'tt load properly because the filename is generated for a wrong path.

Also fixes Automattic/wp-calypso#29309

This has been extensively discussed in https://core.trac.wordpress.org/ticket/45528

Changes proposed in this Pull Request:

  • This implements the filter that has been introduced into Core.

Testing instructions:

Prequisit: WordPress 5.0.2(alpha) or trunk

  1. Set your WordPress to another language than English.
  2. Activate Jetpack Static Asset CDN.
  3. Potentially add a filter to your WordPress if it's running trunk (see Create plugin support for CDN'ing of static assets. #10102)
  4. Create a new post to open Gutenberg and observe broken translations.
  5. Activate the patch: Translations should now load after a reload.

Proposed changelog entry for your changes:

  • Fixes loading of the Block Editor UI translations when Static Asset CDN is used (requires WordPress 5.0.2)

@akirk akirk added Bug When a feature is broken and / or not performing as intended [Status] Needs Review This PR is ready for review. [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Feature] Asset CDN labels Dec 16, 2018
@akirk akirk self-assigned this Dec 16, 2018
@akirk akirk requested review from a team, georgestephanis and zinigor December 16, 2018 09:47
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Dec 16, 2018 •
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: January 10, 2019.
Scheduled code freeze: January 3, 2019

Generated by 🚫 dangerJS against ce2839f

@jeherve jeherve added the [Focus] i18n Internationalization / i18n, adaptation to different languages label Dec 17, 2018
@jeherve jeherve added this to the 6.9 milestone Dec 17, 2018
@jeherve
Copy link
Copy Markdown
Member

jeherve commented Dec 17, 2018

I am not quite sure how to test this. When running a development version of WordPress, the Asset CDN is not applied because that version of WP is not available in a tag. I can use the jetpack_cdn_core_version_and_locale filter to fake a stable release, like so:

add_filter( 'jetpack_cdn_core_version_and_locale', function( $values ) {
	return array( '5.0.1', 'fr_FR' );
} );

But if I do so, the editor does not load at all for me because I am trying to load the block editor from the Alpha version of WP with some outdated JS from the 5.0.1 version of WP.

image

How did you manage to test this? Am I missing something here?

@akirk
Copy link
Copy Markdown
Member Author

akirk commented Dec 17, 2018

Hm, at the time when I tested this, the 5.0.1 cdn version was compatible. I guess this means that it's now only possible to test this with a 5.0.1 install and not an alpha.

@jeherve
Copy link
Copy Markdown
Member

jeherve commented Dec 17, 2018

👍 So I should have a 5.0.1 + manually apply the patch in https://core.trac.wordpress.org/changeset/44209 ?

@akirk
Copy link
Copy Markdown
Member Author

akirk commented Dec 17, 2018

Yes, exactly!

@akirk akirk force-pushed the fix/cdn-broken-json-translation-loading branch from 9b1bfdb to a66d67e Compare December 17, 2018 13:17
@akirk
Copy link
Copy Markdown
Member Author

akirk commented Dec 17, 2018 •
edited
Loading

@jeherve I updated the patch, the filter was missing two parameters which caused a fatal. Maybe that broke Gutenberg for you? I just tried with a trunk WordPress and

add_filter( 'jetpack_cdn_core_version_and_locale', function( $values ) {
	return array( '5.0.1', 'de_DE' );
} );

and it loads up fine for me.

jeherve
jeherve requested changes Dec 17, 2018
View reviewed changes
Copy link
Copy Markdown
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.

It works well for me. There is just a tiny indenting issue!

modules/photon-cdn.php Outdated
* Ensure use of the correct relative path when determining the JavaScript file names.
*
* @param string $relative The relative path of the script. False if it could not be determined.
* @param string $src The full source url of the script.
Copy link
Copy Markdown
Member

@jeherve jeherve Dec 17, 2018

Choose a reason for hiding this comment

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

Spacing is odd here, I think there may be spaces instead of tabs.

@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Dec 17, 2018
@zinigor
Fixing indentation.
3adae0c
@zinigor
Merge branch 'fix/cdn-broken-json-translation-loading' of github.com:…
ce2839f 
…Automattic/jetpack into fix/cdn-broken-json-translation-loading
@zinigor
Copy link
Copy Markdown
Contributor

zinigor commented Dec 18, 2018

@jeherve the indentation should be fixed now, thanks for testing!

@zinigor zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Dec 18, 2018
jeherve
jeherve approved these changes Dec 18, 2018
View reviewed changes
Copy link
Copy Markdown
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.

Working well. Merging.

@jeherve jeherve merged commit 8bb8a1b into master Dec 18, 2018
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Dec 18, 2018
@jeherve jeherve deleted the fix/cdn-broken-json-translation-loading branch December 18, 2018 13:14
jeherve added a commit that referenced this pull request Dec 19, 2018
@jeherve
Changelog: add #10982
6e098b9
@jeherve jeherve mentioned this pull request Dec 19, 2018
Closed
jeherve added a commit that referenced this pull request Jan 3, 2019
@jeherve
Changelog: add #10982
a8a7025
jeherve added a commit that referenced this pull request Jan 3, 2019
@jeherve
Jetpack 6.9: Changelog and testing list (#10722)
8d91603 
* Add first version of the Changelog and testing list for 6.9

* Changelog: add #10710

* changelog: add #10538

* changelog: add #10741

* changelog: add #10749

* changelog: add #10664

* changelog: add #10224

* changelog: add #10788

* Changelog: add #10560

* Chanegelog: add #10812

* changelog: add #10556

* Changelog: add #10668

* Changelog: add #10846

* Changelog: add #10947

* Changelog: add #10962

* Changelog: add #10956

* Changelog: add #10940

* Changelog: add #10934

* Changelog: add #10912

* changelog: add #10866

* changelog: add #10924

* Changelog: add #10936

* Changelog: add #10833

* changelog: add #10867

* Changelog: add #10960

* Changelog: add #10888

* changelog: add #10840

* changelog: add #10972

* Changelog: add #10979

* changelog: add #10909

* Changelog: add #10958

* Changelog: add #10981

* Changelog: add #10564

* Changelog: add #10809

* Changelog: add #10982

* Changelog: add #10706

* Changelog: add #10978

* Changelog: add #10132

* Changelog: add #11022

* Changelog: add #11024

* Changelog: add #10875

* Changelog: add #11030

* Changelog: add #11053

* Changelog: add #10880

* Changelog: add #9359

* Changelog: add #11037

* Update block list

* Changelog: add #11060

* Changelog: add #10755

* changelog: add #11000

* Changelog: add #10786

* Changelog: add #10945

* Changelog: add #10597
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeherve jeherve jeherve approved these changes

@zinigor zinigor Awaiting requested review from zinigor

@georgestephanis georgestephanis Awaiting requested review from georgestephanis

Assignees

@akirk akirk

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Asset CDN [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Focus] i18n Internationalization / i18n, adaptation to different languages

Projects

None yet

Milestone

6.9

Development

Successfully merging this pull request may close these issues.

Atomic sites are not using WordPress.com translations for Gutenberg

5 participants

@akirk @jetpackbot @jeherve @zinigor @matticbot