Improve field language API DX

Nice! :) Adding to #1260534: META-META: Make language support awesome in Drupal 8

Removed a wrong subtitle.

looks like this would be covered by something like

Entity::get($property_name, $language = NULL)

?
So $language is optional and can have a proper default. Why should we have two different methods/functions for that?

Fixing tag.

field_get_raw_items($entity_type, $entity, $field_name, $langcode = NULL): helps but not enough. A good DX would remove $langcode entirely because I still have no idea what $langcode is and what should I provide or any at all -- my best understanding right now is that most of the time I don't. So if we are enhancing the DX why not go the whole nine yards and just remove it? If it's used really rarely the accessing the raw $entity->$field_name is good enough for that use case.

Er, I myself don't see the use of field_get_raw_items() if it has no $langcode param.
The goal is : give me the values of this field in this language if it as any (and without any language fallback rules), right ?

Having a getter and setter that acts only on one single language (the $entity language, if I get things right ?), sounds valid, but why forbid access to the other languages ?

The reason we forbid access to other languages because we want a simple DX. It will still be very, very hard to comprehend the field invoke workflow due to the multilingual fields (this is a major gripe of mine) but at least for contrib developers we provide something. For core developers, hopefully documentation will be enhanced.

Actually, I don't see where the difference between field_get_items() and field_get_raw_items() should be. In order to improve DX we should not introduce yet another function and confuse developers by forcing them to pick one, but do a helper function that fulfills the common needs by default, while ideally it handles specific needs too.

@chx: I don't see how hiding the language from the developer helps to improve DX when it's there. It's there and we can't do away with it now, so let's improve handling it. So an optional langcode parameter makes perfectly sense to me - you don't need to care if you don't want to.

@field_get_raw_items() vs field_get_items():
Why shouldn't field_get_items() default to the entity language too? It seems weird to me to have two very similar functions with different language defaults! DX-- :(
Any reason field_get_items() cannot default to the entity language?

@fago pray tell me what's the use case of supplying a $langcode ? If I'd know that...perhaps , you know, should be in the doxygen :)

This is issue is primarily about D7 DX. I hate this choice, but what you are proposing is an API change that cannot be backported to D7 as it would break BC.

Indeed.

@fago pray tell me what's the use case of supplying a $langcode ? If I'd know that...perhaps , you know, should be in the doxygen :)

Of course.

So what's the use-case of getting the field value using the entity language? If I don't care about the language it should probably default to the language currently displayed - as field_get_items() does?

I still think having to functions doing the same with the only difference being the default for $langcode is going to confuse people even more. For them, to get the difference between those functions they need to understand the difference between the entity default language and the display language - thus they'd need to understand all the language quirks. :(

I see and I agree that the default should be reliable and not vary by requests.

Well, to solve this nightmare I'd think field_get_items() and field_set_items() should be what you propose for the field_get_raw_items() + the build/render phase should use field_language() and specifically use the right display language. That way we'd have a single and straight-forward to use function.

I know that would be an API change, but maybe we should consider it? I guess, some display related modules (like Views?) would have to adapt to go with the right default language..

