Add generic field/property getters/setters (with optional language support) for entities

I think you referenced the wrong issue, should be #1184944-35: Make entities classed objects, introduce CRUD support and #1184944-36: Make entities classed objects, introduce CRUD support

Subscribe

tagging

on this one

My short feedback on this: language should be a mandatory piece of property on an entity. Period. Each entity should be able to tell its language. If its "language neutral", so be it, but it should be able to tell that to us, and let the user enter that metadata. If Drupal 8 is to become a fully multilingual system, there is no way to avoid this. This would mean that we'd move language information support from the node, comment, etc. module level to the general entity level.

(Note that user entities do have a language property right now which *is not* their entity language, it is the user's preferred language. Probably we should rename that, and use the language property consistently there to signify the language of the information the user provided, not the language the user wants to see on the site).

Here is my first attempt to address this: the implemented logic is more or less the same than the one in #1260640: Improve field language API DX. There is a compatibility layer to support language-unaware properties but I think this should be removed in favor of always accessing properties through accessor methods and defining properties as private or protected when possible.

As mentioned in #1 only field language is covered for now.

Feedback welcome :)

This patch depends on:

#1184944: Make entities classed objects, introduce CRUD support
#1018602: Move entity system to a module

Here is an updated version for the testbot including the patches above. Please review #8.

+++ b/modules/entity/entity.module
@@ -424,6 +424,34 @@ function entity_label($entity_type, $entity) {
+ * @param $entity_type
+ *   The entity type; e.g., 'node' or 'user'.
+ * @param $entity
+ *   The entity for which to generate the label.
+ *
+ * @return
+ *   The entity language code, or LANGUAGE_NONE if not found.
+ */
+function entity_language($entity_type, $entity) {

stop passing around the entity_type, this is a method every entity object must have. Type hinting with EntityInterface on $entity would be good.

+++ b/modules/entity/entity.module
@@ -424,6 +424,34 @@ function entity_label($entity_type, $entity) {
+
+  return $label;
+}

label? should be $langcode, right?

Hm, in fact language handling is also happening in #1260640: Improve field language API DX. Is there a sizable overlap between the two issues or how do they relate? If the D7 issue adds much more of a language support on the entity API, should we just take that forward here?

@klausi:

stop passing around the entity_type, this is a method every entity object must have. Type hinting with EntityInterface on $entity would be good.

I think entity_language() could be dropped altogether and just moved to the Entity class. This would avoid the need of having language or language callback entity info keys, since we could provide a base implementation accessing a (protected) defined property and for any other need the method could be overriden. However here I tried to maintain consistency with entity_label(). I think we should move this discussion to #1184944: Make entities classed objects, introduce CRUD support and revisit it here afterwards.

Fixed the entity_language() function.

@Gabor:

Hm, in fact language handling is also happening in #1260640: Improve field language API DX. Is there a sizable overlap between the two issues or how do they relate? If the D7 issue adds much more of a language support on the entity API, should we just take that forward here?

Well, both are introducing the same entity_language() function atm (not sure in the future). Accessor methods are replacing accessor functions but the implemented logic is the same. We are missing something equivalent to the field_process_translation() function, but that is something that we should revisit once the Field API is moved to a real OOP implementation.

Attaching two patches: again, review the tiny one, the big one is for the bot.

Copying from the CRUD thread:

For language, could we do something like $entity->setPreferredLanguage($langcode); rather than passing an argument to get()? that would mean you only have to set things once then you can just access properties as usual, and if you don't set it you get the default. It'd also feels a bit easier to move that to depending on context later on.

This would mean we don't have to implement getters for fields and properties, and ought to make things more compatible with the $context object once that's in place (i.e. eventually entity language could default to the content language in context).

+1 to catch's idea here, it seems very clean and easier to adjust later as we need to (also subscribe)

This would mean we don't have to implement getters for fields and properties, and ought to make things more compatible with the $context object once that's in place (i.e. eventually entity language could default to the content language in context).

I missed this one on the other issue: how would this default language be used without getters/setters?

It'd work with either __get() or get() or __call(), as opposed to only get() or _call(). I guess __get() is still a getter but that's what I meant ;)

I'd say no. As we are translating entities, not property values, it should invalidate nothing *or* the whole translation of the entity. So it might be a good idea to add an method that says "invalidateTranslations()", which removes *all* translated property values (or just marks them as outdated?)

We have built-in support for this in the core translation module: while editing a node translation, the user has the possibility to mark all the other ones as outdated. Entity translation extends this behavior to every entity. At the moment it's implemented as a method setting an internal property for each available translation.

Honestly I won't totally throw away the idea of marking each property as outdated, since it would make far easier for the translator to locate the part pf the translation that needs to be updated. Combine that with revisions and diff support and you get a very complete tool :)

Yes, I agree it should mark them translated, not remove them. Users would be horrified for their careful translations being removed. Also agreed that marking the whole thing outdated combined with revisions and diff module should give you a powerful tool, not sure we need property/entity level flags since that would necessitate a checkbox on each of them, because we rely on the user to say it was a substantial change or not. Think a typo fix in the English text will not necessitate a translation update, but a new paragraph added probably will.

Not sure we need property/entity level flags since that would necessitate a checkbox on each of them

I was thinking about keeping track of every field/property change internally as a support for the diff tool, but this is sci-fi compared to the problems we have to face now :)

