Plural formulas are wiped out upon D7 upgrade

[argiepiano]

Description of the bug

When you upgrade a multi-language site that contains plural formulas, those formulas are not transferred to Backdrop.

Steps To Reproduce

1. Do a vanilla install of D7
2. Install module l10n_pconfig. This module provides defaults for plural formulas, and also provides a UI to edit those
3. Install a predefined "standard" language, for example Spanish. The module l10n_pconfig will automatically define a plural formula. You can check this by inspecting the table languages. You'll see values in columns plurals (e.g. 2), and formula (e.g. ($n!=1))
4. Proceed to upgrade your site to Backdrop
   NOTE: In Backdrop, plural formulas are stored in the state table, under name locale_translation_plurals, keyed by langcode.
5. Inspect the state table after the upgrade. You will see the name locale_translation_plurals, but its value is blank.

Actual behavior

The formulas are not transferred.

Expected behavior

The formulas should be transferred. The state table should contain a value for locale_translation_plurals

Additional information

The issue is locale_update_1004(). This is what's happening during the upgrade:

1. update_prepare_language() is called. The state table stores locale_translation_plurals with the correct values from the D7 languages table 👍🏽
2. At some point locale_update_1004() is run. This hook contains a line that wipes out the value previously stored 👎
   state_set('locale_translation_plurals', update_variable_get('locale_translation_plurals', array()));

The issue is that the variable locale_translation_plurals does not exist in D7. It never existed, in any version of D7. The plural formulas have always been stored in the languages table in D7. Thus, since the variable does not exist, state_set() simply wipes out the previously stored value.

This line should be removed.

BTW, the origin of locale_translation_plurals as a state variable is murky. I believe it was present in the first alpha versions of Drupal 8, and was later removed in favor of a config setting. Apparently this never made it into Backdrop.

---

PR: backdrop/backdrop#4399

[avpaderno]

Essentially, the fix is simply removing the state_set('locale_translation_plurals', update_variable_get('locale_translation_plurals', array())); and update_variable_del('locale_translation_plurals'); lines from the locale.install file. Is that correct?

[avpaderno]

Good catch!

I also noticed locale_update_1004() executes update_variable_del('locale_translation_javascript'); but Drupal 7 does not use that persistent variable, nor is state_set('locale_translation_javascript', update_variable_get('locale_translation_javascript', array())); executed by locale_update_1004().
(This indeed needs to go to a different issue.)

[argiepiano]

Good point, @kiamlaluno! Should we tackle that one in this issue as well rather than opening another one?

I agree about the proposed fix.

[argiepiano]

Also, eventually (version 2.0?) it would be better to store the plural formulas and javascript callback as part of locale.settings.json where the rest of the settings for each enabled language is stored.

[avpaderno]

I would keep the issues separated, at least because this issue describes a more serious bug in code that is resetting to an empty array a state value the update from Drupal 7 correctly set; the other issue is just about code that deletes persistent variables that do not exist.

This issue is more important and it should be fixed as soon as possible.

[argiepiano]

PR available for review and testing.

This PR removes the faulty initialization of locale_translation_plurals as well as the update_variable_del() invocation related to this inexistent variable.

[argiepiano]

@kiamlaluno, wondering if you may be available for testing and code-reviewing?

[avpaderno]

I can confirm the PR correctly changes the code.
I can also confirm that, in the code Backdrop is currently using, update_prepare_language() is called before any update hook is invoked and locale_update_1004() is effectively replacing the values in the locale_translation_plurals state value with an empty array. The PR fixes this.

[argiepiano]

Anyone available to do a code review for this upgrade bug? It's already WFM.

[avpaderno]

In my previous comment, I can confirm the PR correctly changes the code. was supposed to mean I reviewed the code; I did not test it. (I apologize I was not clear about that.)

[avpaderno]

(Yes, pr - works for me should have been replaced by the label that means I reviewed the code and did not find any flaw/error.)

[quicksketch]

Thanks @kiamlaluno and @argiepiano! I have merged backdrop/backdrop#4399 into 1.x and 1.25.x for 1.25.2.