To go without API change in D7 we could leave field_get_items() as is and only introduce entity_language(), which can be used to figure out the right language to use by default. But that puts the burden again on average developers using field-values.. :(

I'm all about DX improvements, but not at the expense of the 150K+ users of D7 out there in the wild right now. If we set up a pattern of "you download a stable release of Drupal and your site breaks," it won't be long before that number starts declining, rather than climbing. It's also not fair to contributed module developers to have to "chase HEAD" during a stable release. So no, we can't break APIs for this.

ad #19:
Thanks for your input - makes sense.

My counter proposal: let's mark field_get_items() as @deprecated and introduce field_view_items() which would behave the same way. This would be consistent with field_view_field() and field_view_value().

I like that proposal, as field_get_items() is really misnamed. So that's probably the best we can do.

Not sure whether we should do field_get_raw_items() and field_set_items() or field_get_raw_items() and field_set_raw_items(). Maybe better the last one, as field_set_items() might let developers think there should be field_get_items() too (which is there but not what they expect).

Subscribe. I'm still using D6-era getters, because it's simpler to write out $node->field_name[$node->language][0]['value']; than it is to keep having to remind myself how all the entity and field API wrappers work with languages (regardless of how many posts I read on the Planet feed extolling the virtues of the new field API...).

Here is an attempt to get this implemented.

entity_language will attempt to use $entity->language, but also supports a using a callback function in entity_info if $entity->language does not exist.

field_(get|set)_raw_items are implemented without using any fallback and will use the entity language if the field is translatable, or will use LANGUAGE_NONE if not translatable. I have left out the $langcode parameter because, at least with the current implementation's lack of fallback, if you know the langcode you want to use then you can access the entity directly.

field_process_translations is my implementation of the originally proposed 'field_process_items'. I think the original names was somewhat confusing, since at least to me it made it sound like it was iterating over the items, instead of the translations. What do others thing about this? I also pass tons of arguments to that callback, some of which may be unnecessary.

I've reworked entity_language to not have bundle-specific callbacks, and to have a 'language' entity key. I also fixed the typos and added return value aggregation for the field_process_translations.

Not too fond of field_view_items() myself. The current field_view_*() funcs are really related to displaying stuff, and they return render arrays. Getting raw value items is something else.

Subscribing.

+++ b/includes/common.inc
@@ -7610,6 +7610,35 @@ function entity_uri($entity_type, $entity) {
+  // Invoke the callback to get the language. If there is no callback, try
+  // to get it from a property of the entity, otherwise LANGUAGE_NONE.
+  if (isset($info['language callback']) && function_exists($info['language callback'])) {
+    $langcode = $info['language callback']($entity_type, $entity);
+  }

Do we really need to have the callback functionality or would a language key suffice? Keys are better as they allow to query for a certain language and having entity-language that is not query-able kind of sounds silly to me.

+++ b/modules/field/field.module
@@ -971,6 +971,91 @@ function field_get_items($entity_type, $entity, $field_name, $langcode = NULL) {
+    $field = field_info_field($field_name);
+    $available_languages = field_available_languages($entity_type, $field);

Do we have notion to get all available languages for an entity? Or is that out of scope for this patch?

+++ b/modules/system/system.api.php
@@ -87,6 +87,9 @@ function hook_hook_info_alter(&$hooks) {
+ *   - language callback: (optional) A function taking an entity type and an
+ *     entity as arguments and returning the langcode of the entity. The entity
+ *     language is used as the default language when processing field values.

I think we need a clear definition of what the entity language is, independent of fields. So is it the default language for a given entity? Also see #1277776-19: Add generic field/property getters/setters (with optional language support) for entities.

8 days to next Drupal core point release.

Do we really need to have the callback functionality or would a language key suffice? Keys are better as they allow to query for a certain language and having entity-language that is not query-able kind of sounds silly to me.

Gabor couldn't think of a use-case for language callbacks, so I'd propose not supporting that.

I think we need a clear definition of what the entity language is, independent of fields. So is it the default language for a given entity? Also see #1277776-19: define language handling for entities.

#1277776: Add generic field/property getters/setters (with optional language support) for entities now has a definition:

* The entity language is basically the *default* entity language, i.e. callers can expect each translatable property to have a value in that language, or there is no property value. Translations to other languages might exist, but need not.

So let's improve the entity_language() docs to clarify that.

Do we have notion to get all available languages for an entity? Or is that out of scope for this patch?

Not sure, whether it's "in scope" for that patch. How does "entity_translation" solve that?

Function naming ideas:
1) entity_languages()
2) entity_translations()
3) entity_available_translations()

I'd prefer 2) over 1) because it clarifies which languages it actually returns.

Yes, overriding the entity language for the purpose of displaying a different language version of it in a form does sound hackish, and I understand it is the best we can do for now in contrib, but we should separate the entity language from the form language in D8.

Looks like the other issue fizzled out a little over 2 months ago with being marked fixed then a little disagreement that didn't result in it being opened again. I'm just an interested observer, not a participant, but thought I'd give this a little bump in case it fell of peoples' radar due to the other one being marked as closed/fixed.

Michelle

Tagging for the content handling leg of D8MI.

+1 on splitting the entity_language() part out, if we can move on faster this way. entity_language() is as of now required in order to be able to correctly read/write translatable field values at all!

FYI: I've been working on fixing this for the entity-metadata-wrappers of the entity API module: #1376126: Fix language handling for translatable fields

@fago: would that temporarily break the translatable fields API or you mean we should get in entity_language() first?

A possibly iterative approach attacking field languages first sounds perfectly fine to me. Especially since the entity language part seems to require a lot more architectural re-thinking at this point. So I'd say; go ahead and keep on rocking :)

Sry for being imprecise, I meant going with entity_language() first as suggested by plach in #35. We need that helper function for fixing field-language as the entity-language is the default-language for translatable fields.

With all of the work done on #1346214: [meta] Unified Entity Field API and with #1277776: Add generic field/property getters/setters (with optional language support) for entities and #1495648: Introduce entity language support in, how critical or relevant is this issue?

Sounds like a duplicate to me?

Ok then how does this map with the magic getter/setter rework of field property access (as explained in http://groups.drupal.org/node/237443 for example)?

I don't see the bigger picture. Are you saying you target 1 function for D8 inclusion, the rest for D7? Why is this issue marked for D8 then?

Then it would be more like a major for D7, right?

#49 makes a lot of sense to me

OK, let's put this back in the critical queue where this belongs. Note: just because I was alone filing an issue about this, people were unhappy back before D7 was released (https://twitter.com/quicksketch/status/11505377996) and look at this in #21 $node->field_name[$node->language][0]['value']; which I always suspected being wrong and I now suspect even stronger. However, #56 still does not help #7 -- when should I provide $langcode to these new wrappers and what should I provide?

Add to "(optional) The language code to be used if the field is translatable. Defaults to the entity language." use what you said "This argument is used very rarely, unless you know what you are doing, it should be left empty."

That is crystal clear, thanks.

Updated issue summary.

Better to have this in 7.15 than not to. We can give examples in 16.

Agreed.

Updated the summary to reflect the RTBC patch in #60

It sounds to me like most of this is already in D8 (or has equivalent methods in D8), but not all of it... For example, we still have field_get_items() in D8 (and no OOP equivalent of that one yet, that I know of), so why isn't that renamed to field_get_display_items() in D8 also?

In addition, some of the documentation and tests look to me like they should really go into D8 too.

***

I also noticed a few nitpicky things:

1. If field_get_items() is deprecated, shouldn't the patch change all instances of it in core to field_get_display_items() instead?

2.  /**
   + * @deprecated
   + * Returns the field items in the language they currently would be displayed.
   

   The @deprecated should probably go later on in the PHPDoc, I think right before the @see line based on other examples.

3. There are a few instances of this:

   + *   Defaults to the current language. This argument is used very rarely, unless
   + *   you know what you are doing, it should be left empty.
   

   The comma before "unless" should be changed to a semi-colon.

***

Also, some general feedback; as a developer, I think this patch would help a fair amount, but I'm still very confused about the big picture. There's an entire field_language @defgroup at the top of field.multilingual.inc with an overview of the language system; I would find it really helpful if a lot of the practical information about "which function to use when" existed there, and the individual functions' PHPDoc could link to it.

What I think I'm missing overall is that I know functions like _field_invoke_multiple() do a foreach() loop over languages when they set things, so to be safe I've always done something similar in custom code. If the field structure has more than one language why is it safe for field_set_raw_items() to only modify one of them, rather than doing a foreach loop? I'd love to see documentation explaining fundamental things like that.

***

Those last couple of paragraphs are more general feedback that don't necessarily have to be solved in this issue, but I do get the strong impression that we need to get some other things in shape for D8 first, before committing this patch to D7. Sorry.

Note that our priority is to support the inclusion of the property API from @fago, which changes all this thing up for Drupal 8. So as it stands here, to me this looks like this being moved back to D8 will postpone this until it becomes irrelevant again on Drupal 8 and then we can move back to Drupal 7 with need of a possibly painful reroll. Given we have to focus on other things to solve key problems in Drupal 8 and the DX is taken care of by property API basically.

I'm moving this down to major. Sorry, chx. We are over thresholds, and my rationale is that it cannot by definition be critical because we shipped D7 despite it, and the D8 situation is already vastly cleaned up.

#61: drupal-tfdx-1260640-61.patch queued for re-testing.

Tests updates will be addressed in a follow-up

It's been two years since the last, agreed upon patch in this issue. See the avatar to the left for more.

I just spent part of the last few days in some kind of multilingual Drupal purgatory while working on a particular site :) I remembered this issue and consulted the patch here to get some help, but it actually left me pretty confused...

The big questions I still had were:

1. The difference between "rendering" and "manipulating" entities (as described in the patch) is not clear to me technically; in some cases it's obvious I'm doing one or the other, but in other cases (loading an entity and checking a particular field value) it seems like neither and I had no idea which function to use. In addition you can have more complicated use cases involving more than one entity; if I'm in the middle of rendering a node and need to make a decision about node rendering based on the value of a field attached to a taxonomy term, which language do I use when getting the field value from the term (the node's display language? the term's display language? the term's working language?). I think we need a more precise technical definition. A concrete example would also be extremely useful here, of a situation in which field_get_raw_items() and field_get_display_items() return different values and why (this could be part of the function docs if the example isn't too convoluted).
2. When setting field values, I also had trouble (similar to my comment from a couple years ago) figuring out which language or languages to use. If I am saving a field value programmatically (let's say a cron run which loads entities and sets a field and saves them) I may want to save it in a way that guarantees it has that value regardless of which language the entity is being viewed or edited in. The only way I could figure out how to do that was to loop over field_available_languages() and set it for each, which seems to work but results in extra rows in the field database tables after saving (one for each possible language on the site, regardless of whether a translation actually exists for that entity in that language). It doesn't seem to cause any harm but it makes me think I'm doing something wrong. I did not find as many situations where field_set_raw_items() from the patch here seemed useful, since it only sets the field value for one language.

Other smaller issues:

1. To keep terminology consistent I think we need to deprecate field_language() in this patch and replace it with a new function called field_display_language().
2. Perhaps field_working_language() should have a static cache, like the other field languages do? (I'm not sure if this is actually important.)
3. The "display" vs "raw" terminology is confusing (it sounds like it has something to do with sanitizing text). Would something like field_get_items_in_display_language() and field_get_items_in_working_language() be good, or too verbose?
4. I am not sure "working" is the right terminology either, but it could be (depends on the answers to the bigger questions I asked above).

Hope this is useful feedback and questions - I would really like to get something into Drupal 7 for this issue, to improve the developer experience.

Adding a related issue, which could be used for improving the overall documentation at the top of field.multilingual.inc (with this issue just about adding the new functions and documenting them individually).