Re: end of #24 :
We considered something similar back when discussing the data structure for translatable fields - a kind of switch that substitutes the field values in a given language, putting one single language forward while the collection of values for all languages are kept in a "less visible" bucket.

IIRC, we punted with that approach, not feeling really comfortable with the conceptual impact of introducing a notion of "state", which afaik is completely absent in our current APIs and DX concepts, and is not a minor change - an $entity is not "just the same $entity throughout its load / display / modify / save cycle" anymore.

Not to say that such an approach shouldn't be considered again, just saying there is a conceptual shift I personally don't fully measure. We should probably dig for that previous discussion in the original TF d7 thread.

I personally don't think we should complicate our life with this state concept, contrib modules can provide that by overriding the displays. IMHO we should skip this and focus on our core goals.

#28: drupal_entity_lang.patch queued for re-testing.

I think this generally looks good except langauge_load() unfortunately is located in locale.module, so you cannot just use it as you did. We can move that too to bootstrap.inc, and so some similar logic like in langauge_list() to work when we have no locale, and just work with the default language in that case. Not sure we should do the move of language_load() here, but I see this patch demonstrates a good reason to do move it to a common place. (Making language.module required for sites did fall short of getting any support elsewhere).

Yeah, well, Drupal 8 now provides specific hooks for when languages are removed, so entities can react to that in ways. See http://api.drupal.org/api/drupal/core--modules--node--node.module/functi... for example for the node module that nulls out node language info for removed languages, that might or might not in fact be good for fields, but its the same behavior like in D7, just we have a dedicated hook to do it. In D7 it was directly in http://api.drupal.org/api/drupal/modules--locale--locale.admin.inc/funct..., woooooaaahh, yeah.

There are no hooks to prevent languages from being removed, if we'd rather add that, and require user to clean up their stuff first, we can do that too. All-in-all I think it is best to handle the possibility we have a language that is not language_load()-able.

Needs a reroll now that classes entities landed.

:)

Tagging for the content handling leg of D8MI.

I've just submitted "Drupal 8 entity property translation BoF" for Drupalcon Denver. If you are going to be there, please comment there so we can figure out the best time to do it.: http://denver2012.drupal.org/forum/drupal-8-entity-property-translation-bof Thanks!

Can someone summarize the outstanding questions here? I have a hard time following what are the exact goals and where are we against those?! Thank you!

Also likely more accurate title and an attempt to elevate the discussion by bringing to focus :)

I tried to sum up the issues to be covered in http://groups.drupal.org/node/197848#api-dx and following.

I don't think we want to solve all that here, do we?

+++ b/core/modules/entity/entity.class.incundefined
@@ -245,6 +299,86 @@
+    if (module_exists('locale') && isset($this->langcode)) {
+      return language_load($this->langcode);
+    }

Drupal 8 has language module which handles languages now. The field language system is still in some level placed in locale module, but we are clearing that out for UI translation stuff only, so maybe add a @todo to move to language.module later.

Also, the progress on making all entities use langcode instead of language as property is going much slower then expected. #1410096: Convert comment language code schema to langcode might land soon, but we still have outstanding issues for users and (#1439680: Rename $user language property to langcode) nodes (#1439686: Rename $node language property to langcode). (We also have an issue for fields, but that does not apply to this patch: #1439692: Rename field language properties to langcode).

So if we want to have this land soon, we'd need to add a backwards compatibility layer, so it checks for ->language and ->langcode as well. Both should not be there at the same time. (Also with a @todo). Looks like this is the only place this is being checked so the damage is limited. I think that would look a bit ugly but don't have better ideas to get this moving sooner.

Otherwise looks good IMHO.

+++ b/core/modules/entity/entity.class.incundefined
@@ -245,6 +299,86 @@
+      $field = field_info_field($property_name);
+      $default_language = $this->language();
+      if ($default_language && field_is_translatable($this->entity_type, $field)) {
+        $langcode = isset($langcode) ? $langcode : $default_language->language;
+      }
+      else {
+        // Non-translatable fields always use LANGUAGE_NONE as language code.
+        $langcode = LANGUAGE_NONE;
+      }

That snippet is completely duplicated between get() and set().
Could this be factored out to a protected helper method ? like $langcode = $this->resolveLangcode($property_name, $langcode); ?

13 days to next Drupal core point release.

+++ b/core/modules/entity/entity.class.incundefined
@@ -245,6 +299,86 @@
+    if ($default_language = $this->language()) {
...
+        $langcode = isset($langcode) ? $langcode : $default_language->language;

Shouldn't this be $default_language->langcode ?

13 days to next Drupal core point release.

@fago: well $comment->language to $comment->langcode looks close to land in #1410096: Convert comment language code schema to langcode. There are definitely other entities like $user and $node where the issue is not started yet (look at the "langcode" tag). There also entities so far missing langcode (taxonomy term, file, etc). The plan was/is to convert the language properties to langcode and introduce langcode in schemas where it was missing. These are happening in parallel in other issues, just wanted to double check you agree its a good plan.

I see / agree we can overcome the missing $langcode values with a LANGUAGE_NONE default and the language properties with a method override, so we don't make the commit of this patch dependent on any of the other parallel work. (I'm holding a D8MI sprint in person tomorrow in Budapest and over IRC on #drupal-i18n and will try and get people make significant progress on the langcode conversion and introduction issues).

It looks good IMHO. Anybody else has concerns? :)

How will that default be handled? Not via a global check, I hope...

@Crell:

As I wrote in http://groups.drupal.org/node/197848#api-dx, I was playing with the idea of making the Entity object context-dependent by passing it an (optional?) context/request at instantiation. @fago's perplexity made me somehow reconsider this approach, which IMO has a good potential in terms of easing DX if properly handled, i.e. by always defaulting to the entity language except for specific cases, such as the editing workflow.

An alternative could be to make the default alterable by controllers, which would be responsible for providing the right context to the entity object.

@fago:

However simply defaulting to the entity language will not always work, unless people start passing the langcode parameter, which is not going to happen. Think about entity tokens, for instance. It's a good approach for D7, which ships with an half-baked entity language support, but for D8 it's far too limiting.

Well the entity object itself should not get a request object, because HTTP behavior should be utterly irrelevant to a data object. If it needs the language, which is usually tracked in a request object, then we need some intermediary code that will take the language information and set that on the data object using an agnostic API.

As long as the default language that an entity uses is something passed to it as a language string/object/struct, rather than something it has to retrieve from elsewhere or an irrelevant object that gets passed to it (eg, Http Request object), I'm OK with that.

Well the entity object itself should not get a request object, because HTTP behavior should be utterly irrelevant to a data object.

This is one of the reasons why I suggested to stick with the context terminology in the "Rethinking WSCCI" thread :)

Usually code should pass the target language, as the token system already supports. Still, we can think about adding helpers for appearing use-cases, as getting properties in display language.

Honestly I did not see enough code handling with ML entities yet, so I have not a solid opinion about this. I'm just afraid people will start skipping/forgetting the $langcode parameters since things work anyway without them (unless you test your code in a ML environment, obviously).

As usually the display case should be handled by an "entity display" system anyway, I'm not sure we'll need such a helper. But if the need comes up, why not.

Let's go on with this approach then, we still have time to revisit this part if need be.

@plach/@fago/@Crell: the same problem will apply to CMI and people will need to pass around language information or they are going to get their fare share of bug reports :) And we need to document this proper obviously for them. I don't think this is really hard if we give proper examples.

The comment langcode change landed last week, so the above code is not correct anymore. The rest of the langcode renames / introductions at least for entities are at #1454538: Add langcode property to all entity types; for the user entity, distinguish entity language from user's language preference which is also pretty close to RTBC. Not sure if we want to help push that in this week and then get this one in right after? (Ie. hopefully before Drupalcon :)

Removed the comment part, the others still applied. Needs review! Anyone has more concerns?

Hm, I'm a bit embarrassed for not coming up with this earlier, but his approach reintroduces a hard dependency on field.module within entity.module, which is something other patches that went in earlier in the cycle tried to eliminate.

Not sure of the right answer about this.

A possible answer is to have field.module provide a FieldableEntity class:

Class FieldableEntity extends Entity {
  function get($property_name, $langcode = NULL) {
    If $property is a field {
      Do the field stuff 
    }
    else {
      return parent::get($property_name, $langcode);
    }
  }
  
  ...
}

Then fieldable entities extend that class rather than Entity.

@yched: yes, that sound like a good compromise. I guess core entities would extend from that.

Although this could be a working solution, I think Fieldable would be better suit as an interface: should a custom entity need a different class hierarchy it would be stuck with no field (and no field language) support.

Perhaps defining Field(s) as a subclass of a Property class, as @fago is proposing, would help here?

Agreed with both #70 and #72: Provide an interface, always code to the interface, but also provide a default class implementation that the 90% use case can just use.

In which case, we have a FieldableEntityInterface, a PropertyEntityInterface, and abstract classes (or maybe concrete classes) that implement one or both of those that most people will use in practice.

(In PHP 5.4, I'd say provide an interface and a trait. Something to look forward to....)

If a module wants to change an entity type from not being fieldable to being fieldable, would be nice to be able to do that simply by changing 'fieldable' in hook_entity_info_alter(), not that *and* change the class. Even if we have a default class that implements only PropertyEntityInterface and not FieldableEntityInterface, is there any reason we would ever want to use that class? Also, seems like a default class that does implement FieldableEntityInterface should still be able to handle the case of field.module being disabled or 'fieldable' being set to FALSE.

So while I like the idea of a FieldableEntityInterface, I'm questioning whether field.module is the correct place to define it.

Perhaps defining Field(s) as a subclass of a Property class, as @fago is proposing, would help here?

If this means decoupling the entity class from the concept of fields, such that the entity class just gets something, and what it gets is managed by a Property class for properties and by a Field class for fields, then the Field class can be defined in field.module, and my concern in #74 would be addressed, I think. I don't know if the idea has other problems though.

I'm not sure the ability to make an entity that was not coded as "fieldable" into "fieldable" just by altering an info hook is something I'm really fond to serve as a base for the architecture :-/.

However, the other approach of having field values / entity properties ($entity->foo implements PropertyValueInterface / FieldValueInterface) is an idea I've been toying with myself, and could have many advantages.

$node->field_foo->view($formatter_name, $settings);
etc...

Retitling to make it clear it is not for "entities with language support". :)

#965300: Change LANGUAGE_NONE to LANGUAGE_NOT_SPECIFIED; add LANGUAGE_NOT_APPLICABLE and LANGUAGE_MULTIPLE got in and I think we need to rename some stuff in this patch. Move status back if I'm incorrect.

