In case if there are more than one plural form in the language Drupal.formatPlural would not pass arguments to translation function. See code fragment from drupal.js below.

  else {
    args['@count['+ index +']'] = args['@count'];
    delete args['@count'];
    return Drupal.t(plural.replace('@count', '@count['+ index +']')); <<<<< Bug Here, "args" are missing!
  }
Files: 
CommentFileSizeAuthor
#97 504506-formatPlural-d6.patch3.3 KBandypost
#90 formatPlural_followup-504506-90.patch1.38 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 29,912 pass(es).
[ View ]
#89 formatPlural_followup-504506-89.patch1.82 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 29,910 pass(es).
[ View ]
#77 504506-formatPlural-followup-drupal.patch25.53 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 29,399 pass(es).
[ View ]
#67 504506-formatPlural-followup-drupal.patch25.53 KBwebchick
PASSED: [[SimpleTest]]: [MySQL] 31,500 pass(es).
[ View ]
#59 504506-formatPlural-followup-drupal-d7.patch1.09 KBpodarok
#50 504506-formatPlural-followup-d7.patch1.14 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 30,362 pass(es).
[ View ]
#45 504506-formatPlural-d6.patch3.81 KBandypost
#45 504506-formatPlural-followup-d7.patch1.14 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 504506-formatPlural-followup-d7.patch.
[ View ]
#32 fbss.ru_.po1018 bytesandypost
#26 504506-formatPlural-d6.patch3.84 KBandypost
#24 504506-formatPlural-d6.patch3.82 KBandypost
#19 504506-formatPlural-d6.patch3.03 KBandypost
#6 504506-formatPlural-d6.patch3.03 KBandypost
#6 504506-formatPlural-d7.patch3.1 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 504506-formatPlural-d7_0.patch.
[ View ]
#3 504506-formatPlural-d6.patch2.84 KBandypost
#3 504506-formatPlural-d7.patch2.91 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 20,429 pass(es).
[ View ]

Comments

Version:6.12» 7.x-dev

I can confirm. Bumping to 7.x, we try to fix bugs in the current development version, before backporting the fixes.

Bug exists in 6 and 7.

Missing args parameter does not solve this problem because of difference of Drupal.t() and _locale_rebuild_js()

Drupal.t() make search for string in plain array but _locale_rebuild_js() stores plurals in array keyed by singular

