Spin-off from #657972: Make field fallback logic actually reusable.

The TF code is black magic even to me after a couple weeks away from that area. I had a hell of a hard time making sense of the logic in the patch above.

- A docblock at the beginning of field.multilingual.inc explaining the general logic of TF regarding f_a_*() functions, field_invoke(), hook_f_m_fallback_alter(), language handlers (which for instance I don't know the first thing about). Doesn't need to be two pages, but should give the overall logic of the architecture and mechanisms, which currently is only scattered in code here and there.

- $options['language'] is currently not documented at all in the PHPdoc for _field_invoke() - not even in HEAD. Its behavior should be made extra clear, as well as the fact that it can be an array per field. The fact that the function will iterate on all language of all fields, or on the specific field and /or language specified in $options should also be stated in a PHPdoc paragraph below the function description.

- I'd actually advocate that the $language_suggestion logic in f_m_available_languages() needs to be moved inside _field_invoke. If there is an explicit language, use it, or else iterate on all available_languages(). Then f_m_available_languages() == the current _f_m_available_languages() helper func.
Would make the TF logic of field_invoke() clearer by looking at the code. Right now all you can see "is iterate on all available_languages()" and you need to read the code in f_m_available_languages() to understand that it restricts to $options['language'], which is not conveyed by the func name.

- The function names in field.multilingual.inc are cryptic. I'm really not a fan of the field_multilingual_* namespace, really convoluted. The fact that functions are grouped in a 'multilingual.inc' file is one thing, this doesn't mean we need to use that as a namespace.
For instance, field_multilingual_available_languages() could be field_available_languages(). Some functions could keep the 'multilingual' term, but only because it's needed / relevant for what the function does (e.g field_multilingual_enabled())

- field_multilingual_content_languages() doesn't seem like it belongs to field API. It's more generic than that.

CommentFileSizeAuthor
#41 tf-664136-41.patch703 bytesplach
#35 tf-664136-35.patch4.88 KBplach
#18 tf-664136-18.patch4.87 KBplach
#15 tf-664136-15.patch4.9 KBplach
#13 drupal.tf-docs.13.patch5.01 KBsun
#10 tf-664136-10.patch5.28 KBplach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

+1

plach’s picture

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

I apologize for the code above being poorly documented but it was just a tentative approach. It didn't mean to be a definitive patch, so I didn't spend much time on that aspect. If the overall idea is deemed good, I'll document it the best I can.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Issue tags: +translatable fields
yched’s picture

yched CreditAttribution: yched commented
Issue tags: +translatable fields

@plach: I tried to keep issues and remarks that are actually tied to the 'fallback logic' within my comments in #664136: Enhance TF code documentation, so that the patch can be polished and get in faster. The remarks above are more general about TF code as currently stands.

plach’s picture

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

Ok, I almost finished to implement your suggestions. I tried to fix all the docs except the field.multilingual.inc initial docblock in the other patch. I'll post it tomorrow morning.

yched’s picture

yched CreditAttribution: yched commented

Cool, thks plach !

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Priority: Critical » Normal

Demoting from critical, if we held up core releases on code only one person actually understands we'd never release anything.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Active » Needs review
FileSize
tf-664136-10.patch5.28 KB

And here is the documentation: I tried to cover all the TF aspects. When this gets committed I'll link http://api.drupal.org/api/drupal/modules--field--field.multilingual.inc/7 from the D6/D7 upgrade guide.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Title: Enhance TF code readability and documentation » Enhance TF code documentation

Retitling as the readability has been addressed in #657972: Make field fallback logic actually reusable.

yched’s picture

yched CreditAttribution: yched commented

Awesome. A couple remarks :

+++ modules/field/field.multilingual.inc	4 Apr 2010 14:36:54 -0000
@@ -3,7 +3,65 @@
+ * Fields have native multilingual support, as a matter of fact they all share
+ * the following structure:
+ *
+ * $entity->{$field_name}[$langcode][$delta][$column_name]

Minor: the "as a matter of fact" bit sounds a bit casual IMO.
+ using the word "share" is a bit ambiguous (kind of clashes with "shared fields"...)

+++ modules/field/field.multilingual.inc	4 Apr 2010 14:36:54 -0000
@@ -3,7 +3,65 @@
+ *   altered by modules implementing hook_field_available_languages_alter().
...
+ * translation handler. The available languages for a particular field are
+ * returned by field_available_languages().

It looks like this bit belongs to the previous paragraph (that mentions hook_field_available_languages_alter()) ?

+++ modules/field/field.multilingual.inc	4 Apr 2010 14:36:54 -0000
@@ -3,7 +3,65 @@
+ * checks the $field['translatable'] property, where $field is the array
+ * returned by field_info_field(), and the available translation handlers. A

"where $field is the array returned by field_info_field()" is a bit extra verbose IMO + messes the flow of the sentence.

+++ modules/field/field.multilingual.inc	4 Apr 2010 14:36:54 -0000
@@ -3,7 +3,65 @@
+ * are given a valid language suggestion, which can narrow the available
+ * languages set to a single value. The langugage suggestion is processed by

"which narrows the processing to a single language" ?
+ typo "langugage"

+++ modules/field/field.multilingual.inc	4 Apr 2010 14:36:54 -0000
@@ -3,7 +3,65 @@
+ * languages set to a single value. The langugage suggestion is processed by
+ * _field_language_suggestion(), which returns the actual set of languages to
+ * act on.

Actually, I'm not sure that last sentence is needed. Possibly too much detail on an internal function ? Your call.

+++ modules/field/field.multilingual.inc	4 Apr 2010 14:36:54 -0000
@@ -3,7 +3,65 @@
+ * Most of field_attach_X() functions act on every available language, however

"of" needs to be removed in "most of field_attach_X() functions".

+ I'd add "As a result, most field_attach_X() functions ..." at the beginning of the sentence, to make it clear that this is a consequence of _field_invoke() behavior.

+++ modules/field/field.multilingual.inc	4 Apr 2010 14:36:54 -0000
@@ -3,7 +3,65 @@
+ *   1) ignoring missing translations; ¶

