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
- Enabled Language and Interface Translation
- Added Romanian
- Found some text that uses plurals: Module %name has been enabled.
- Changed the plurals for these for Romanian
- Enabled some modules (1 module, 2 modules, and 3 modules)
- 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.
Comment | File | Size | Author |
---|---|---|---|
#66 | reroll_diff_59-66.txt | 4.48 KB | ravi.shankar |
#66 | 2660338-66.patch | 18.04 KB | ravi.shankar |
#59 | interdiff-2660338-56-59.txt | 1.75 KB | mohit_aghera |
#59 | 2660338-59.patch | 18.07 KB | mohit_aghera |
Comments
Comment #2
alexpottNo that the major issue is fixed. I think this can be treated as a normal bug.
Comment #4
claudiu.cristea#2766857: Provide a PluralTranslatableMarkup::getPluralVariantIndex() factory method is related.
Comment #5
claudiu.cristeaHere's patch.
Comment #6
claudiu.cristeaSmall improvement.
Comment #9
claudiu.cristeaAdded the missed service in unit tests container.
Comment #10
Kristen PolThanks for the patch. I have reviewed it and don't see any obvious issues though there are some comments that should be changed slightly.
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...
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.
Typo:
local_get_plural => locale_get_plural
Typo:
Local => Locale
I'll test the patch shortly.
Comment #11
Kristen PolI've tested this as follows:
Comment #12
Kristen PolAssigning to myself to make the comment changes. I will try to finish this today.
Comment #13
Kristen PolStill 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.
Comment #15
Kristen PolI reworded some of the comments. Here's the patch and interdiff.
Comment #16
Kristen PolComment #17
claudiu.cristea@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:
Compute > Computes
Comment #21
alberto56 CreditAttribution: alberto56 at Dcycle commentedThis 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.
Comment #22
alexpottI 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.
Comment #23
alexpottHere's #21 implemented but not yet working. This could be becasue of how code / services are loaded.
No interdiff because a different approach.
Comment #26
claudiu.cristeaWe 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()
anddrupal_static_reset()
fromlocale_get_plural()
and this makes this part of the #1577902: [META] Remove all usages of drupal_static() & drupal_static_reset() effort.Comment #27
andypostReroll, looks tests do a lot of mocks for services so instead of strings translatable object returns
Comment #28
andypostDebugging test
\Drupal\Tests\Core\Datetime\DateTest::testFormatInterval()
shows thatcount
passed wrong somehowComment #30
andypostShould fix some tests
Comment #32
andypostDecorated service should implement all public methods
Comment #34
andypostreroll
Comment #38
ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commentedReroll patch for 9.1.x.
Comment #39
ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commentedFixing the error in patch #38.
Comment #40
Kristen PolThanks for the patch. I tried testing it twice (two different installs) but keep getting an error after adding the language:
When the patch is updated, here are a few nitpicks that could be addressed:
Nitpick: Character wrapping (move some words up).
Nitpick: Comment should be a sentence with starting capital and ending period.
Nitpick: More than 80 chars.
Nitpick: Extra empty line.
Comment #41
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedComment #42
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedComment #43
adityasingh CreditAttribution: adityasingh at Srijan | A Material+ Company for Drupal India Association commentedComment #44
adityasingh CreditAttribution: adityasingh as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedUpdated patch as per the changes suggested in #40, please review.
Comment #45
Kristen PolThanks for the update. #44 fixes the nitpicks noted in #40.
Comment #46
Kristen PolTagging for manual testing. I had issues but maybe someone else can try. If you want test steps, see #11.
Comment #47
tanubansal CreditAttribution: tanubansal at Salsa Digital commentedWe 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.
Comment #48
tanubansal CreditAttribution: tanubansal at Salsa Digital commentedComment #49
Kristen PolOk, since @tanubansal also had an issue, moving this back to needs work.
Comment #50
andypostTests failing because of
Comment #51
andypostAttempt to fix #50
Comment #52
quietone CreditAttribution: quietone as a volunteer commentedComment #54
quietone CreditAttribution: quietone as a volunteer commentedPatch fails to apply to 9.2.x setting to NW for that.
The Issue summary needs to have a proposed resolution, adding tag.
Comment #55
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded reroll of patch #51 on Drupal 9.2.x.
Comment #56
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedFixed custom command fails.
Comment #57
mohit_aghera CreditAttribution: mohit_aghera at QED42 commentedComment #58
andypostNew method needs CR also it could help to deprecate locale code a bit
Comment #59
mohit_aghera CreditAttribution: mohit_aghera at QED42 commentedFixing 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.Comment #61
Kristen PolPulling out the failure info:
Related code:
Comment #65
andypostComment #66
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded reroll of patch #59 on Drupal 9.5.x.
Comment #67
andypostLets see what will fail now
Comment #69
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis 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.