I have noidea how to write tests for this :( But going to fix

PS: this hits me with http://drupal.org/project/facebook_status and russin language which have nplurals=3. But it seem that wold be broken and for nplurals=2

Status:Active» Needs review
StatusFileSize
new2.91 KB
PASSED: [[SimpleTest]]: [MySQL] 20,429 pass(es).
[ View ]
new2.84 KB

I see only two solutions but both require changes in format of Drupal.locale.strings array

here is a patches for simplest which changes the format of Drupal.locale.strings array

It's uses an array without grouping plural strings under singular key to pass to Drupal.locale.strings array this is a currents Drupal.t() implementation in D6 and D7

{"source 1": "translation @count", "source @count": "translation @count", "source @count[2]": "translation @count[2]"}

This approach simplifies a query - removes order and 2 fields

Another approach is supposing to rewrite Drupal.t() and Drupal.formatPlural() to pass index of plural from Drupal.formatPlural to Drupal.t() to make .t() get right transtation from strings array
Also it require change _locale_rebuild_js() to store plurals with n>1 under the same singular key so this adds a lot of complexity to searching for parent of plural which probably could not be translated ...

Status:Needs review» Active

@andypost: no test needed, this is JavaScript and we don't have a test framework for it :(

Indeed, plural forms generated by _locale_rebuild_js() are *severely* broken. We don't need this crap (not sure how long it has been broken!).

Status:Active» Needs review

@andypost: sorry, crossposted. The patch looks great to me. Could you please just add a t.translation IS NULL condition to the query and remove the sad if (!empty($data->translation))?

StatusFileSize
new3.1 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 504506-formatPlural-d7_0.patch.
[ View ]
new3.03 KB

Changed LEFT JOIN to INNER JOIN and add condition t.translation IS NOT NULL (suppose you mean NOT NULL :) to exclude strings without translation.

Also I think translations could be specially set to "" (empty string) so NOT NULL will not exclude them

EDIT: suppose, last 3 years nobody never tried to translate javascript plurals... which seems terrible to me too!

Status:Needs review» Reviewed & tested by the community

I'm not 100% clear on the correctness of this patch without writing some local test code for it, but it looks good. From looking at the code, I can't see any problem with this simplified approach. I'm marking this RTBC so other people will hopefully comment.

I, too, am puzzled that we didn't ran into this sooner.

Status:Reviewed & tested by the community» Fixed

Alright, committed to CVS HEAD. Thanks all.

Version:7.x-dev» 6.x-dev
Status:Fixed» Patch (to be ported)

To be ported to D6.

Status:Patch (to be ported)» Needs review

There is a Drupal 6 patch already.

subscribing...

subscribing

subscribing

subscribing

Subscribing for final solution. However, patch in #306715: _locale_rebuild_js doesn't store plural forms correctly works for me.

@sgabe Please, test a patch from #6, it's a bit different from what you pointed.

StatusFileSize
new3.03 KB

Same patch against current D-6

@andypost: Patch in #19 applies fine and seems to work for me.

#19 works fine for me too.

Status:Needs review» Needs work

Let's make sure we regenerate all those language files (we need an update function for that), and we are good to go.

I found no code that clean languages folder...

drupal_clear_js_cache() just clean preprocessed files, so I think update function should manually clean files from {languages}.javascript and delete 'javascript_parsed' variable

Status:Needs work» Needs review
StatusFileSize
new3.82 KB

Suppose _locale_invalidate_js(); is enough to rebuild all language JS files.

Status:Needs review» Needs work

update does not work

Status:Needs work» Needs review
StatusFileSize
new3.84 KB

Should be called through locale_inc_callback()

#6: 504506-formatPlural-d7.patch queued for re-testing.

subscribe

2 new core versions are released but this still have no reviews...

I just tested the patch in #26 with Drupal 6.19 and it seems to work just fine.

Could someone please describe a test case/example for this one? I want to help, but I don't know what to test. If someone would describe a few steps to actually reproduce a problem, then more people would help with testing the latest patch.

Thanx in advance.

StatusFileSize
new1018 bytes

@klonos, an only possible test case could be done for locales that have more then 2 plural forms

I faced with this in #2
where.html(Drupal.formatPlural(fbss_remaining, '1 character left', '@count characters left', {}));
valid russian translation is attached

The 3rd
- Install http://drupal.org/project/facebook_status
- Enable locale, Add russian language, make it default
- Import attached translation
- Visit profile page, you see that for numbers 5-9 there's no translation

- apply patch
- run update.php
- visit profile page to make sure that everything works

...locales that have more then 2 plural forms

I see the Plural-Forms: nplurals=3;, but can you please give an example in the Russian language so I can understand & test? For example how would one say the following in Russian:

- one dog
- two dogs
- three dogs
...so on.

you should see

200-195 символов осталось
194-192 символа осталось
191 символ остался

Ok, I googled around and I found this that explains that the plural is different when it comes to numbers ending in 1 than numbers ending in 2, 3 & 4 and again different when they end in 5-9 or 0.

I get it now ;)

I experienced this as well with Vertical tabs and the upload module. Removed the Swedish translation which helped when only one file is uploaded, but it's still quite wierd (for me).

I had the same problem with Vertical tabs + Upload module. #26 fixed it. Thx!

Hope this patch will find its way to core.

Maybe anyone who tested this patch just mark it RTBC?

I understand that this needs to be fixed soon, but RTBC stands for reviewed & tested by the community. So we do need a proper review of the code besides testing. I guess what I am trying to say Andrey is that this effects core and perhaps you should ping one of the 'Big Fellas' for some review instead(?).

Status:Needs review» Needs work

Wonderful! Looked at the D7 and D6 patches in #7. I was just debugging a similar problem and came to this shocking revelation that we have this oversight in the code here. I agree its the best way to represent the strings in JS as they are in the DB for easier handling and we already have @count[3] type of indexing for languages with multiple plural forms that is used on the server side. We can use the same on the client side by applying @andypost's approach.