Trailing whitespace

Other than that,
- the "2)" paragraph and the last paragraph look like they're dealing with the same topic, so the layout between the two of them is a bit unclear / redundant to me.
- I still don't actually get what is a 'translation handler' ;-) Is there a short way to roughly explain this here ?

128 critical left. Go review some!

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
drupal.tf-docs.13.patch5.01 KB

Slightly revamped, reworded, restyled this. Seems to address most of yched's concerns already.

yched’s picture

yched CreditAttribution: yched commented

Actually, a couple still apply ;-)

+++ modules/field/field.multilingual.inc	4 Apr 2010 17:56:06 -0000
@@ -3,7 +3,65 @@
+ * Every field can hold a single or multiple values for each language belonging
+ * to the available languages:
+ * - Untranslatable fields have a cardinality of 1 and only contain
+ *   LANGUAGE_NONE.

This part got more confuse, I think.
+ 'cardinality' is dangerous here, clashes with the usual sense of 'field cardinality'

+++ modules/field/field.multilingual.inc	4 Apr 2010 17:56:06 -0000
@@ -3,7 +3,65 @@
+ *   by modules implementing hook_field_available_languages_alter().
...
+ * The available languages for a particular field are returned by
+ * field_available_languages(). By default, _field_invoke() and

Looks like this belongs one paragraph up, where hook_field_available_languages_alter() is mentioned.

+++ modules/field/field.multilingual.inc	4 Apr 2010 17:56:06 -0000
@@ -3,7 +3,65 @@
+ * Whether a field is translatable is determined by calling
+ * field_is_translatable(), which checks the 'translatable' property in the data
+ * returned by field_info_field(), and whether there is a translation handler
+ * available for the field.
+ *
+ * A translation handler is a module that registers itself via
+ * hook_entity_info() to handle field translations. A field is translatable if
+ * its 'translatable' property is set to TRUE and it has at least one available
+ * translation handler.

The 2 paragraphs basically say the same thing ?

+++ modules/field/field.multilingual.inc	4 Apr 2010 17:56:06 -0000
@@ -3,7 +3,65 @@
+ * field_is_translatable(), which checks the 'translatable' property in the data
+ * returned by field_info_field(), and whether there is a translation handler

The $field['translatable'] property would be clearer ?

+++ modules/field/field.multilingual.inc	4 Apr 2010 17:56:06 -0000
@@ -3,7 +3,65 @@
+ * Most of field_attach_*() functions act on all available languages, except for

"Most of f_a_X() functions" -> "Most f_a_X() functions"

+++ modules/field/field.multilingual.inc	4 Apr 2010 17:56:06 -0000
@@ -3,7 +3,65 @@
+ * - field_attach_view() requires a language code the entity is displayed in.

badly formed sentence

The end of the doc is much clearer IMO.

128 critical left. Go review some!

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
tf-664136-15.patch4.9 KB

This should take care of #14.

yched’s picture

yched CreditAttribution: yched commented

I hate to be such a sucker :-(, but I'd really prefer we didn't mention the 'cardinality' of the set of languages.
Can we agree on "For untranslatable fields this set only contains LANGUAGE_NONE." ?

Other than that, RTBC. Great writeup.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Needs work

After working on the previous patch I thought quite extensively about this issue, and I think this @defgroup will be the #1 lookup/search target for anyone who tries to understand fields + languages + multilingual fields support + translatable fields + field value handling in multilingual environments + interaction with other workflows (like permissions, AJAX, anyone?) + whatnot.

I think we need to re-think. I think we need to take a large step back. And look at what we have done, possibly for the first time. :)

