Problem/Motivation

At the moment there's no generic way to access the entity language (if available). The need for an API function to rely on for this grows as there are efforts to simplify / unify the handling of translations (e.g. #1282018: Improve UX of language-aware entity forms).
Further the lack of an unified function to fetch this information lead to several implementations of code which tries to determine the entity language (e.g. #1376126: Fix language handling for translatable fields)

Related Issues:

Proposed resolution

Introduce a function like entity_label() which takes care of the language detection for all entity types.
Proposed function signature is entity_language($entity_type, $entity).
Logic of the function:

  1. Check the entity info for $info['entity keys']['language callback'] and use it for further processing.
    This property is introduced to allow contrib modules to affect the returned language. This new callback functionality will be used by entity_translation to provide the new language-aware entity edit form.
    Since this addition is optional existing will fallback greacefully.
  2. Check the entity info for $info['entity keys']['language'] and use the defined property to fetch the language information from the entity.
    This property is introduced to get around the fact that there's no such thing as a well defined property containing the entity language information.
  3. Fallback to NULL
    The value returned in D8 by Entity::language() is LANGUAGE_NOT_SPECIFIED (which in D7 is LANGUAGE_NONE), we cannot use it here to preserve backward compatibility. In fact LANGUAGE_NONE is an assignable value and in D7 we need a way to differentiate between the case where an entity has LANGUAGE_NONE assigned and the one where it has no language property at all. See #12 for details.

Remaining tasks

Original report by plach

This has been split from #1260640: Improve field language API DX. See the OP there for a complete summary.

At the moment there is no generic way to access the entity language (if available). We need an API function to rely on for this like entity_label(). Among the rest this will be useful to fix:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

Version: 8.x-dev » 7.x-dev
Status: Active » Needs review
FileSize
21.27 KB

Actually this is targeting D7, for D8 this issue is already being addressed in #1277776: Add generic field/property getters/setters (with optional language support) for entities.

The patch looks big but it's mostly replacing the occurrences of $node->language and $comment->language with calls to entity_language().

plach’s picture

Title: Introduce entity language » Introduce entity language support

Status: Needs review » Needs work

The last submitted patch, entity_language-1495648-1.D7.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
21.48 KB

New attempt

Gábor Hojtsy’s picture

Issue tags: +D8MI, +sprint

It would be great to have a more extended description of what this is about. Also, the patch looks like coming out of the blue, but in fact it is based on discussions in the referenced issues where more extended approaches were attempted. This patch would help entity_translation.module in contrib to reuse the node form and other entity forms for translation of the entity.

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

das-peter’s picture

Added some other related issue to the issue summary.
Patch applied successfully and everything seems to work as expected.
And on my visual review I found not a thing I could complain about ;)

das-peter’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

So, RTBC? ;)

plach’s picture

Issue summary: View changes

Updated summary

das-peter’s picture

From my perspective this is RTBC, but as I don't think I've the in-depth knowledge to mark this as RTBC, I preferred to follow Gabors hint instead and updated the issue summary. I hope this makes the review easier for others.
Maybe we could ask fago for his thoughts, I'm pretty sure the entity API module will adapt this construct.

bojanz’s picture

+1 from me.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. Has great summary for committers too :)

