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!
  }
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

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.

andypost’s picture

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

andypost’s picture

Status: Active » Needs review
FileSize
2.91 KB
2.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 ...

Damien Tournoud’s picture

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!).

Damien Tournoud’s picture

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))?

andypost’s picture

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!

andypost’s picture

Dries’s picture

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.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Alright, committed to CVS HEAD. Thanks all.

Heine’s picture

Damien Tournoud’s picture

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

To be ported to D6.

Heine’s picture

Status: Patch (to be ported) » Needs review

There is a Drupal 6 patch already.

klonos’s picture

subscribing...

VladSavitsky’s picture

subscribing

pvasili’s picture

subscribing

Peter Arius’s picture

subscribing

sgabe’s picture

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

andypost’s picture

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

andypost’s picture

Same patch against current D-6

sgabe’s picture

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

DakoCwerf’s picture

#19 works fine for me too.

Damien Tournoud’s picture

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.

andypost’s picture

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

andypost’s picture

Status: Needs work » Needs review
FileSize
3.82 KB

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

andypost’s picture

Status: Needs review » Needs work

update does not work

andypost’s picture

Status: Needs work » Needs review
FileSize
3.84 KB

Should be called through locale_inc_callback()

Enemy’s picture

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

gibus’s picture

subscribe

andypost’s picture

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

sgabe’s picture

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

klonos’s picture

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.

andypost’s picture

FileSize
1018 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

klonos’s picture

...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.

andypost’s picture

you should see

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

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 ;)

mattiasj’s picture

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).

haggins’s picture

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

Hope this patch will find its way to core.

andypost’s picture

Maybe anyone who tested this patch just mark it RTBC?

klonos’s picture

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(?).

Gábor Hojtsy’s picture

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!

Gábor Hojtsy’s picture

Priority: Normal » Critical

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

andypost’s picture

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)) {

andypost’s picture

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}

Gábor Hojtsy’s picture

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?

andypost’s picture

Status: Needs work » Needs review
FileSize
1.14 KB
3.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

joep.hendrix’s picture

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

Leeteq’s picture

Subscribing.

joep.hendrix’s picture

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

andypost’s picture

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

Let's review this for D7 first, #45

andypost’s picture

Let's test follow-up, re-roll

Status: Needs review » Needs work

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

andypost’s picture

Status: Needs work » Needs review
podarok’s picture

subscribe

jazzitup’s picture

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.

joep.hendrix’s picture

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

podarok’s picture

#50 looks like working for me
D7-RC2

podarok’s picture

Status: Needs review » Reviewed & tested by the community
arlinsandbulte’s picture

podarok’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.09 KB

#50 rewrited against current HEAD

chx’s picture

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.

joep.hendrix’s picture

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.

andypost’s picture

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

webchick’s picture

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.

andypost’s picture

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...

Gábor Hojtsy’s picture

@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?

andypost’s picture

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.

webchick’s picture

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.

andypost’s picture

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

andypost’s picture

Status: Needs review » Reviewed & tested by the community

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

tcibah’s picture

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?

Madis’s picture

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.

klonos’s picture

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 ;)

Ilya1st’s picture

subscribe

sharpbites’s picture

Status: Needs work » Reviewed & tested by the community

Reverting status as per #70

rfay’s picture

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.

andypost’s picture

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

doomed’s picture

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

andypost’s picture

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

doomed’s picture

A bot tests your patch against Drupal automatically?

Thats awesome :D

candelas’s picture

thanks for the patch!!!

candelas’s picture

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 :)

joep.hendrix’s picture

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

candelas’s picture

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...

Anonymous’s picture

subscribe

hctom’s picture

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

hctom’s picture

Priority: Major » Critical

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

pillarsdotnet’s picture

Priority: Critical » Major

This is not critical.

pillarsdotnet’s picture

Updated the code in #77 to current standards.

pillarsdotnet’s picture

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

candelas’s picture

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 :)

aspilicious’s picture

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...

hctom’s picture

... 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

Gábor Hojtsy’s picture

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:

  $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.

aspilicious’s picture

Thnx Gábor for the review :)

webchick’s picture

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.

andypost’s picture

Status: Patch (to be ported) » Needs review
Issue tags: -Needs backport to D6
FileSize
3.3 KB

re-roll of #45 for D6

Gábor Hojtsy’s picture

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.

andypost’s picture

Status: Needs work » Needs review

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

Gábor Hojtsy’s picture

Issue tags: -Quick fix

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

Gábor Hojtsy’s picture

Issue tags: +quickfix

(Restore before spam tag state :)

joep.hendrix’s picture

Status: Needs review » Reviewed & tested by the community

I can confirm that patch #97 works.

hansfn’s picture

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.

candelas’s picture

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

ChrisFlink’s picture

Patch worked, thanks!

crea’s picture

Subscribing

joep.hendrix’s picture

Can we get this committed?

bear_beavis’s picture

Subscribing

Agence Web CoherActio’s picture

Subscribing

joep.hendrix’s picture

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

rfay’s picture

@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.

joep.hendrix’s picture

I really hope so!

fermer’s picture

Subscribing

marvil07’s picture

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

podarok’s picture

Issue tags: +Favorite-of-Dries

wow
such an old issue

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

joep.hendrix’s picture

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

webchick’s picture

Issue tags: -Favorite-of-Dries

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

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for those reviews. Committed to Drupal 6!

joep.hendrix’s picture

Yahoo!!!

klonos’s picture

Finally!! Thanx ;)

müzso’s picture

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.

andypost’s picture

@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

Gábor Hojtsy’s picture

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.