We need to answer questions. To kick-start - perhaps but not necessarily in this order:
<schizophrenic>

  1. How are fields and languages connected? And why?
  2. What's the meaning of multilingual and translatable? Is it the same? (no workflow/processing differences?)
  3. If there can be field values for a certain language only (and no other languages), how am I supposed to work with this field?
  4. What's the purpose of LANGUAGE_NONE, and, can it happen that a field has no values for LANGUAGE_NONE but only for a certain language?
  5. If there can be field values for more than one language, what does that mean?
  6. When, and perhaps how, are those values used and/or displayed, and how am I supposed to work with field value translations?
  7. Do I need to care for translated field values? If so, when? If always, how? If not, which parts does Field API handle for me?
  8. Who decides whether a field can have a language? Who decides whether it can be translated?
  9. How is the language to input field values determined? How is the language to display field values determined?
  10. What happens if an entity is displayed in a language for which no field value translations exist?
  11. How do translated field values and the global language configuration interact?
  12. If they do interact, is there also something like a "default language"? Do I need to account for that?
  13. Can I intercept the language fallback logic somehow?
  14. When is a field considered to have no translated values, and when is it considered to have empty values for a language?
  15. Which translation handlers are built-in in Drupal core?
  16. Why am I not able to translate a field's values into other languages? (UI)

</schizophrenic>

Possibly more. Though, answering those questions step by step in this order will likely lead to a @defgroup description that humans are able to understand. :)

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
tf-664136-18.patch4.87 KB

This one takes care of #16.

I ain't sure that the answers to the above questions fit in this initial doc block. Maybe a dedicated page in the Field API handbook?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

To some extent, these docs belong into code. To my knowledge, the best @defgroup example we currently have is http://api.drupal.org/api/group/ajax/7

One important observation on above list of questions is that the latter ones (starting somewhere around 10.) are more or less covered in this patch. But the preceding and some others are not.

plach’s picture

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

One important observation on above list of questions is that the latter ones (starting somewhere around 10.) are more or less covered in this patch. But the preceding and some others are not.

Yes, I noticed that. I didn't know such a complete documentation with code excerpts and things could fit into source code.
I'd like to read yched's opinion before starting everything again.

Anyway I'm a little bit worried about #2: I just realized that in this context we don't have a solid difference between the two terms, in my mind they have a slightly different meaning, but that's absolutely not reflected by code.

plach’s picture

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

yched?

yched’s picture

yched CreditAttribution: yched commented

Yup, sorry for the delay, only got like one hour a day in front of the keyboard for the time being :-).

I think the questions in sun's #17 are valid, of course. However, the scope of this issue was primarily to get the general *internal* workflow (like : how does it work ? how wrt to the general Field API workflow ? - which was unclear even to me a couple weeks after the initial TF patch landed) written down somewhere instead of encrypted in obscure code in _field_invoke(). The patch in #18 adresses that, and as such is worth committing IMO.

plach’s picture

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

@sun: So? RTBC or needs work?

plach’s picture

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

Still waiting to know if it's ok to commit this and answer the questions from #17 in a handbook page.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Let's get some core maintainer feedback on how to proceed.

Dries’s picture

Dries CreditAttribution: Dries commented

I think the docgroup should paint a picture of the architecture/design so that people can easily understand the code. We shouldn't try answer all of the above questions IMO -- those are probably best answered in the handbook. However, the docgroup should set people up to read the code and to help them answer these questions. It's sort of a subtle difference.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work

I guess that's a needs work.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue tags: +Needs documentation

Hm. heheh... the core maintainer feedback leaves lots of room for interpretation. I guess it's neither the patch nor the proposal to explain all from high-level to low-level then. Rather, both, but differently. :-)

Therefore, remixing some answers to the questions with parts of the patch will likely lead to something committable.

Ideally, I think we should pull someone from the documentation team into this issue to find the right level of detail - as we all are more or less familiar with the system. (YMMV)

plach’s picture

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

Yes, we definitely need some hint on the direction to take here. The current patch tries to provide an architectural overview, but still keeping the description low level as yched asked in the OP. If that's somehow incorrect an example on how to improve it would be helpful.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Postponed (maintainer needs more info)

I'm sorry but I have absolutely no clue about how to proceed here.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Priority: Normal » Major

I'd say this needs to be done sooner or later.

yched’s picture

yched CreditAttribution: yched commented
Status: Postponed (maintainer needs more info) » Needs review

Does it still apply ?
We're definitely better with than without this...

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
tf-664136-35.patch4.88 KB

The previous patch applied with some offset. Here is a reroll.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

I can't quite parse Dries's #28 either, but I think he's basically saying that docblocks don't need to answer all of the questions outlined in sun's #17. The handbook's a much better place for this kind of information, because we can elaborate, re-organize, pull in additional questions from comments, etc. See http://drupal.org/node/146843 and http://drupal.org/node/310069 for examples where we've done this.

As long as one of the TF maintainers still feels the docs in the patch are an accurate reflection of what TF does N months later, I'm happy to commit this patch.

plach’s picture

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

I confirm the documentation in the patch is still up-to-date, but I'll leave someone else to RTBC it (again).

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed to HEAD. Thanks!

plach’s picture

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

Whoo-hoo! :)

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Priority: Major » Normal
Status: Fixed » Needs review
FileSize
tf-664136-41.patch703 bytes

Small follow-up: the Field language API should be cited in the main Field API documentation.

yched’s picture

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

Makes sense.

Dries’s picture

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

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)
Issue tags: -Needs documentation, -translatable fields

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

Issue tags: +Needs documentation, +translatable fields

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.