fago’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/includes/common.inc
@@ -7759,6 +7759,37 @@ function entity_label($entity_type, $entity) {
+  else {
+    // @todo The right value for D8 should be LANGUAGE_NONE, we cannot use it
+    // here to preserve backward compatibility.
+    $langcode = NULL;

Hm, actually - why? Could you please elaborate on the different meaning between NULL and LANGUAGE_NONE here? To me, that's totally unclear.

+++ b/modules/path/path.module
@@ -291,7 +291,8 @@ function path_taxonomy_term_update($term) {
-      $path['language'] = LANGUAGE_NONE;
+      $langcode = entity_language('taxonomy_term', $term);
+      $path['language'] = !empty($langcode) ? $langcode : LANGUAGE_NONE;

This changes/fixes term url aliases to be saved language specific once we have a term language. Might deserve its own issue, but anyway it makes sense. We can also keep it here, but at least it might be worth a mention.

+++ b/includes/common.inc
@@ -7759,6 +7759,37 @@ function entity_label($entity_type, $entity) {
+ * @return
+ *   The entity language, or NULL if not found.

It does not return the language (object), but the language code. This should be visible from reading the function docs, but isn't.

+++ b/includes/common.inc
@@ -7759,6 +7759,37 @@ function entity_label($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 NULL.
+  if (isset($info['language callback']) && function_exists($info['language callback'])) {
+    $langcode = $info['language callback']($entity_type, $entity);
+  }

Afaik we have no implementation requiring that, but moreover we should not facilitate the use of such a callback as it won't work with queries at all. Thus, let's better don't add support for it in the first place.

Update: Noted that it's planned for entity translation to use it to temporarily change the entity language. If doing so, why not just change $entity->language? In both ways it's a ugly hack anyway. ;)

+++ b/modules/comment/comment.module
@@ -2028,7 +2029,8 @@ function comment_form($form, &$form_state, $comment) {
-  field_attach_form('comment', $comment, $form, $form_state);
+  $langcode = entity_language('comment', $comment);
+  field_attach_form('comment', $comment, $form, $form_state, $langcode);

This looks like an API change to me - as the default-call has changed. Actually, it looks to me as the default value of the language parameter of field_attach_form() is broken. But instead of fixing it, this patch puts the burden on entity-providing modules to fix the default theirselves in order to avoid an API change.

So maybe, it would be better to do the API-change and not put the burden on entity-type providing modules? That should fix default handling for them automatically and so create less headaches for module developers.

+++ b/modules/path/path.module
@@ -230,7 +226,10 @@ function path_node_delete($node) {
+    $langcode = entity_language('taxonomy_term', (object) $form['#term']);

Is that cast really necessary? It shouldn't be so.

Also, the doesn't the patch add support for the user account's language code as entity language? That basically makes $user->language the user's preferring language only, while in the d8 the default is imho to be both. Is there an implication for the upgrade path?
If not, at least we might want to document some where why we are not adding $user->language as entity language.

plach’s picture

Status: Needs work » Needs review
FileSize
21.86 KB
2.74 KB

Hm, actually - why? Could you please elaborate on the different meaning between NULL and LANGUAGE_NONE here? To me, that's totally unclear.
[...]
Actually, it looks to me as the default value of the language parameter of field_attach_form() is broken. But instead of fixing it, this patch puts the burden on entity-providing modules to fix the default theirselves in order to avoid an API change.

Exactly. The main problem here is that the proper default language for field_attach_form() is the entity language. Unfortunately in D7 most core entities do not have a language property, so we decided to default to the default language since we did not have a reliable entity language available.

Defaulting to the value returned by entity_language() inside field_attach_form() is a potentially dangerous API change. AAMOF if an entity is not providing the correct language value to field_attach_form(), this change would make the field widgets result as blanked when editing an existing entity (see the comment example below).

Since LANGUAGE_NONE is an assignable value we need a way to differentiate between the case where an entity has LANGUAGE_NONE assigned and the one where it has no language property at all.

Hence IMO at this point of D7 lifecycle we cannot perform such an impacting API change. This needs to be a purely additive one. For this reason we cannot default to the entity language inside field_attach_form() and, yes, put the burden on the shoulder of modules defining entities. Moreover if we returned LANGUAGE_NONE when there is no property available we would have no way to preserve backward compatibilty when passing the result of entity_language() to field_attach_form().

I realize that we could simply skip passing this value to core entities not having language support, e.g. taxonomy terms, but this way Entity translation would not have a way to make them translatable.

This changes/fixes term url aliases to be saved language specific once we have a term language. Might deserve its own issue, but anyway it makes sense. We can also keep it here, but at least it might be worth a mention.

Agreed, I added a note about this. Actually this is not a harmful change since there is no way to assign a language to terms before this patch. Hence only sites adopting code providing language support for terms (likely only ET) will be affected and this will have to be a conscious choice.

Noted that it's planned for entity translation to use it to temporarily change the entity language. If doing so, why not just change $entity->language? In both ways it's a ugly hack anyway. ;)

I agree it's an ugly hack but the language callback allows ET to implement it in a "clean" way, changing the entity property is way uglier and more error prone. Moreover It would force ET to backup the value to keep track of the actual entity language. Honestly I think the language callback should not be needed in most situations and hence modules taking advantage of it should know the implications of doing so. ET should be able to confine its usage to the entity edit/submit workflow, thus hopefully not affecting queries based on the language property. Moreover label should have the same problem but we allow a label callback, so another reason to keep the language callback is consistency.

This looks like an API change to me - as the default-call has changed.

Indeed. I reverted this hunk as I realized that comment field language is totally broken and will need to be properly fixed in a dedicated issue :(
Currently the comment form is always in the default language instead of matching the comment language, which is the right way to do things (as nodes show). Probably this has not been spotted before because comment body has always been untranslatable by default and translating comments is surely not a common use case.

Is that cast really necessary? It shouldn't be so.

Why not? $form['#term'] is an array and all the functions having an entity-type/entity couple in their signature assume an entity object. AAMOF I added it as I was getting notices.

Also, the doesn't the patch add support for the user account's language code as entity language? That basically makes $user->language the user's preferring language only, while in the d8 the default is imho to be both. Is there an implication for the upgrade path?
If not, at least we might want to document some where why we are not adding $user->language as entity language.

Not sure about this. In D7 $user->language is used only for the preferred language so I didn't think it was correct to define it as the entity language. I added a comment about this in user_entity_info().

plach’s picture

And here is the comment language issue: #1534674: Comment field language is completely broken.

plach’s picture

+++ b/modules/user/user.module
@@ -161,6 +161,9 @@ function user_entity_info() {
+        // language fields have we do not define it as the entity langauge key.

Typo. Replaced comment with:

      // $user->language is only the preferred user language for the user
      // interface textual elements. As it is not necessarily related to the
      // language assigned to fields, we do not define it as the entity language
      // key.

-14 days to next Drupal core point release.

Status: Needs review » Needs work

The last submitted patch, entity_language-1495648-14.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
21.9 KB

newlines...

bojanz’s picture

Status: Needs review » Reviewed & tested by the community

As far as I can see, fago's feedback was addressed. Back to RTBC.

bojanz’s picture

Issue summary: View changes

Added commerce integration issue.

plach’s picture

Just to be sure @webchick sees this on the next commit round.

plach’s picture

Priority: Normal » Major

Also increasing priority since lots of stuff are held back by this one.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

Looks like a great patch overall, but a couple things:

  1. The hook_entity_info() API docs need to be updated for the new options.
  2. +    // @todo The right value for D8 should be LANGUAGE_NONE, we cannot use it
    +    // here to preserve backward compatibility.
    

    @fago asked for further elaboration on this and got an excellent response here in the issue queue; can we add some of that to the comment too? :)

Also, putting on my (very new) core maintainer hat, I'm pretty scared of the "language callback" property. Without it, this patch is a really clean-looking improvement and bug fix. With it, suddenly it's the case that any module which directly accesses something like $node->language is now doing it "wrong". True, the difference might only matter when certain specific contrib modules are installed, but that won't stop bug reports being filed about it, and this would still mean Drupal core is introducing an API change.

So I'm wondering if we can talk a bit more about the consequences of that. I skimmed through #1282018: Improve UX of language-aware entity forms and got a sense of how it's being used, but what I'm not sure I understood is exactly what requires it. In other words, if we added the new API here but nothing else, what are the places in core and contrib where things like $node->language would need to change to entity_language('node', $node) to allow the new functionality to work? Maybe we can discuss a little more what the potential workarounds for those are instead.

Finally: This issue was cross-linked to #1277776: Add generic field/property getters/setters (with optional language support) for entities (for Drupal 8) a while ago, but the patch there was committed only recently. Can someone confirm that all the functionality being introduced here really does match what wound up going into Drupal 8 already? It looks like there is a standard language property for entities now, but I wasn't immediately sure about "language callback". What's the equivalent method in Drupal 8 for dynamically altering an entity's language (and if there isn't one, why do we need it in Drupal 7 but not Drupal 8)?

Gábor Hojtsy’s picture

David: thanks for the review. For the language callback, we can get this patch in without the language callback but frankly, that was the driver for getting this in. The way it is being used is for the entity form to be able to be displayed for different language version of the entity. That was not supported before this patch. I don't think we'd need any other areas in Drupal contrib updated for this to be working well for entity translation, except maybe field modules that had no regard for node language (which are already buggy). Since fields support languages all on the same level (regardless of whether a field value was originally entered on the node or on a translation, those should work with the updated entity form). @plach has this working in the entity translation module patch in context.

plach’s picture

Status: Needs work » Needs review
FileSize
24.32 KB
2.84 KB

@David_Rothstein:

The attached patch implements the suggestions from #20

Also, putting on my (very new) core maintainer hat, I'm pretty scared of the "language callback" property. Without it, this patch is a really clean-looking improvement and bug fix. With it, suddenly it's the case that any module which directly accesses something like $node->language is now doing it "wrong".

After this patch any module accessing $node->language would be doing it wrong even if we didn't introduce the language callback. However nothing is going to stop working by the simple intoduction of the entity_language() function, since its behavior is fully backward compatible. Only modules implementing the language callback would have a chance to change the way things work, but any module implementing any core hook/callback has the capability to break/alter the core behavior. I don't see how this could be classified as an API change.

In other words, if we added the new API here but nothing else, what are the places in core and contrib where things like $node->language would need to change to entity_language('node', $node) to allow the new functionality to work? Maybe we can discuss a little more what the potential workarounds for those are instead.

In core the places that would need to change are the ones touched by this patch :) Namely passing to the various field_attach_form() calls the result of entity_language() as $langcode parameter. The other core places that would need to change are the ones dealing with path aliases: the language callback allows to make those portions of code multilingual-aware by changing the entity language to the active form language. This is basically what happens also in contrib: everywhere I have a piece of code that is language-aware (as path aliases are), but that in the case of a node (entity) form would be confined to a single language per node (entity), the language callback allows to magically make it multilingual-capable with almost zero effort on the contrib module side. See #1155132-18: Add Entity Translation support to Pathauto for a typical example. See also #12 for some other detail on this.

Getting the same level of support for free is nearly impossible without the language callback. Probably simply making an entity form multilingual-aware would be very difficult, since we would need to manually change all the language codes appearing in it after any form manipulation by field_attach_form() has happened. And we would require any module performing a custom addition to explicitly take into cosideration that the form might have a language associated to it. Moreover there would be no official place where taking this language from. This looks like a nightmare scenario to me and something I'm not willing to spend a minute to code for.

Can someone confirm that all the functionality being introduced here really does match what wound up going into Drupal 8 already? It looks like there is a standard language property for entities now, but I wasn't immediately sure about "language callback". What's the equivalent method in Drupal 8 for dynamically altering an entity's language (and if there isn't one, why do we need it in Drupal 7 but not Drupal 8)?

D8 defines an Entity::language() method that basically makes any entity language aware. This coupled with the planned form language (see #1498874: Provide language awareness to entity forms (introduce the form language concept)) provides the same functionaly we are trying to introduce here, but way more cleanly. See also #1188388: Entity translation UI in core for a more general overview.

To sum up I don't think the language callback will be a problem if people make a sensible use of it, as ET is planning to do. Rejecting it would kill the work that has been done in the past 6/7 months on the Entity translation project based on a discussion held with @Gabor and @webchick, as Angie assured me she would commit the patch in that precise moment had it been ready. I really hope this is not the direction we are going to take since I'm pretty sure it would not be the best choice for the future of Drupal multilingual support.

plach’s picture

Issue tags: +API addition

Surely this is an API addition, I thought this was already tagged as such.

andypost’s picture

Issue tags: -API addition

there's a mention about forms in #1448330-25: [META] Discuss internationalization of configuration
Also I think the langcode properties/fields affects mostly on render layer of core

Gábor Hojtsy’s picture

Issue tags: +API addition

@andypost: how would that relate to Drupal 7?

plach’s picture

@andypost:

I don't get your point: are you sure you are on the right issue?

plach’s picture

Setting back to RTBC to ensure this does not get out of @David's radar.

plach’s picture

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

Don't worry, I have no intention of rejecting something that has been carefully developed for 6-7 months :) I'm just trying to make sure we're aware of the consequences of what we're doing here and have minimized any negative repercussions. Hopefully you can appreciate why this issue is a particularly tricky one; while I guess it's not quite an API change, neither is it really a standard "API addition" (since it requires cooperation from other code in order for the new API to be used). That's why I wanted to be extra sure about what other kind of code needs to change for this to work; thanks for providing that information.

After this patch any module accessing $node->language would be doing it wrong even if we didn't introduce the language callback.

Can you clarify that statement? With the exception of the language callback, I don't see how $node->language is different from entity_language('node', $node). (Unless someone wanted to try altering the 'language' key itself via hook_entity_info_alter(), but I don't think that's something that could ever work anyway.)

I've been thinking about this a little more, and one of the reasons I'm feeling better about this patch is that it actually seems very similar to the situation we currently have with entity_uri() and the 'uri callback' property. We still have code that calls "node/$nid" directly and we don't consider that a bug (and in some places it never could be converted even if we wanted to because the code doesn't have the correct context). I believe the semi-conclusion that was reached on that topic e.g. in #823428: Impossible to alter node URLs efficiently is that if modules want to try altering the standard 'uri callback', they shouldn't necessarily expect it to work in all scenarios and for all entities, but it's really more of a best effort thing designed for particular uses cases.

Given that this situation is similar and especially given that we're years past the Drupal 7 code freeze, here's what I think we should do here:

  1. Can we change the documentation for the 'language callback' property to clarify that modules which want to use it should be aware that it won't always be called; basically, that it's an "opt in" thing for code which is working with an entity to allow its language to be altered in this way?
  2. Maybe also add some documentation to entity_language() to clarify its usage; e.g. maybe it's recommended over $node->language and friends, but not required.

I think those changes will help set expectations about how these new APIs can be used.

Finally, I'm still a little unclear on the equivalent for 'language callback' in D8. Yes, #1498874: Provide language awareness to entity forms (introduce the form language concept) might help with that, but that's not in D8 yet. Also, it's the main use case this is being introduced for, but once 'language callback' exists in D7 people might start using it for other things too. I want to make sure we avoid adding a feature to D7 which disappears on people in D8. So if there is no equivalent for being able to hook in and dynamically alter an entity language in D8 currently (and I can't think of one) I think the simplest thing to do would just be to add it there also? Even though the entity system has been refactored since D7, D8 still has the 'uri callback' property, and again, this would be similar.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

Setting back to "needs review" to discuss those remaining points.

plach’s picture

Can you clarify that statement? With the exception of the language callback, I don't see how $node->language is different from entity_language('node', $node). (Unless someone wanted to try altering the 'language' key itself via hook_entity_info_alter(), but I don't think that's something that could ever work anyway.)
I've been thinking about this a little more, and one of the reasons I'm feeling better about this patch is that it actually seems very similar to the situation we currently have with entity_uri() and the 'uri callback' property.

Sorry for not being more explicit about this: as I was writing in #12, the model behind this patch actually is the entity_label() function, which, as you are pointing out, is an opt-in API function. However, leaving alone alterations to the entity keys, the entity_(uri|label|language) functions should not be optional, as they are a primitive form of encapsulation of the logic needed to return their respective information. The only reason to directly access the entity information is that in D7 we have no OOP and visibility modifiers to enforce encapsulation. When I say that after this patch lands accessing $node->language is wrong anyway, I mean that it is conceptually wrong even if things keep working exactly as before. From this POV, you are right, this is more than an API addition in the sense that suddenly perfectly legal code becomes not so legal :). However nothing should break and this is what matters here.

I believe the semi-conclusion that was reached on that topic e.g. in #823428: Impossible to alter node URLs efficiently is that if modules want to try altering the standard 'uri callback', they shouldn't necessarily expect it to work in all scenarios and for all entities, but it's really more of a best effort thing designed for particular uses cases.

Exactly. Hopefully many will use the entity_language() function but very few will mess with the language callback.

Given that this situation is similar and especially given that we're years past the Drupal 7 code freeze, here's what I think we should do here: [...]

TBH I don't see the value of adding an API function and then saying it's not required, although you'd better to. If every module maintainer replaced all the usages of $node->language with calls to entity_language() the day after this is commited my life would be way easier :)
Also there is no such recommendation for entity_label(), despite it never being used in core. If your concern is that, given the time this API addition comes up, people might be unconfortable with adopting it, perhaps we may want to provide the additional information you are suggesting above in the change notice?

Finally, I'm still a little unclear on the equivalent for 'language callback' in D8. [...]

You are right, this would have no strict equivalent in D8 atm. Personally I'm not totally sold on the value of those callbacks (uri, label) once the information they relate to is properly encapsulated (a subclass or a different interface implementation might provide the same functionality in a cleaner way). Rather I'd easily see them go away in future refinements. I, for one, would totally support the language callback to be dropped in D8. Not sure how to proceed here.

However, I made a small improvement to docs while we discuss further integrations.

tim.plunkett’s picture

Issue tags: +Needs backport to D7

Fixing tag.

plach’s picture

Setting back to RTBC to ensure @David does not miss my reply.

plach’s picture

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

Gábor/others: Any thoughts on how to proceed here?

I don't feel too strongly about changing the entity_language() documentation, since probably anyone who finds the function in the first place should just go ahead and use it... So I'm fine with mentioning its usage in the change notification and/or release notes instead.

I do still think the limitations of 'language callback' should be documented directly in the patch though; it just seems like a good way to be honest with people who might want to use it or rely on it, especially with the whole thing being a new API whose uptake we can't really be sure of. (I think we should do the same for 'label callback' and 'uri callback' for what it's worth, although that's a different issue.)

So the big question remaining is the D8 equivalent. Let's see, if everyone in D8 creates new entities using entity_create() (which I suppose they should), then maybe it's true that subclassing an entity type and overriding the language() method does provide equivalent functionality... because if your module also implements hook_entity_info_alter() and uses that to alter the entity's class, then entity_create(), entity_load(), etc. should always respect that. So perhaps that's a good enough equivalent after all?

Gábor Hojtsy’s picture

@David: Yes, we do expect all entities to be OOP-ified in Drupal 8, not everything is there yet, but it is a work in progress. Such as #1361226: Make the file entity a classed object that just got recently RTBC-ed again.

fago’s picture

Rather I'd easily see them go away in future refinements. I, for one, would totally support the language callback to be dropped in D8. Not sure how to proceed here.

I'd agree with that - I see no need for the callback in d8. Language needs to be query-able. So, if we introduce it now, we should probably mark it as deprecated?

I agree it's an ugly hack but the language callback allows ET to implement it in a "clean" way, changing the entity property is way uglier and more error prone. Moreover It would force ET to backup the value to keep track of the actual entity language.

Still, we are in fact introducing this for ET only or are there any other use cases as well?

If not, I'd tend to have ET stay with the entity property change. Backing up the real-language shouldn't be hard (e.g. use an #entity_builder) and sounds less error-prone to me than the language callback variant.
Problem of the language callback variant: As a module developer I'd expect the entity type has a not query-able language given the callback is used, so I'd have to disable any functionality depending on a queryable language for this entity type.

plach’s picture

@David:

I do still think the limitations of 'language callback' should be documented directly in the patch though; it just seems like a good way to be honest with people who might want to use it or rely on it,

I added some additional documentation providing infromation about the relative unreliability of the language callback.

@fago:

Still, we are in fact introducing this for ET only or are there any other use cases as well?

Well, it's no secret I came up with this solution having ET in mind but, as I hinted in the docs, the language callback may be useful to provide language support to entity types not having it from other modules than the defining ones.

If not, I'd tend to have ET stay with the entity property change. Backing up the real-language shouldn't be hard (e.g. use an #entity_builder) and sounds less error-prone to me than the language callback variant.

Backup is not my primary concern, actually I'm more worried about not being able to persistently change the value for the whole request processing and that the replaced value might be stored to the DB by contrib/custom code. However if there is no alternative I'll try to go this way. I hate this idea though, as it will further delay a "stable" release of ET.

Problem of the language callback variant: As a module developer I'd expect the entity type has a not query-able language given the callback is used, so I'd have to disable any functionality depending on a queryable language for this entity type.

This makes totally sense but, given the 'volatile' nature of the value returned by the language callback, what about suggesting that the entity is still queriable by language if it defines a language property? This would serve as the main entity language source albeit being alterable (see the attached interdiff).

I still have the entity_label() parallel in mind: if I define a label callback for each available entity type to translate their labels on rendering, I don't expect those types to become suddenly non-queriable by label.

David_Rothstein’s picture

Problem of the language callback variant: As a module developer I'd expect the entity type has a not query-able language given the callback is used, so I'd have to disable any functionality depending on a queryable language for this entity type.

That kind of thing is one of the reasons I wanted the documentation added here - we definitely don't want to give people that impression. Do you think the new documentation added in the above patch is sufficient to communicate that? It looks pretty good to me.

+ *     language callback can be used to return the proper value. The language
+ *     callback is meant to be used primarily for temporary alterations of the
+ *     language property: entity-defining modules are encouraged to always rely
+ *     on it, instead of using the language callback as the main entity language
+ *     source.

Not sure if there's a typo above or it was just unclear to me, but "entity-defining modules are encouraged to always rely on it".. makes it sound like "it" refers to the language callback property. Perhaps change to "entity-defining modules are encouraged to always rely on the language property" to make it clear what is being referred to.

plach’s picture

fago’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/includes/common.inc
@@ -7773,6 +7773,47 @@ function entity_label($entity_type, $entity) {
+ * This is an opt-in API function, thus modules implementing the language
+ * callback should be aware that it might not be always called.

That confuses me? opt-in API? :) I guess it's more worth documenting it for the label callback, not for the API function. The API function is save to call.

+++ b/modules/system/system.api.php
@@ -97,6 +97,19 @@ function hook_hook_info_alter(&$hooks) {
+ *     the 'language' element of the 'entity keys' should be enough. However
+ *     some entity types might not define such a property, in which case the
+ *     language callback can be used to return the proper value. The language

For adding language support, I think the preffered way should be to alter in a proper language column using hook_schema_alter().

+++ b/modules/system/system.api.php
@@ -97,6 +97,19 @@ function hook_hook_info_alter(&$hooks) {
+ *     language property: entity-defining modules are encouraged to always rely
+ *     on this property, instead of using the language callback as the main
+ *     entity language source. In fact not having a language property defined is

Again, this confuses me. So if I provide an entity type, I should not use this API function? Or not use use the callback?

+++ b/includes/common.inc
@@ -7773,6 +7773,47 @@ function entity_label($entity_type, $entity) {
+    // @todo The right value for D8 should be LANGUAGE_NONE, we cannot use it

It's fun making todo's that are already done :)

Should we make a change record for the usual invocation of field_attach_form() being changed?

Also, could we please properly communicate the taxonomy path language changes as well?

plach’s picture

Status: Needs work » Needs review
FileSize
25.03 KB
4.83 KB

@fago:

That confuses me? opt-in API? :) I guess it's more worth documenting it for the label callback, not for the API function. The API function is save to call.

Agreed, moved this sentence to the language callback docs.

For adding language support, I think the preffered way should be to alter in a proper language column using hook_schema_alter().

I didn't mention this possibility since in the past messing with core table caused upgrade path troubles. However I removed the related doc lines.

Again, this confuses me. So if I provide an entity type, I should not use this API function? Or not use use the callback?

I tried to say that when you define a new entity you'd better always define a corresponding language property, since relying only on the callback has implications on the entity queriability. Hopefully now docs are more clear on this.

It's fun making todo's that are already done :)

Yeah, I hoped this would get in far earlier ;) Slightly adjusted the docs here too.

Should we make a change record for the usual invocation of field_attach_form() being changed?

Do you mean warning people that entity_language() is the new preferred default for the langcode parameter of field_attach_form()? Yes, I definitely think we should.

Also, could we please properly communicate the taxonomy path language changes as well?

Well, actually nothing would change here unless one enables a module implementing the language callback for taxonomy. So this is only a future possibility. However it might make sense to provide a note about this too.

fago’s picture

Status: Needs review » Reviewed & tested by the community

Well, actually nothing would change here unless one enables a module implementing the language callback for taxonomy. So this is only a future possibility. However it might make sense to provide a note about this too.

If I'm not mistaken this allows for language-specific path alias for terms?

Do you mean warning people that entity_language() is the new preferred default for the langcode parameter of field_attach_form()? Yes, I definitely think we should.

ok, so we'll need two change then? One for entity_language() got introduced and one for field_attach_form() should be used differently now?

@improvements: Yep, that's better now thanks. The language callback docs seem to be a bit verbose though, but better verbose docs as too less. Then, I still dislike the language callback, but that has been discussed extensively in that issue anyways and it seems to be the best way forward now. So back to RTBC.

plach’s picture

If I'm not mistaken this allows for language-specific path alias for terms?

Yes, only through the installation of a contrib module implementing the language callback though. The core code keeps creating language neutral aliases for taxonomy terms. This opens new possibilities but there is no actual change in the current behavior to notify.

ok, so we'll need two change then? One for entity_language() got introduced and one for field_attach_form() should be used differently now?

Not sure about the best practices here: maybe @David can enlighten us?

Then, I still dislike the language callback, but that has been discussed extensively in that issue anyways and it seems to be the best way forward now. So back to RTBC.

A huge thank you for this. I hope you won't regret for not being stronger about it, but honestly I feel we already have too much hacks in the entity translation workflow to introduce yet another tricky one. That said, I don't like this solution too and I wish I realized during the D7 API polishing phase that a function like entity_language() could still be introduced. This would save us many headaches but, as you know better than anyone else, all the D7 core entity API is half-baked and this is yet another attempt to make it less incomplete.
Thanks again :)

David_Rothstein’s picture

Thanks for your patience on this issue. I think it looks good now, and I'll leave it for another day or two in case anyone else wants to review it, but after that I'm intending to commit it.

Regarding the change notification, I would say it makes sense to combine the entity_language() and field_attach_form() notifications into one. (Basically, they both fall under the category of "things that you're recommended to do if you want your module's code to support translations better" so it's best to summarize that all at once.)

Actually, though, I think we still will need two change notifications:

  1. One for Drupal 7 explaining the entity_language(), field_attach_form() and hook_entity_info() changes.
  2. One for Drupal 8 explaining the "removal" of entity_language() and the language callback, and how to convert any Drupal 7 code that was using them. Kind of odd since by definition no Drupal 7 code is using it yet :) But of course the situation will be different by the time Drupal 8 is released.
David_Rothstein’s picture

Title: Introduce entity language support » Change notification for: Introduce entity language support
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +7.15 release notes

Committed to 7.x and added to CHANGELOG.txt - thanks! http://drupalcode.org/project/drupal.git/commit/a121523

Moving back to active for the change notifications.

One small question for possible followup:

+ *     - language: The name of the property, typically 'language', that contains
+ *       the language code representing the language the entity has been created
+ *       in. This value may be changed when editing the entity and represents
+ *       the language its textual components are supposed to have. If no
+ *       language property is available, the 'language callback' may be used
+ *       instead.

Should this be changed to have a sentence like "This entry can be omitted if..."? The other optional entity keys in hook_entity_info() have sentences like that, and not all entities actually support a language property (or even make sense to).

plach’s picture

Title: Change notification for: Introduce entity language support » Introduce entity language support
Priority: Critical » Major
Status: Active » Needs review
FileSize
826 bytes

Who-hoo!

And here are the change records:

http://drupal.org/node/1626346
http://drupal.org/node/1626352

plach’s picture

Priority: Major » Normal

Unblocking the major queue.

fago’s picture

Priority: Normal » Major

thanks!

I've edited [#1626352] to simplify the last part of the sentence a bit. [#1626346] gives a good summary of the whole issue, but lacks a really short overview. Could we add something in the lines of:

"A generic way to access the entity language (if available) has been introduced: now it is possible to use the entity_language() function to retrieve an entity's language, if the entity type defines it. In order to support entity translation in your entity forms, make use of it when calling field_attach_form() as following:

-  field_attach_form('node', $node, $form, $form_state, $node->language);
to
+  field_attach_form('node', $node, $form, $form_state, entity_language('node', $node));"

Done, that's all someone really needs to know. Continue reading the details if you care. Makes sense?

fago’s picture

Priority: Major » Normal
plach’s picture

Well, that summary is very specific to the entity translation part, what about moving the last paragraph ("This is a full backward-compatible API change [...]") on top after the very first one? Changes to the other CR look good.

fago’s picture

Yep, that would be a good idea as well. Still, I think we should include the field_attach_form() before/after comparison in the "overview", so you don't have to go through all details in order to know what to do.

plach’s picture

Updated the CR moving up the last paragraph and adding the text from #49.

@fago:

What about the small follow-up patch?

plach’s picture

Status: Needs review » Reviewed & tested by the community

Tentatively marking RTBC, since the patch is pretty staightforward.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

#47 seems to be what David was asking for, so committed/pushed to 7.x. Thanks!

plach’s picture

Priority: Normal » Major

Thanks!

Restoring the original priority.

David_Rothstein’s picture

Yup, looks great; thanks for the quick turnaround.

I also went ahead now and linked to the change notification in CHANGELOG.txt: http://drupalcode.org/project/drupal.git/commit/d072581

plach’s picture

FYI the D8 counterpart of the default for field_attach_form() is being addressed in #1499596: Introduce a basic entity form controller.

Gábor Hojtsy’s picture

Issue tags: -sprint

Done, can move off of sprint now.

Status: Fixed » Closed (fixed)

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

hass’s picture

Status: Closed (fixed) » Active

My module uses $node->language and I'd like to convert them, but how can this backward compatible? The entity_language() function is only available in Drupal 7.15+ and not in 7.14 and before. This means it is not backward compatible and if a module implements the new function, it forces everyone to upgrade to 7.15 or later. Otherwise the users may see messages like function does not exits and so on. How can I prevent this?

Should this solved this way?
dependencies[] = system (>7.14)

tim.plunkett’s picture

Use function_exists('entity_language'), or make your module depend on 7.15 like you suggest

hass’s picture

Status: Active » Closed (fixed)

I cannot add everywhere function exists calls!

hass’s picture

Status: Closed (fixed) » Needs work

Reopening as http://drupal.org/node/1626346 needs more information like dependencies[] = system (>7.14).

bforchhammer’s picture

You can avoid the dependency by using a "compatibility layer" such as the following:

/**
 * Returns the language code of the given entity.
 *
 * Backward compatibility layer to ensure that installations running an older
 * version of core where entity_language() is not avilable do not break.
 *
 * @param string $entity_type
 *   An entity type.
 * @param object $entity
 *   An entity object.
 * @param bool $check_language_property
 *   A boolean if TRUE, will attempt to fetch the language code from
 *   $entity->language if the entity_language() function failed or does not
 *   exist. Default is TRUE.
 */
function pathauto_entity_language($entity_type, $entity, $check_language_property = TRUE) {
  $langcode = NULL;
  if (function_exists('entity_language')) {
    $langcode = entity_language($entity_type, $entity);
  }
  elseif ($check_language_property && !empty($entity->language)) {
    $langcode = $entity->language;
  }
  return !empty($langcode) ? $langcode : LANGUAGE_NONE;
}
webchick’s picture

Status: Needs work » Fixed

This issue is fixed, if you have a support request about how to specify minimum dependencies in your module, please open a separate issue for that.

hass’s picture

Status: Active » Fixed

I'm only saying that there is a major DX issue. Saying it is backward compatible is plain wrong and this need to be corrected in http://drupal.org/node/1626346

hass’s picture

Status: Fixed » Active

The update notice needs changes to be clear. I would have replaced it without thinking more about. It's plain wrong that I should just replace all $node->language with entity_language().

webchick’s picture

Those are just wiki pages. Please feel free to edit it however makes it clear.

plach’s picture

I added some notes to the change notice as suggested by @hass. Hope it looks better now.

hass’s picture

Thanks for adding these. I'm going with the system dependency key for now as it looks cleaner to me. Hopefully not so many people are running old versions.

The code example in #65 looks better than the snipped added to the docs as the example only returns LANGUAGE_NONE where we have mostly a language in the entity (pre 7.15). I would suggest to change the example code. I'm not mr. entity, so I'm not changing any articles about these topics myself as I could be wrong.

Status: Fixed » Closed (fixed)

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

hass’s picture

@bforchhammer: Shouldn't it not this? In entity_language() function it's noted that returning LANGUAGE_NONE may introduce DC issues.

/**
* Returns the language code of the given entity.
*
* Backward compatibility layer to ensure that installations running an older
* version of core where entity_language() is not available do not break.
*
* @param string $entity_type
*   An entity type.
* @param object $entity
*   An entity object.
*/
function mymodule_entity_language($entity_type, $entity) {
  $langcode = NULL;
  if (function_exists('entity_language')) {
    $langcode = entity_language($entity_type, $entity);
  }
  elseif (!empty($entity->language)) {
    $langcode = $entity->language;
  }
  return $langcode;
}
plach’s picture

This code will interpret $user->language as the entity language, which is not correct.

hass’s picture

It's $node->language not $user->language. It's the way how we detected the node language before we had 'entity_language' function... the only think strange in pathauto is LANGUAGE_NONE that is documented in entity_language to break something - I have not understood.

plach’s picture

The following code:

<?php
  $langcode = mymodule_entity_language('user', $user)
?>

might not always work as intended because $user->language holds the user preferred language which may not be the same of the user entity language. We had this issue in Pathauto. If you wish to use this code in the change notice you should document this possibility and retain the complete function which had a flag to skip checking the language property. Btw, this is why I originally used a simpler example.

hass’s picture

Well I have not understood the $check_language_property in pathauto and it is not properly documented why it's there. Now I have more question marks in mind... can you provide examples when a $user->language is not the same like an user entity language or what's the difference? I just thought I have finally understood what an entity is, but I start to doubt about this again. I'd really like to understand this as I planed to implement user fields support in linkchecker and I do not like to run into unknown troubles.

Should we add a $entity->type == 'user' condition to make it easier or is this really conditional per individual request? This may get answered by the above answer. :-)

hass’s picture

Issue summary: View changes

Updated the issue summary as the D8 issue has been committed before this one.

fago’s picture

Can someone confirm that all the functionality being introduced here really does match what wound up going into Drupal 8 already? It looks like there is a standard language property for entities now, but I wasn't immediately sure about "language callback".

Actually, no we miss the language key in Drupal 8. See #2143729: Entity definitions miss a language entity key

What's the equivalent method in Drupal 8 for dynamically altering an entity's language (and if there isn't one, why do we need it in Drupal 7 but not Drupal 8)?

We don't need the language callback though, as it was only introduced (late) in Drupal 7 for entity translation for being able to hack-around core APIs - which is not necessary for d8 any more.