My only slight issue with the proposed patches is that before empty strings were treated as no-translation, while with the patch they are not so... We should not output empty strings as translations, as it was not supported previously either. People might import a half translated .po file, and the server side will not consider empty strings as valid translations, but will store the source strings without translations. Not sure if we do an INNER JOIN, how NULL values can creep in, but empty string values can possibly creep in (I'm not 100% we avoid saving empty string entirely).

Otherwise, let me say again: wonderful!

Priority:Normal» Critical

Also, I think we can consider this a critical issue for D6.

Priority:Critical» Normal

Gábor, I've searched my DBs for empty strings and found no occurrences.

I think that http://api.drupal.org/api/function/_locale_import_one_string_db/6 prevents storing empty translations with if (!empty($translation)) {

Priority:Normal» Critical

Cross-posted ... So I think INNER JOIN is enough to skip empty translations

EDIT: only possible way is set translation to space and when you submit empty string the record is wiped out from {locales_target}

Indeed, locale_translate_edit_form_submit() also seems to ensure that empty strings are not saved but rather removed. So we should only remove the NOT IS NULL check then since INNER JOIN ensures that already, right?

Status:Needs work» Needs review
StatusFileSize
new1.14 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 504506-formatPlural-followup-d7.patch.
[ View ]
new3.81 KB

Agreed, here's a re-roll

I check for _locale_import_one_string_db/6 not locale_translate_edit_form_submit/6 because a lot of users use l10n_update or similar so I need to be sure that no NULL records appear on DB

Also here's a followup for D7

504506-formatPlural-d6.patch from #45 works for me.
Thanks much!

Subscribing.

#45 The D6 patch works like a charm. I really hope this will be commited soon!

Version:6.x-dev» 7.x-dev
Priority:Critical» Major

Let's review this for D7 first, #45

StatusFileSize
new1.14 KB
PASSED: [[SimpleTest]]: [MySQL] 30,362 pass(es).
[ View ]

Let's test follow-up, re-roll

Status:Needs review» Needs work

The last submitted patch, 504506-formatPlural-followup-d7.patch, failed testing.

Status:Needs work» Needs review

subscribe

I'd like to confirm that the patch for D6 in #45 works for me. In fact, it just resolved a weird issue of having vertical_tabs module enabled while there's an attached file to a node (at node edit form). This patch should be ported to D6.

Version:7.x-dev» 7.0-rc3
Priority:Major» Critical

Am I allowed to change this to critical?
It was crititcal before and I think this issue is forgotten somehow.

Joep

#50 looks like working for me
D7-RC2

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new1.09 KB

#50 rewrited against current HEAD

Priority:Critical» Major

Issues which have significant repercussions but do not render the whole system unusable are marked major. An example would be a PHP error which is only triggered under rare circumstances or which affects only a small percentage of all users.

seems like "rare circumstance" applies.

ok, I agree.
For me it is critical though on D6 together when using the vertical tabs module. See issue http://drupal.org/node/421318.

Version:7.0-rc3» 7.x-dev
Status:Needs review» Reviewed & tested by the community
Issue tags:+needs backport to D6, +Quick fix

Patch #50 still applies with offset.

Patch #59 seems just cross-post a status and done not from root of core

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

Let's add a test for this, so we don't end up getting this bug again.

Status:Needs work» Needs review

@webchick #50 is just a follow-up patch recomended by Gabor in #44

And yes it's good to have tests for JS but it's not possible for now... sad but maybe someone knows a way...

@webchick: the query before patch and after patch should perform the same operation, its just about removing an excessive NOT IS NULL which the INNER JOIN already assumes. Not sure what would you like to see tested here?

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs tests

Suppose we should commit this quick fix for D7 and move this issue to D6.

StatusFileSize
new25.53 KB
PASSED: [[SimpleTest]]: [MySQL] 31,500 pass(es).
[ View ]

Ah, ok. Sorry about that. I didn't catch it was JS, since the patch changes SQL.

Because of that, I'd like to see the testbot sign off on this patch, so re-uploading without the D7 extension.

Status:Reviewed & tested by the community» Needs work
Issue tags:-needs backport to D6, -Quick fix

The last submitted patch, 504506-formatPlural-followup-drupal.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+needs backport to D6, +Quick fix

#67: 504506-formatPlural-followup-drupal.patch queued for re-testing.

EDIT: Previous retesting was green let's test 3rd time

Status:Needs review» Reviewed & tested by the community

Suppose first broken pass been caused by something wrong|fragile within file-test

Re 504506-formatPlural-d6.patch from #45 above:

Our install is Pressflow/Drupal 6.20 (and naturally Locale 6.20). When trying to apply this patch, it does not return any data even with --verbose option included. The patch is apparently not applied and bug/error still there.

We are getting un-translated text when posting or editing a Status:
@count[4] characters left
@count[3] characters left

Another batch is required for d6.20? Any recommendations?

Version:7.x-dev» 6.20
Status:Reviewed & tested by the community» Needs work

I'm having this issue as well. There were no problems when using patched Drupal 6.19, but things seem to break again with version 6.20.

Version:6.20» 7.x-dev

@Madis: I am with you on this one, but in #49 it was decided that this issue will be dealt in D7 first, then backported to D6 (it does have a "needs backport to D6" tag already). So, I am switching it back before someone else does ;)

subscribe

Status:Needs work» Reviewed & tested by the community

Reverting status as per #70

Version:7.x-dev» 8.x-dev
Status:Reviewed & tested by the community» Needs review

D8 first. Let's get these fixed on D8 and into D7.

Issue tags:+needs backport to D7
StatusFileSize
new25.53 KB
PASSED: [[SimpleTest]]: [MySQL] 29,399 pass(es).
[ View ]

D8 needs only patch from #67 so let's get bot's reviews

I wonder when the fix is coming to Drupal 6 core.

Affecting us users of Vertical Tabs and File Uploads http://drupal.org/node/533720

Bump, this is a very small patch that been RTBCed still not in core but D7.1 is upcoming

A bot tests your patch against Drupal automatically?

Thats awesome :D

thanks for the patch!!!

hello

they told to me in vertical tabs issues to report here.
i had vertical tabs, upload and wysiwyg working with no problem in drupal 6.20 multilingual.

i have the l10n_client module to translate the interface and i was surprised that strings from vertical tabs didnt appear to translate.
i went to drupal interface to search and i found them but they were not translated.
i made those few changes and kept working.
some hours later i see the problem with javascript in pages that had a file uploaded.

the bug is hidden in a translation string in the vertical tabs module :

@count attachments
; sites/all/modules/vertical_tabs/core/upload.js

it has ";" and it shouldnt have it (none of the other strings in the interface has it).

if you translate that string, everything goes nuts.
if you leave it blank, everything works.

// $Id: upload.js,v 1.1.2.1 2009/12/09 01:08:39 davereid Exp $
Drupal.verticalTabs = Drupal.verticalTabs || {};
Drupal.verticalTabs.attachments = function() {
  var size = $('#upload-attachments tbody tr').size();
  if (size) {
    return Drupal.formatPlural(size, '1 attachment', '@count attachments');
  }
  else {
    return Drupal.t('No attachments');
  }
}

so thanks for the patch, i go to apply it :)
and hope that for the next drupal 6.xx it is already accepted :)

