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

i18n: Absorb errors from Jed localization #5481

Merged
merged 1 commit into from
Mar 14, 2018
Merged

Conversation

aduth
Copy link
Member

@aduth aduth commented Mar 7, 2018

Fixes #5459

This pull request seeks to update localization functions to wrap Jed localization functions, as they throw errors on invalid input, which as can be seen in #5459 causes the entire application to crash. The wrapper absorbs the error, logging it to the console, and returning a sensible default (the untranslated singular string or format).

Testing instructions:

Verify that there are no regressions in the display of localized strings.

Extra credit: Verify that, with an invalid string (e.g. missing placeholder value), no crash occurs and the sensible default is shown.

@aduth aduth added the Internationalization (i18n) Issues or PRs related to internationalization efforts label Mar 7, 2018
@bordoni
Copy link
Member

bordoni commented Mar 7, 2018

Without #5168 we will always hit this console.error().
Should it be logged only once?

@aduth
Copy link
Member Author

aduth commented Mar 7, 2018

Without #5168 we will always hit this console.error().
Should it be logged only once?

Can you elaborate here on what you're expecting? There should be no errors, except in the case where there's a faulty string. The Gutenberg strings assign themselves as the default domain. Other plugins which try to provide a domain which don't exist... will error, yes. Arguably this is the correct behavior, because those domain strings are not available.

@aduth aduth added this to the 2.4 milestone Mar 7, 2018
@aduth
Copy link
Member Author

aduth commented Mar 7, 2018

In fact, I worry that #5235 may inadvertently cause some plugins to crash the editor without the changes in this pull request, since I've seen in some cases people using __ as it exists in PHP, where we had not previously supported domain and simply ignored it. Now that we support the argument, at least in passing it through to Jed, these would result in errors.

@aduth
Copy link
Member Author

aduth commented Mar 8, 2018

All the same, since it could become noisy, I've added the "log once" behavior in aec58e6.

@aduth
Copy link
Member Author

aduth commented Mar 8, 2018

See #5489 for closure of #5168.

@aduth aduth force-pushed the fix/5459-absorb-l10n-error branch from aec58e6 to 9021fee Compare March 14, 2018 00:34
@aduth aduth merged commit 817c0e0 into master Mar 14, 2018
@aduth aduth deleted the fix/5459-absorb-l10n-error branch March 14, 2018 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internationalization (i18n) Issues or PRs related to internationalization efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants