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

Asset CDN: Fix json translation loading #10982

Merged
merged 4 commits into from
Dec 18, 2018

Conversation

akirk
Copy link
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 [Type] Bug When a feature is broken and / or not performing as intended [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [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 zinigor, georgestephanis and a team December 16, 2018 09:47
@jetpackbot
Copy link

jetpackbot commented Dec 16, 2018

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
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
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
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
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
Member Author

akirk commented Dec 17, 2018

@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.

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.

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

* 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
Member

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 would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Dec 17, 2018
@zinigor
Copy link
Member

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 would need you to make some changes or provide some more details about your PR. Thank you! labels Dec 18, 2018
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.

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 added a commit that referenced this pull request Jan 3, 2019
jeherve added a commit that referenced this pull request Jan 3, 2019
* 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
Labels
[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 [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Atomic sites are not using WordPress.com translations for Gutenberg
5 participants