Status:Reviewed & tested by the community» Needs review

I am confused here.

Why is this patch not accepted yet?
It is a major bug in D6 for a long time and we keep moving it to D7 and than D8 (and next year to D9?).
Pathes have been provided and tested as far as I can tell.

Please commit this to D6 since we have been hacking core too long now!

Cheers,
Joep

today i saw the new 6.20.
i look for the text to see if this was implemented but for what i undertand, it didnt.
i imagine that they are too busy with so many code but we should find a way...

subscribe

subscribe.. hope to see this fixed in Drupal 6 as soon as possible

Priority:Major» Critical

changing to critical, as this seems to affect versions 6 to 8!

Priority:Critical» Major

This is not critical.

StatusFileSize
new1.82 KB
PASSED: [[SimpleTest]]: [MySQL] 29,910 pass(es).
[ View ]

Updated the code in #77 to current standards.

StatusFileSize
new1.38 KB
PASSED: [[SimpleTest]]: [MySQL] 29,912 pass(es).
[ View ]

Reverted the db_select() back to a db_query() at the suggestion of chx and Dave Reid.

Priority:Major» Critical

@pillarsdotnet thanks a lot for the patch :)
you are a programmer and for you it is easy to apply a patch.
as you speak english, i think more of your sites are in that language, so you dont suffer with this.
but for multilingual sites and for people that is not tech, this is critical.
i hope you understand why i rechange to critical.
also its long time ago that it is around...
have a good day :)

Priority:Critical» Major

Again this is NOT critical.

Read http://drupal.org/node/45111 why that is. The bug won't get commited faster if you put it to critical...

... Okay... but as mentioned in the linked post:

Critical bugs either render a system unusable (not being able to create content or upgrade between versions, blocks not displaying, and the like)...

And that's exactly what happens on one of our customer's sites. We use the Vertical tabs module in combination with the Path redirect module. The latter module provides a vertical tabs summary using Drupal.formatPlural(). And as this function throws an error, all other fieldsets vanish from the node editing forms... which results in the loss of node settings (e.g. the author name).

So I guess it should be treated as critical... and also because it is a bug that seems to appear in all 3 major Drupal versions.

What do you think?

Cheers

TOM

Status:Needs review» Reviewed & tested by the community

Gosh, this simple patch we are discussing is just about removing a "translation IS NOT NULL" clause from a query for a table where that field is defined NOT NULL, so it cannot even be NULL anytime.

This is the schema:

<?php
  $schema
