Follow-up to #2573975: function_exists check in PluralTranslatableString is wrong and #2571375: Remove TranslationManager dependency from LanguageManager

Problem/Motivation

A core class shouldn't refer to code provided by a module. We should somehow be able to this and reverse the dependencies so core has no knowledge of the locale module.

Steps to reproduce

  1. Enabled Language and Interface Translation
  2. Added Romanian
  3. Found some text that uses plurals: Module %name has been enabled.
  4. Changed the plurals for these for Romanian
  5. Enabled some modules (1 module, 2 modules, and 3 modules)
  6. The messages were as expected (see screenshots below) (though I'm not sure what the 2. plural is for in Romanian)

Taken from #11.

Proposed resolution

tbd

Remaining tasks

User interface changes

tbd. Should be none.

API changes

tbd. Maybe.

Data model changes

tbd. Should be none.

CommentFileSizeAuthor
#66 reroll_diff_59-66.txt4.48 KBravi.shankar
#66 2660338-66.patch18.04 KBravi.shankar
#59 interdiff-2660338-56-59.txt1.75 KBmohit_aghera
#59 2660338-59.patch18.07 KBmohit_aghera
#56 interdiff_55-56.txt1.34 KBravi.shankar
#56 2660338-56.patch16.93 KBravi.shankar
#55 rawdiff.txt4.51 KBravi.shankar
#55 2660338-55.patch16.9 KBravi.shankar
#51 2660338-51.patch16.91 KBandypost
#51 interdiff.txt930 bytesandypost
#47 Screen Shot 2020-07-10 at 3.39.47 PM.png326.72 KBtanubansal
#47 Screen Shot 2020-07-10 at 3.34.28 PM.png328.69 KBtanubansal
#47 Screen Shot 2020-07-10 at 3.33.21 PM.png239.26 KBtanubansal
#44 interdiff_39-44.txt2.33 KBadityasingh
#44 2660338-44.patch14.17 KBadityasingh
#39 interdiff_38-39.txt624 bytesridhimaabrol24
#39 2660338-39.patch14.3 KBridhimaabrol24
#38 2660338-38.patch13.69 KBridhimaabrol24
#34 2660338-32-reroll.patch13.68 KBandypost
#32 2660338-32.patch13.67 KBandypost
#32 interdiff.txt665 bytesandypost
#30 2660338-29.patch13.48 KBandypost
#30 interdiff.txt1.9 KBandypost
#27 2660338-27.patch11.58 KBandypost
#23 2660338-23.patch11.58 KBalexpott
#15 interdiff-2660338-9-15.txt4.12 KBKristen Pol
#15 drupal-plural_variant_index-2660338-15.patch10.03 KBKristen Pol
#11 drupal-plural-ro-first-3-modules.png182.45 KBKristen Pol
#11 drupal-plural-ro-first-2-modules.png169.19 KBKristen Pol
#11 drupal-plural-ro-singular.png164.19 KBKristen Pol
#11 drupal-plural-module-update-text.png284.29 KBKristen Pol
#9 2660338-9.patch10.11 KBclaudiu.cristea
#9 interdiff.txt5.27 KBclaudiu.cristea
#6 interdiff.txt903 bytesclaudiu.cristea
#6 2660338-6.patch4.84 KBclaudiu.cristea
#5 2660338-5.patch4.88 KBclaudiu.cristea
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Priority: Major » Normal

No that the major issue is fixed. I think this can be treated as a normal bug.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

claudiu.cristea’s picture

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
4.88 KB

Here's patch.

claudiu.cristea’s picture

FileSize
4.84 KB
903 bytes

Small improvement.

The last submitted patch, 5: 2660338-5.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: 2660338-6.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
5.27 KB
10.11 KB

Added the missed service in unit tests container.

Kristen Pol’s picture

Thanks for the patch. I have reviewed it and don't see any obvious issues though there are some comments that should be changed slightly.

  1. +++ b/core/core.api.php
    @@ -2236,6 +2236,61 @@ function hook_validation_constraint_alter(array &$definitions) {
    + * implementation should return a valid zero-based positive integer or nothing,
    

    I had not heard "zero-based positive integer" used before. I googled it and it does appear to be used but it might be better to change to something like:

    ...implementation should return zero, a positive integer, or...

  2. +++ b/core/core.api.php
    @@ -2236,6 +2236,61 @@ function hook_validation_constraint_alter(array &$definitions) {
    + * if he wants to pass the decision to core or other modules. Returning anything
    

    The "he" should be changed to be gender neutral. In other places "you" is used. Or, the sentence could be restructured something like:

    The implementation should return zero, a positive integer, or, to not impact the results, nothing.

  3. +++ b/core/core.api.php
    @@ -2236,6 +2236,61 @@ function hook_validation_constraint_alter(array &$definitions) {
    + * can provide your own implementation, in your custom module, when you don't
    

    Typo:

    local_get_plural => locale_get_plural

  4. +++ b/core/core.api.php
    @@ -2236,6 +2236,61 @@ function hook_validation_constraint_alter(array &$definitions) {
    + *   example the Local module implementation locale_plural_variant_index() is
    

    Typo:

    Local => Locale

I'll test the patch shortly.

Kristen Pol’s picture

I've tested this as follows:

  1. Enabled Language and Interface Translation
  2. Added Romanian
  3. Found some text that uses plurals: Module %name has been enabled.
  4. Changed the plurals for these for Romanian
  5. Enabled some modules (1 module, 2 modules, and 3 modules)
  6. The messages were as expected (see screenshots below) (though I'm not sure what the 2. plural is for in Romanian)

Kristen Pol’s picture

Assigned: Unassigned » Kristen Pol
Status: Needs review » Needs work

Assigning to myself to make the comment changes. I will try to finish this today.

Kristen Pol’s picture

Still plan on working on this but the day got away from me. Crossing fingers for tomorrow. If I don't finish by Friday EOD, then I'll take my name off.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Kristen Pol’s picture

I reworded some of the comments. Here's the patch and interdiff.

Kristen Pol’s picture

Assigned: Kristen Pol » Unassigned
Status: Needs work » Needs review
claudiu.cristea’s picture

@Kristen Pol, thank you for your work and review. Sorry for my poor English. I think the patch looks good. I'm not very sure yet about the solution: if invoking a hook is the way to go. Also I don't find it bad. Probably in 99.99% of the cases nobody will implement such a hook and we'll live with the only implementation which is locale_plural_variant_index(). Normally we should offer also an "alter hook" but for me that sounds a little bit over-engineering.

Nit:

+++ b/core/core.api.php
@@ -2247,6 +2247,63 @@ function hook_validation_constraint_alter(array &$definitions) {
+ * Compute the plural variant index.

Compute > Computes

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alberto56’s picture

This might be related to #2273889: Don't use one language's plural index formula with another language's string in the case of untranslated strings using format_plural()., in the sense that computing the plural index requires more information than just the target language -- we also need to know the string itself: if no translation exists for a string, we should not be using the target language plural index, but rather -1, it seems. Perhaps the string itself could be passed to hook_plural_variant_index, so that the locale_plural_variant_index could then check if the string is, in fact, translated; if it is not then the plural variant index would always be -1.

alexpott’s picture

I don't think a new hook is a way to do this. I think PluralTranslatableMarkup should be asking the TranslationManager for the plurals and then when locale is enabled it should be wrapping the TranslationManager service and enhancing the getPluralIndex method. A new hook adds an extension a new extension point that's not needed.

alexpott’s picture

Here's #21 implemented but not yet working. This could be becasue of how code / services are loaded.

No interdiff because a different approach.

Status: Needs review » Needs work

The last submitted patch, 23: 2660338-23.patch, failed testing. View results

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

claudiu.cristea’s picture

We can also deprecate locale_get_plural() in the scope of this issue as in #23, the procedural function became only a wrapper of the service method call.

Also this approach help us also to remove usages of drupal_static() and drupal_static_reset() from locale_get_plural() and this makes this part of the #1577902: [META] Remove all usages of drupal_static() & drupal_static_reset() effort.

andypost’s picture

Reroll, looks tests do a lot of mocks for services so instead of strings translatable object returns

andypost’s picture

Debugging test \Drupal\Tests\Core\Datetime\DateTest::testFormatInterval() shows that count passed wrong somehow

8) Drupal\Tests\Core\Datetime\DateTest::testFormatInterval with data set #15 (7344000, 3, '2 months 3 weeks 4 days')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'2 months 3 weeks 4 days'
+'1 month 1 week 1 day'

Status: Needs review » Needs work

The last submitted patch, 27: 2660338-27.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

andypost’s picture

Status: Needs work » Needs review
FileSize
1.9 KB
13.48 KB

Should fix some tests

Status: Needs review » Needs work

The last submitted patch, 30: 2660338-29.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

andypost’s picture

Status: Needs work » Needs review
FileSize
665 bytes
13.67 KB

Decorated service should implement all public methods

Status: Needs review » Needs work

The last submitted patch, 32: 2660338-32.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

andypost’s picture

Status: Needs work » Needs review
FileSize
13.68 KB

reroll

Status: Needs review » Needs work

The last submitted patch, 34: 2660338-32-reroll.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

ridhimaabrol24’s picture

Status: Needs work » Needs review
FileSize
13.69 KB

Reroll patch for 9.1.x.

ridhimaabrol24’s picture

Fixing the error in patch #38.

Kristen Pol’s picture

Status: Needs review » Needs work

Thanks for the patch. I tried testing it twice (two different installs) but keep getting an error after adding the language:

LogicException: The database connection is not serializable. This probably means you are serializing an object that has an indirect reference to the database connection. Adjust your code so that is not necessary. Alternatively, look at DependencySerializationTrait as a temporary solution. in Drupal\Core\Database\Connection->__sleep() (line 1725 of /var/lib/tugboat/stm/web/core/lib/Drupal/Core/Database/Connection.php).

When the patch is updated, here are a few nitpicks that could be addressed:

  1. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
    @@ -109,4 +109,22 @@ public function translateString(TranslatableMarkup $translated_string);
    +   *   Optional language code to translate to a language other than
    +   *   what is used to display the page.
    

    Nitpick: Character wrapping (move some words up).

  2. +++ b/core/modules/locale/locale.services.yml
    @@ -37,3 +37,9 @@ services:
    +    # pass the old service as an argument
    

    Nitpick: Comment should be a sentence with starting capital and ending period.

  3. +++ b/core/modules/locale/src/LocaleTranslationManager.php
    @@ -0,0 +1,142 @@
    +      // If there is a plural formula for the language, evaluate it for the given
    

    Nitpick: More than 80 chars.

  4. +++ b/core/modules/locale/src/LocaleTranslationManager.php
    @@ -0,0 +1,142 @@
    +
    +
    +  /**
    

    Nitpick: Extra empty line.

ravi.shankar’s picture

Assigned: Unassigned » ravi.shankar
ravi.shankar’s picture

Assigned: ravi.shankar » Unassigned
adityasingh’s picture

Assigned: Unassigned » adityasingh
adityasingh’s picture

Assigned: adityasingh » Unassigned
Status: Needs work » Needs review
FileSize
14.17 KB
2.33 KB

Updated patch as per the changes suggested in #40, please review.

Kristen Pol’s picture

Issue tags: +Bug Smash Initiative

Thanks for the update. #44 fixes the nitpicks noted in #40.

Kristen Pol’s picture

Issue tags: +Needs manual testing

Tagging for manual testing. I had issues but maybe someone else can try. If you want test steps, see #11.

tanubansal’s picture

We have followed the steps given in #11 and after adding the patch mentioned #44 (https://www.drupal.org/pift-ci-job/1751390). While enabling singular/plural modules , its showing 'Site encountered error'. When we refresh the modules page then it should that the module is enable. Please refer the attached screenshots below.

tanubansal’s picture

Assigned: tanubansal » Unassigned
Kristen Pol’s picture

Status: Needs review » Needs work

Ok, since @tanubansal also had an issue, moving this back to needs work.

andypost’s picture

Tests failing because of

Exception: TypeError: Argument 8 passed to Drupal\help_topics\Plugin\HelpSection\HelpTopicSection::__construct() must be an instance of Drupal\Core\StringTranslation\TranslationManager, instance of Drupal\locale\LocaleTranslationManager given, called in /var/www/html/core/modules/help_topics/src/Plugin/HelpSection/HelpTopicSection.php on line 128
Drupal\help_topics\Plugin\HelpSection\HelpTopicSection->__construct()() (Line: 107)
andypost’s picture

Status: Needs work » Needs review
FileSize
930 bytes
16.91 KB

Attempt to fix #50

quietone’s picture

Issue summary: View changes

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

quietone’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll, +Needs issue summary update

Patch fails to apply to 9.2.x setting to NW for that.

The Issue summary needs to have a proposed resolution, adding tag.

ravi.shankar’s picture

Added reroll of patch #51 on Drupal 9.2.x.

ravi.shankar’s picture

Fixed custom command fails.

mohit_aghera’s picture

Assigned: Unassigned » mohit_aghera
andypost’s picture

New method needs CR also it could help to deprecate locale code a bit

mohit_aghera’s picture

Fixing test cases.
All the test cases failing in #56 are passing on local now.

InstallerExistingConfigTest test is something that is passing without any change. So not sure if past failure was due to other reasons.

Status: Needs review » Needs work

The last submitted patch, 59: 2660338-59.patch, failed testing. View results

Kristen Pol’s picture

Pulling out the failure info:

There was 1 failure:
1) Drupal\Tests\locale\Functional\LocalePluralFormatTest::testGetPluralFormat
Plural translation of 1 hours / @count hours for count 0 in fr is 0 heure
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'0 heure'
+'0 heures'

Related code:

./core/modules/locale/tests/src/Functional/LocalePluralFormatTest.php:        $this->assertSame($expected_plural_string, \Drupal::translation()->formatPlural($count, '1 hour', '@count hours', [], ['langcode' => $langcode])->render(), 'Plural translation of 1 hours / @count hours for count ' . $count . ' in ' . $langcode . ' is ' . $expected_plural_string);

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

ravi.shankar’s picture

Added reroll of patch #59 on Drupal 9.5.x.

andypost’s picture

Status: Needs work » Needs review

Lets see what will fail now

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Status: Needs review » Needs work

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

For the IS update and change record

Did not test or review yet.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.