Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#45441 closed defect (bug) (fixed)

wp_set_script_translations() does not support text domains other than "messages"

Reported by: swissspidy's profile swissspidy Owned by: ocean90's profile ocean90
Milestone: 5.0.3 Priority: normal
Severity: normal Version: 5.0
Component: I18N Keywords: has-patch
Focuses: javascript Cc:

Description

I'm working on some WP-CLI tooling for the new JS I18N stuff we added in #45103.

While doing so I noticed that \WP_Scripts::print_translations() assumes that strings are in locale_data.messages. However, messages is just the default text domain of a Jed translation file.

Since Jed JSON files can contain translations for multiple text domains, one might want to combine all translations into one for easier maintenance. Especially with #45425.

See https://messageformat.github.io/Jed/ for examples of how these JSON files can look like.

Here's an example from my current WP-CLI script:

{
    "translation-revision-date": "2018-10-21 17:21+0200",
    "generator": "WP-CLI\/2.1.0-alpha-c44d019",
    "domain": "foo-plugin",
    "locale_data": {
        "foo-plugin": {
            "": {
                "domain": "foo-plugin",
                "lang": "de_DE",
                "plural-forms": "nplurals=2; plural=(n != 1);"
            },
            "Source": [
                "Quelle"
            ],
            "URL": [
                "URL"
            ],
            "block name\\u0004Recommendation": [
                "Empfehlung"
            ]
        }
    }
}

You'll get the same result when using the po2json npm script with the --domain argument.

IMO the inline JS added by \WP_Scripts::print_translations() should check whether locale_data.$text_domain exists and fall back to locale_data.messages otherwise.

Also can't hurt to esc_js() the variables there.

Attachments (2)

45441.diff (970 bytes) - added by swissspidy 6 years ago.
45441.2.diff (11.1 KB) - added by ocean90 6 years ago.

Download all attachments as: .zip

Change History (11)

#1 @swissspidy
6 years ago

  • Milestone changed from Awaiting Review to 5.0.1

#2 @pento
6 years ago

  • Milestone changed from 5.0.1 to 5.0.2

#3 @pento
6 years ago

  • Keywords needs-patch added
  • Milestone changed from 5.0.2 to 5.0.3

#4 @audrasjb
6 years ago

  • Milestone changed from 5.0.3 to 5.1

Hello,

5.0.3 is going to be released in a couple of weeks.

It doesn't appear this ticket can be handled in the next couple of weeks. Let's address it in 5.1 which is coming in February. Feel free to ask for changing the milestone if you think this issue can be quickly resolved.

Cheers,

Jb

#5 @swissspidy
6 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from 5.1 to 5.0.3

I think this can land in 5.0.3 just fine. The patch is very simple.

@swissspidy
6 years ago

#6 @desrosj
6 years ago

Since this effects the new JS I18N, let's get this into 5.0.3. @swissspidy are you able to commit this before Monday?

@ocean90
6 years ago

#7 @ocean90
6 years ago

45441.2.diff updates the tests.

#8 @ocean90
6 years ago

In 44403:

I18N/Script Loader: Support text domains other than "messages".

The inline JavaScript added by WP_Scripts::print_translations() should check whether locale_data.$text_domain exists and fall back to locale_data.messages otherwise.

Props swissspidy.
See #45441.

#9 @ocean90
6 years ago

  • Owner set to ocean90
  • Resolution set to fixed
  • Status changed from new to closed

In 44404:

I18N/Script Loader: Support text domains other than "messages".

The inline JavaScript added by WP_Scripts::print_translations() should check whether locale_data.$text_domain exists and fall back to locale_data.messages otherwise.

Merge of [44403] to the 5.0 branch.

Props swissspidy.
Fixes #45441.

Note: See TracTickets for help on using tickets.