Skip to content
This repository was archived by the owner on Sep 28, 2023. It is now read-only.

Add extra check to handle formal and informal locales.#46

Merged
afercia merged 2 commits intomasterfrom
fix-formal-locales-notification
May 3, 2019
Merged

Add extra check to handle formal and informal locales.#46
afercia merged 2 commits intomasterfrom
fix-formal-locales-notification

Conversation

@IreneStr
Copy link
Copy Markdown
Contributor

@IreneStr IreneStr commented Jan 22, 2019

Changelog:

  • Fixes a bug where a 'You're using WordPress in a language we don't support yet.' notice would be shown for formal and informal locales when the locale actually was supported.

Technical choices:

The wp_locale in the translation_sets object do not contain the _formal or _informal part. For example: the wp_locale of Dutch (Formal) is nl_NL. The information that it's formal is only in the slug property. Because this->locale does have the _formal or _informal part, the comparison with the wp_locale had an incorrect result for informal and formal languages. This commit adds an extra check in which the slug is appended to the locale if the slug is not 'default' (i.e. is 'informal' or 'formal'). This string is compared to this->locale to make sure the correct $set is returned.

Test instructions:

On wordpress-seo trunk:

  • Go to settings >General and change the language to Deutsch (Sie).
  • Go to SEO > General. Even though German Formal is 99% translated, you'll see a 'You're using WordPress in a language we don't support yet.' notice on the dashboard.

Copy the code changes from this PR to wordpress-seo's vendor > yoast > i18n-module > src > i18n-module.php.

  • [> 90% translated formal language] (Keep the language set to Deutsch (Sie)) Go to SEO > General. See no translation-related notice.
  • [> 90% translated non-formal language] Set the language to English (US). See no translation-related notice.
  • [partly translated formal language] Set the language to Dutch (Formal). You'll see "Zoals u kunt zien is er een vertaling van deze plugin in Dutch (Formal). Deze vertaling is momenteel voor 57% klaar... ".
  • [partly translated informal language] Set the language to Deutsch (Schweiz, Du). You'll see "Wie du sehen kannst, existiert eine Übersetzung dieses Plugins in German (Switzerland) Informal. Diese Übersetzung ist aktuell zu 61% fertiggestellt. "
  • [partly translated non-formal language] "You're using WordPress in Esperanto. While Yoast SEO has been translated to Esperanto for 8%, it's not been shipped with the plugin yet."

The wp_locale in the translation_sets object do not contain the _formal or _informal part. For example: the wp_locale of Dutch (Formal) is nl_NL. The information that it's formal is only in the `slug` property. Because this->locale does have the _formal or _informal part, the comparison with the wp_locale had an incorrect result for informal and formal languages. This commit adds an extra check in which the slug is appended to the locale if the slug is not 'default' (i.e. is 'informal' or 'formal'). This string is compared to this->locale to make sure the correct $set is returned.
@IreneStr IreneStr force-pushed the fix-formal-locales-notification branch from 5c9609c to 5c14b40 Compare January 22, 2019 15:02
@afercia afercia self-assigned this Apr 26, 2019
@afercia
Copy link
Copy Markdown
Contributor

afercia commented Apr 26, 2019

Testing a bit this, there are 2 important steps to follow to test properly, otherwise you'll get unexpected results:

  • delete the transients each time before refreshing the SEO > General page or after you've changed the code: otherwise you'll end up with a stored transient e.g. _transient_yoast_i18n_wordpress-seo_de_DE_formal that will give you the stored translation set
  • when you install a new language for testing, remember to go to Dashboard > Updates and run the translations updates: for me, the DE_formal mo/po files were missing so some checks (translation_exists and translation_exists) were failing, making the notice still displayed

That said, I think the new check added in this PR should be within the foreach loop? Currently, it's outside of the loop so it always checks the last translation set returned by the api response, which is Zulu 🙂

@afercia afercia assigned IreneStr and unassigned afercia Apr 26, 2019
@IreneStr
Copy link
Copy Markdown
Contributor Author

IreneStr commented May 1, 2019

To delete the transient:

  • vagrant ssh in the WordPress vagrant.
  • cd /srv/www/wordpress-develop/public_html/
  • wp transient delete-all

@IreneStr IreneStr removed their assignment May 1, 2019
@abotteram abotteram self-assigned this May 1, 2019
@abotteram
Copy link
Copy Markdown

CR 👍

Also I acceptance tested. But will let someone else take a look at it as well.

@abotteram abotteram removed their assignment May 1, 2019
@afercia afercia self-assigned this May 3, 2019
@afercia
Copy link
Copy Markdown
Contributor

afercia commented May 3, 2019

Code wise, 👍 However, because of... WordPress 🙂I'm not sure the actual behavior is what we'd want.

Today I've learned that when switching language, WordPress downloads its own .mo/.po files but it doesn't downloads the .mo/.po files for plugins (and themes).

Our I18n-module checks for is_textdomain_loaded() which sets $this->translation_loaded and prints out the notification messages based on that. is_textdomain_loaded() returns true when the related .mo file exists.

Thus, after switching language $this->translation_loaded is false and the printed messages are misleading. Note that $this->translation_loaded is true only after users go in the Updates page and click "Update Translations" (this actually downloads the plugins/themes .mo/.po files).

To reproduce:

  • on WordPress trunk, delete the whole build/wp-content/languages directory
  • activate Yoast SEO and, to double check, also another plugin e.g. WooCommerce
  • go to Settings > General Settings
  • change language to Deutsch (Sie) and save
  • observe the build/wp-content/languages directory and see only the WordPress de_DE_formal .mo/.po files have been downloaded
  • go to SEO > General
  • see the wrong notification message:

without mo po

  • go to the Updates page and update the translations (click "Übersetzungen aktualisieren")
  • observe the build/wp-content/languages directory and see two new sub-directories have been created: plugins and themes
  • the plugins directory finally contains the wordpress-seo de_DE_formal files
  • go to SEO > General
  • see there's no notification message: this is the expected behavior:

with mo po

This happens with all languages: the wrong notification message is displayed until users update the translations and the .mo/.po files get actually downloaded.

Dutch formal without .mo/.po files:

dutch without mo po

Esperanto without .mo/.po files:

esperanto without mo po

Not sure why WordPress doesn't immediately downloads the plugins/themes translations, will ask upstream. From our side, in this scenario the messages that depends on $this->translation_loaded will be misleading. Maybe we should suggest users to update the translations first?

@afercia
Copy link
Copy Markdown
Contributor

afercia commented May 3, 2019

Thanks to @swissspidy for shedding some light on the WordPress behavior. Looks like it always worked this way: changing language in Settings -> General doesn't handle plugin/theme language packs. This part should be addressed upstream.

From our side we should probably change the notification messages because especially the it's not been shipped with the plugin yet is misleading when there are no mo/po files yet.

I'd lean towards merging this PR because it's an improvement anyways and create a new issue.

@afercia afercia merged commit 36f0a12 into master May 3, 2019
@afercia afercia removed the has patch label May 3, 2019
@afercia afercia removed their assignment May 3, 2019
@afercia afercia added this to the Next release milestone May 3, 2019
@IreneStr IreneStr deleted the fix-formal-locales-notification branch May 6, 2019 07:27
@IreneStr IreneStr modified the milestones: Next release, 3.1.0 May 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants