Currently the "language neutral" concept is used in two ways:

  • to indicate the language of nodes which have not been explictly assigned a language;
  • to indicate the language of fields which have not been explictly assigned a language;

In both cases the "language neutral" concept has the same meaning, but the actual values assigned are different: nodes use an empty string, fields a ISO639-2 language code (zxx) which means No linguistic content / Not applicable.

Having two different language codes to indicate the same language is both wrong and error prone, AAMOF Field API has to take into account this situation, subverting the usual dependency order which sees Node module depending on the Field module.

I'd suggest to refactor Node to make it use the standard language code.

CommentFileSizeAuthor
#30 language_none-635094-30.patch2.25 KBplach
#27 language_none-635094-25.patch672 bytesplach
#16 language_none-635094-16.patch156.68 KBplach
#16 language_none-635094-work.patch21.25 KBplach
#14 language_none-635094-work.patch21.25 KBplach
#14 language_none-635094-14.patch156.76 KBplach
#11 language_none-635094-11.patch154.56 KBplach
#8 language_none-635094-8.patch153.88 KBplach
#7 language_none-635094-7.patch19.55 KBplach
#5 language_none-635094-5.patch10.13 KBplach
#1 language_none-635094-1.patch142.83 KBplach
#1 language_none-635094-work.patch8.09 KBplach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Active » Needs review
FileSize
language_none-635094-work.patch8.09 KB
language_none-635094-1.patch142.83 KB

The current patch moves the FIELD_LANGUAGE_NONE definition to bootstrap.inc, rename it to LANGUAGE_NONE and make Node use it instead of the empty string.

Attached is also a "light" version of the patch which does not rename FIELD_LANGUAGE_NONE. You can use this one for reviews, as the real one is bloated by hundreds of braindead search/replace hunks.

We just need an upgrade path and then we should be ok.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Issue tags: +API clean-up
yched’s picture

yched CreditAttribution: yched commented

Makes sense IMO, and +1 for a shorter constant :-).
Patch raises exceptions, though.

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Needs review
FileSize
language_none-635094-5.patch10.13 KB

This one provides the upgrade path and should fix tests. I removed FIELD_LANGUAGE_NONE renaming, but I'll add it again as soon as I get an ok for the light version.

@yched: ;)

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Needs review
FileSize
language_none-635094-7.patch19.55 KB

Obviously URL aliases must be fixed too.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
language_none-635094-8.patch153.88 KB

DamZ reviewed #7 in IRC and asked for the Big One!

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs review » Reviewed & tested by the community

I like it, and I hope the test bot will too.

Dries’s picture

Dries CreditAttribution: Dries commented

I'm OK with this but I do want to note that, with this patch, we start mixing ISO-639-1 (two character code) and ISO-639-2 (three character codes). That is probably not a crime, but might be somewhat unexpected.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
language_none-635094-11.patch154.56 KB

Rereolled #8 and added the following hunk as #624290: Wrong default comment language went in:

-    '#value' => isset($comment->language) ? $comment->language : '',
+    '#value' => isset($comment->language) ? $comment->language : LANGUAGE_NONE,

@Dries:

I'm OK with this but I do want to note that, with this patch, we start mixing ISO-639-1 (two character code) and ISO-639-2 (three character codes). That is probably not a crime, but might be somewhat unexpected.

I'd prefer to go with a ISO-639-1 language code too, unfortunately there is nothing like zxx in it.

OTOH the w3c specification of language tags allows mixing 2 and 3 character codes, AAMOF only languages defined in the IANA registry are legal and "When languages have both an ISO 639-1 two-character code and a three-character code (assigned by ISO 639-2, ISO 639-3, or ISO 639-5), only the ISO 639-1 two-character code is defined in the IANA registry."

Hence the IANA registry actually mixes 2 and 3 language codes, as we are doing here for the lack of an appropriate 2 characters code.

Status: Reviewed & tested by the community » Needs review
Issue tags: -translatable fields, -API clean-up

Dries requested that failed test be re-tested.

Status: Needs review » Needs work
Issue tags: +translatable fields, +API clean-up

The last submitted patch failed testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Needs review
FileSize
language_none-635094-14.patch156.76 KB
language_none-635094-work.patch21.25 KB

rerolled

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
language_none-635094-work.patch21.25 KB
language_none-635094-16.patch156.68 KB

system updates count went ahead, meanwhile

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Needs review
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs review » Reviewed & tested by the community

Green. The patch to be committed is language_none-635094-16.patch (the big one), the other is just for reviewers' convenience.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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

anarchivist’s picture

anarchivist CreditAttribution: anarchivist commented
Status: Closed (fixed) » Active

Actually, I'm a little concerned about this - zxx in my opinion is not the proper code for this. See this FAQ entry on the Library of Congress's ISO 639-2 FAQ. By specifying zxx as the code, Drupal will be saying that nodes don't have any linguistic content! While in some cases this might be true, it's a pretty safe assumption that most Drupal nodes everywhere contain some linguistic content. und would probably be more correct.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

I didn't notice the und code, I agree that it would be more appropriate, and the replacement would be rather easy.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

The 'und' code has been there all along in the spec - I think there are other issues where it was argued against. I don't really care, but seems like a bikeshed argument at this point.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

I haven't noticed 'und' either. Sounds like a good idea. I'm not aware of any discussions between zxx and und, Peter do you have specific pointers?

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Active » Needs review

I'd like to read that discussion too. Meanwhile here's a patch that replaces zxx with und.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Issue tags: -translatable fields

This does not concerns TF anymore.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
language_none-635094-25.patch672 bytes

the patch

Status: Needs review » Needs work

The last submitted patch failed testing.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

Hmm, maybe I'm misremembering - all I can find is http://drupal.org/node/394728#comment-2303062 where I suggested 'und' would be better :)

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Needs review
FileSize
language_none-635094-30.patch2.25 KB

Can't reproduce any error on install but I found a couple of tests using explicitly 'zxx' instead of LANGUAGE_NONE.

@pwolanin: I didn't read any particular argument against 'und' in that issue. It seems we can safely go on, right?

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

@plach - I will be annoyed to have to fix up Apache Solr module, but otherwise, 'und' seems fine to me.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Fixed

Committed to CVS HEAD.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Fixed » Needs work
Issue tags: +Needs documentation

This needs upgrade docs. I just cvs-upped and got notices everywhere. We don't do HEAD-HEAD upgrades yet, but it took me a good ten minutes to work out there was no typo for uid anywhere and find this issue, it's quite likely some modules other than ApacheSolr are depending on the old field structure too.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

@catch: which should be the proper place to document this?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Just at http://drupal.org/update/modules/6/7

axyjo’s picture

axyjo CreditAttribution: axyjo commented
Status: Needs work » Needs review

http://drupal.org/update/modules/6/7#drupal_language_none

How's that?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Fixed

Looks great.

Status: Fixed » Closed (fixed)

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

Andrew Schulman’s picture

Andrew Schulman CreditAttribution: Andrew Schulman commented

See also #965300: Change LANGUAGE_NONE to LANGUAGE_NOT_SPECIFIED; add LANGUAGE_NOT_APPLICABLE and LANGUAGE_MULTIPLE.