['locales_target'] = array(
   
'description' => 'Stores translated versions of strings.',
   
'fields' => array(
     
'lid' => array(
       
'type' => 'int',
       
'not null' => TRUE,
       
'default' => 0,
       
'description' => 'Source string ID. References {locales_source}.lid.',
      ),
     
'translation' => array(
       
'type' => 'text',
       
'mysql_type' => 'blob',
       
'not null' => TRUE,
       
'description' => 'Translation string value in this language.',
      ),
     ...
?>

This is the query modified:

SELECT s.lid, s.source, t.translation FROM {locales_source} s INNER JOIN {locales_target} t ON s.lid = t.lid AND t.language = :language WHERE s.location LIKE '%.js%' AND s.textgroup = :textgroup AND t.translation IS NOT NULL

The locales_target table will not contain any NULL in translation, therefore that can only ever appear in a JOIN. Now, the query uses INNER JOIN, so it sounds impossible that a row would would be found with NULL value. That is all that the D8/7 patch is about. Can we get it committed please, so we can move on to more serious business to fix it for Drupal 6. The latest patch in #90 looks good.

Note that I'll have comments on the D6 core patch once D8/7 lands. That is an entirely different animal since we did not even fix the original bug there.

Thnx Gábor for the review :)

Version:8.x-dev» 6.x-dev
Status:Needs review» Patch (to be ported)
Issue tags:-needs backport to D7

Ok. I still would prefer to have some kind of tests demonstrating expected behaviour since we managed to break this twice(!), but I guess that gets tricky with JS stuff. :\

Committed to 8.x and 7.x. Moving down to D6, which I think still needs a viable patch.

Status:Patch (to be ported)» Needs review
Issue tags:-needs backport to D6
StatusFileSize
new3.3 KB

re-roll of #45 for D6

Status:Needs review» Needs work

@webchick: thanks!

Commenting on the D6 patch then. It is still very well possible that $data->translation will be empty. Why are we not leaving that part of the condition there? Could save us considerable JS size I think, depending on your site translation status.

Status:Needs work» Needs review

@Gabor please take a look at #41 - #45 we are already discussed that $data->translation could NOT be empty!

Issue tags:-Quick fix

You are right, this was already verified. Ok, any other concerns from someone? (I have not tested the patch personally).

Issue tags:+quickfix

(Restore before spam tag state :)

Status:Needs review» Reviewed & tested by the community

I can confirm that patch #97 works.

I too can confirm that patch #97 works. (Tested with Drupal 6.22.)

PS! Don't forget to run update.php. Before doing that I thought the patch didn't work.

me too :)
patch #97 works. (Tested with Drupal 6.22.)
thanks a lot!

Patch worked, thanks!

Subscribing

Can we get this committed?

Subscribing

Subscribing

Could someone explain to me why this has not been commited yet? I am confused about the commiting to core process.

@CompuBase because D6 is now the "old man" and stable, it only gets occasional attention, when it's time to make a number of commits. Since this is RTBC and @gabor has looked at it, I'd think there would be a good chance for it to go into the next D6 point release.

I really hope so!

Subscribing

I can also confirm #97 works fine. Hopefully this get is 6.x soon.

Issue tags:+Favorite-of-Dries

wow
such an old issue

I`ve don`t see any stopping steps for making this already commited

another core hack today, please guys, get this one committed!

Issue tags:-Favorite-of-Dries

Sorry, but non-Dries people don't get to use that tag. :)

Status:Reviewed & tested by the community» Fixed

Thanks for those reviews. Committed to Drupal 6!

Yahoo!!!

Finally!! Thanx ;)

Unfortunately this code (patch) fails for me in Drupal 6 during upgrade from 6.22 to 6.24. :-(
The error is the following:

An error occurred. http://example.com/update.php?id=1&op=do
Fatal error: Call to undefined function locale_inc_callback() in /home/www-data/example.com/www/modules/locale/locale.install on line 236

Reported this as a bug: http://drupal.org/node/1425260
It seems Andrey's patch in #97 (http://drupal.org/node/504506#comment-4579602) is not OK after all.

@müzso locale_inc_callback() is a part of locale.module so I suppose some contrib module does not allow locale.module to be loaded. Let's continue at #1425260: "Call to undefined function locale_inc_callback()" during 6.22 -> 6.24 upgrade

Added this to the known issues on http://drupal.org/drupal-7.12

In Drupal 6.24 if you had locale module enabled earlier, but it is not currently turned on, the update will fail with Call to undefined function locale_inc_callback(). A fix is being worked on for Drupal core.

My understanding is that is the underlying problem. Still feel free to continue fixing on the linked issue, just noting here for people looking for the issue.

Status:Fixed» Closed (fixed)
Issue tags:-quickfix

Automatically closed -- issue fixed for 2 weeks with no activity.