@fago: well, so far the translation model we have for fields/entities in D7/D8 (since we did not make any changes to that yet in D8) is that unless the entity has a concrete language code, it is not translatable. The language code on the entity is used to store the language it was originally submitted in, that is one of the langcodes that you have fields available in. That is used as the base/source language for translations for example. It is used to display the edit form, as a default fallback language for displaying the entity, etc. So if you have a node that has LANGUAGE_NOT_SPECIFIED, that cannot really be a base for translation, can it? Can an unspecified language set of data be translated to German or English? I think as long as we keep that model going, if the entity has any special language code, it should not be translatable. It should only be translatable if it has a concrete language code (and its field configs say translatable for the respective fields).

That entity_translation did not add the language properties on entities such as terms yet is just a stop gap. I consider that a misbehavior not by design. Also, the addition of the language code is exactly so we know the language. If we make assumptions about entities where the user specifically said we should not make assumptions, then we don't follow our own rules. When you start off a not multilingual site, terms, nodes, etc. are saved in the site default language (practically English) until you enable language support, so we know the language of those very well. The purpose of the special language codes is to be used for special cases not to make our approach sloppier. :)

Right, if we are not certain of the language of the entity, it should not support translations.

+++ b/core/modules/entity/entity.class.inc
@@ -74,6 +74,70 @@ interface EntityInterface {
+  public function languageCode();

Having two methods to get the language *and* the language code might be confusing DX. We might want to remove this in the future if we are able to come up with a valid alternative.

+++ b/core/modules/entity/entity.class.inc
@@ -74,6 +74,70 @@ interface EntityInterface {
+  public function translations();

The translations() name might be misleading as the current field 'translatable' property is, since we are returning the available language variants. Which might not necessarily be translations. Again since this is fine for most use cases let's go on with it and figure out an alternative in a dedicated issue.

+++ b/core/modules/entity/entity.class.inc
@@ -245,6 +316,92 @@ class Entity implements EntityInterface {
+      // Go through translatable properties and determine all languages for
+      // which translated values are available.

We should move away form this approach in the long run, since we might have field values which are assigned a language but not belong any translation (linguistic shared fields). We probably should track this information with an internal property instead of computing it. For now this is covering the 90% of use cases, so I'd say it's fine.

+++ b/core/modules/entity/entity.class.inc
@@ -245,6 +316,92 @@ class Entity implements EntityInterface {
+      // Remove the default language from the translations.
+      unset($languages[$default_language->langcode]);

I don't think we should do this, this method should return all the available languages, even if there is only one. Only LANGAUGE_NOT_APPLICABLE and friends should not be returned. However while the name stays this might make sense.

+++ b/core/modules/entity/entity.class.inc
@@ -245,6 +316,92 @@ class Entity implements EntityInterface {
+    if (field_is_translatable($this->entityType, $field) && ($default_language = $this->language())) {

This could use a comment specifying why we ignore the $langcode parameter if the entity language is not available.

+++ b/core/modules/entity/tests/entity.test
@@ -65,6 +65,135 @@ class EntityAPITestCase extends DrupalWebTestCase {
+    // Set the value in a certain language. As the entity is not translatable it
+    // should use the default language.

This comment should be a little more clear about the fact that the language specified by the setter is actually being ignored.

+++ b/core/modules/entity/tests/entity.test
@@ -65,6 +65,135 @@ class EntityAPITestCase extends DrupalWebTestCase {
+    $entity->langcode = $this->langcodes[0];

We should have a language setter here in the long run.

5 days to next Drupal core point release.

Nice, let's get this in!

I hope for followups that make the properties protected so people cant bypass set/get.

@chx

I asked @fago the same but dynamic properties cannot be assigned a visibility.

I think more documentation is needed for the confusing functions of languageCode() and language(). Currently one says "Returns the language code associated with the entity." the other says " Returns the default language of the entity." They return different values but that is not clear. Nor is it clear why they are so similar yet different. Plus if the goal is to deprecate one of them in the future, I think a comment about that should be made with an explanation. Does that mean developers should avoid using one of them? This is the kind of code that will make people tear their hair out in the future if not better documented :)

I didn't change the status because I don't want to derail progress, but I think those functions need a better explanation. I would try to provide a suggestion for improvement, but I don't understand the difference myself.

I think that is much more clear, thanks! I didn't test this but others have and the only change is the terminology, so marking it back to reviewed and tested.

I noticed a couple small oddities with the docs:

1. +++ b/core/modules/entity/entity.class.incundefined
   @@ -245,6 +319,94 @@ class Entity implements EntityInterface {
   +    // @todo: Check for language.module instead, once field API language
   +    // handling depends upon it too.
   

   SUPER nitpicky, but I think "Field API" should be capitalized.

2. +++ b/core/modules/entity/tests/entity.testundefined
   @@ -65,6 +65,135 @@ class EntityAPITestCase extends DrupalWebTestCase {
   +    // Also getting a translated value should go with the default language.
   

   I don't quite understand this comment.

3. +++ b/core/modules/entity/tests/entity.testundefined
   @@ -65,6 +65,135 @@ class EntityAPITestCase extends DrupalWebTestCase {
   +    // Try to get a not available translation.
   ...
   +    $this->assertNull($value, 'Not available translation is NULL.');
   

   "Not available translation" isn't really idiomatic. "A translation that is not available" would be better.

Not touching the status because these are so minor they could easily be moved into a followup instead, and not rerolling myself since I don't know what the comment in my second point above is actually saying. :)

I think our best practice for D8 recommends type hinting for @param and @return directives. Hence, I was surprised to see that there were none in EntityInterface. Should type hinting be added here?

@xjm: ok, so the code for the second comment you highlighted looks like it creates an entity without assigned language, but then attempts to load a field with a specific language. Since the entity has no assigned language, it should fall back in that case to the value then entity was created in. That is being tested. Good ideas to reword that?

- Updated comments in patch per xjm's revisions in c100.
- "Also getting a translated value should go with the default language." -> "Entity has no assigned language. Field should default to the language the entity was created in."

Comment only changes. Should be back to RTBC then.

+++ b/core/modules/entity/entity.class.incundefined
@@ -74,6 +74,73 @@ interface EntityInterface {
   /**
+   * Returns the language code associated with the entity.
+   *
+   * The language code of the entity either refers to a language or to one of
+   * the system language constants, e.g. LANGUAGE_NOT_SPECIFIED,
+   * LANGUAGE_NOT_APPLICABLE or LANGUAGE_MULTIPLE.
+   *
+   * @return
+   *   A valid language code.
+   */
+  public function languageCode();
+
+  /**
+   * Returns the default language of the entity.
+   *
+   * @return
+   *   The language object of the entity's default language, or FALSE if no
+   *   valid language is assigned.
+   *
+   * @see EntityInterface::translations()
+   * @see EntityInterface::languageCode()
+   */

It is not clear why we need both. I expected only one of those to exist. It took me a while to understand the difference. I think it makes the Interface more confusing and less clean.

+++ b/core/modules/entity/entity.class.incundefined
@@ -245,6 +319,94 @@ class Entity implements EntityInterface {
+  /**
+   * Determines the language code to use for a field.
+   */
+  protected function getFieldLangcode($field, $langcode = NULL) {
+    // Only apply the langcode if the entity is language specific, thus has a
+    // language assigned.
+    if (field_is_translatable($this->entityType, $field) && ($default_language = $this->language())) {
+      return isset($langcode) ? $langcode : $default_language->langcode;
+    }
+    else {
+      // Non-translatable fields always use LANGUAGE_NOT_SPECIFIED.
+      return LANGUAGE_NOT_SPECIFIED;
+    }

I'm not sure this logic makes sense. In the if-clause, we return the specified $langcode even if it may not have been set? The getFieldLanguage() API seems a bit weird. You enter a $langcode and then you get a $langcode back? I would not expect one to pass in a $langcode. It needs a bit more documentation before I can make sense of that.

@Dries, @fago: in fact in #1471432: Rework language_list(), let people use more special languages we are leaning towards making the special languages part of the regular language list, ie. let people disable them, so they cannot be assigned to nodes lets say, or reorder them, so they show up in their priority order for content assignment, etc. That would need all kinds of filtering so they don't show up in a language switcher block let's say. So there are quite a few odd ends to resolve there before we can make them generally available as such. I think moving forward with the entity/field/property API is of paramount importance that it should not be held back for unresolved questions in this related field. If we introduce language objects like that, then we can return here and drop one of the API functions to clean it up. We can also introduce langcode return only for now and return later to the language object question, if that sounds better.

We can also introduce langcode return only for now and return later to the language object question, if that sounds better.

+1

Also:

+++ b/core/modules/entity/entity.class.inc
@@ -392,12 +396,14 @@ class Entity implements EntityInterface {
+   * Determines the language code to use for accessing a field value in a certain language.

This line exceeds 80 chars. It seems the latest coding standards do not allow even PHP doc summay lines to go over column 80. What about the following?

Determines the language code to use to access a field in a given language.

@fago: yes, that is a very good goal I fully agree with BUT I don't know we want to block this patch on that. Once again the direction of #1471432: Rework language_list(), let people use more special languages is that you are going to get a language object for special languages, but it is going to have its own property by which you can tell it is a special (non-configurable) language. That property used there is currently $language->locked (in accordance with content types which have a similar pattern for non-editable code defined objects). We can certainly make up something better for that there, BUT this patch introduced definitely some important things, and we can return to the special language problem once that is there. This was held back for a looong time IMHO.

I'm fine with either so as long as we don't wait on #1471432: Rework language_list(), let people use more special languages being resolved, which can take on considerable time still.

I'm ok with going back to #106 (not sure if it'll pass the coding standards check ;), an alternative could be to have a (possibiliy temporary) method to detect the type of the system langcode assigned to the entity (if any). Something like

<?php

/**
 * Implements EntityInterface::systemLanguageCode().
 *
 * This replaces EntityInterface::languageCode().
 */
function systemLanguageCode() {
  return !$this->language() ? $this->langcode : FALSE;
}

?>

which would be used only when $entity->language() is false.

Rerolled with languageCode() method removed. I think the utility of this patch far outshines this (IMHO) minor question as to how we deal with special language codes temporarily before we add them as real looking languages stamped as special in #1471432: Rework language_list(), let people use more special languages. I think we have been attached to so many little details of this patch that if we keep doing it, it will never get in :/ Looks like @fago agrees just getting rid of languageCode() keeps us the interface we needed to be able to tell for how whether we had a special language code or not, while also making the language info clearly available. That seems to be pretty good for me.

Mistyped $this->langcode as $this->langucode.

Ok, we have arrived at the same thing. This satisfies Dries' concern in a way that fit well with other reviewers. Let's get this in. We can revisit this later after #1471432: Rework language_list(), let people use more special languages.

Committed/pushed this to 8x. One hunk didn't apply (in Entity::__construct()) but it's no longer a relevant change I think, we'll find out if HEAD breaks in the next 20 minutes..

This will need a change notification

Nope, this did break HEAD so I've rolled it back, could we get a re-roll?

Here's a stab at this. Let's wait for the bot on this one :)

Okay, last try for the night.

Looks good, thanks for the re-roll.

crosspost by fago, back to RTBC.

Alrighty, trying again!

Ok, now we need to get the backports in ;)

Added an initial change notice at http://drupal.org/node/1532946. @fago or @plach can you help with before / after code examples of how the getter/setter simplifies accessing/setting fields / properties? :) Would be good to set this fixed then (and set back the status accordingly).

Added a couple of code snippets. I think this can be called fixed, yay!

The EntityTranslationTestCase test case could almost be a unit test case (and be a lot faster to run).

Follow-up patch to decouple EntityTranslationTestCase from the parent site environment executing the test.

Please don't do such couplings.

Ick, yes. :)

+++ b/core/modules/entity/tests/modules/entity_test/entity_test.moduleundefined
@@ -20,7 +20,7 @@ function entity_test_entity_info() {
+  if (variable_get('entity_test_translation')) {

Shouldn't this be variable_get('entity_test_translation', FALSE)

Doesn't matter, NULL is as good as FALSE.

Back on sprint for easier tracking.

Committed/pushed to 8.x.

Thanks.

Re-tagging.

fixed issue reference