Not possible to use the entity getter to retrieve non-translatable field values

Here is a figure I did before to help people understand the meaning of not defined language. For this scenario, the "non-translatable field" stands, which is the rightmost one, where it says "All shared/non-translatable fields LANGUAGE_NONE only, cannot have multiple variants". Since this condition is where you end up with a non-translatable field, it should not inherit the node language because it is in fact stored with no language per definition. That is the background of this patch.

Still needs tests.

+++ b/core/modules/entity/lib/Drupal/entity/Entity.phpundefined
@@ -220,9 +220,12 @@ class Entity implements EntityInterface {
     else {
-      // If there is a langcode defined for this field, just return it. Otherwise
-      // return LANGUAGE_NOT_SPECIFIED.
-      return (isset($this->langcode) ? $this->langcode : LANGUAGE_NOT_SPECIFIED);
+      if ($langcode != NULL && $langcode != LANGUAGE_NOT_SPECIFIED) {
+        $field_name = $field['field_name'];
+        throw new EntityStorageException("Field $field_name is not translatable and does only work with langcode = LANGUAGE_NOT_SPECIFIED");
+      }
+      // For non-translatable the langcode is LANGUAGE_NOT_SPECIFIED.
+      return LANGUAGE_NOT_SPECIFIED;

Should this be thrown from set() only? Sounds like if you request some language that is not present, we should not freak out. The code above falls back on default language for example, no?

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityTranslationTest.phpundefined
@@ -86,18 +87,25 @@ class EntityTranslationTest extends WebTestBase {
+    $message = "An exception is thrown when trying to set an field with an invalid language";

an field => a field

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityTranslationTest.phpundefined
@@ -86,18 +87,25 @@ class EntityTranslationTest extends WebTestBase {
+    } catch (EntityStorageException $e) {
+      $this->assertTrue($e instanceof EntityStorageException, $message);
+    }
...
+    } catch (EntityStorageException $e) {
+      $this->assertTrue($e instanceof EntityStorageException, $message);
+    }

} and catch should be separated by newline :)

Looks good, thanks.

Sorry to set this back, but

+++ b/core/modules/entity/lib/Drupal/entity/Entity.phpundefined
@@ -220,9 +223,18 @@ class Entity implements EntityInterface {
+          throw new EntityStorageException("Field $field_name is not translatable and does only work with langcode = LANGUAGE_NOT_SPECIFIED");

This should use format_string().

Tagging.

Added format_string().

Looks good, thanks.

This deals with the rapidly changing entity landscape and is at a definite risk of commit conflicts.

#13: 1738368-13.patch queued for re-testing.

Here's my review; mostly docs stuff. In general, my concern is that there's a bunch of crazy-assed stuff in #4 that's not communicated at all here. I don't think we need to move all of that into this code, but we certainly should move enough of it in so someone coming into these tests/functions without the benefit of having seen this issue will have an idea what's going on.

+++ b/core/modules/entity/lib/Drupal/entity/Entity.php
@@ -185,7 +185,9 @@ class Entity implements EntityInterface {
+      // We call getFieldLangcode with strict = FALSE, so if we request a field
+      // with a invalid language we don't throw an exception.
+      $langcode = $this->getFieldLangcode($field, $langcode, FALSE);

Ok, I can mostly make that out from reading that line of code. Why do we do that?

+++ b/core/modules/entity/lib/Drupal/entity/Entity.php
@@ -216,7 +219,7 @@ class Entity implements EntityInterface {
   /**
    * Determines the language code to use for accessing a field value in a certain language.
    */
-  protected function getFieldLangcode($field, $langcode = NULL) {
+  protected function getFieldLangcode($field, $langcode = NULL, $strict = TRUE) {

I guess this wasn't introduced in this patch, but we need PHPDoc of this new parameter. I have no idea why I might or might not want to set that to FALSE. Might as well document all of the parameters while we're here, and shorten that description to 80 chars.

+++ b/core/modules/entity/lib/Drupal/entity/Entity.php
@@ -225,9 +228,18 @@ class Entity implements EntityInterface {
+          $field_name = $field['field_name'];
+          throw new EntityStorageException(format_string("Field @field_name is not translatable and does only work with langcode = LANGUAGE_NOT_SPECIFIED", array('@field_name' => $field_name)));

"and only works with" rather than "does only work with"

Minor, but is there a reason to pull $field['field_name'] into a variable if you're only using it once?

Attempted addressing webchick's comments from #17. Docs/comments need review by someone who really understands this to see if I got them right.

oops. a little trailing whitespace problem. Ignore the patch in #18 and look at this one.

Subtagging for D8MI.

This looks good to me, a few minor remarks:

+++ b/core/modules/entity/lib/Drupal/entity/Entity.phpundefined
@@ -213,9 +217,30 @@ class Entity implements EntityInterface {
+   * Determine the language code for accessing a field value.

"Determine the language code" should be "Determines the language code". See http://drupal.org/node/1354#functions for more information.

+++ b/core/modules/entity/lib/Drupal/entity/Entity.phpundefined
@@ -213,9 +217,30 @@ class Entity implements EntityInterface {
+   * The language code to use for accessing a field value in a certain language
+   * is determined to be the langcode if the entity is language-specific. This
+   * function handles the special cases of not translateable fields and
+   * languages that are not specified.

I was thinking of something like this:
If the entity is language-specific and the field is translatable, the langcode is used to access the field value. Otherwise LANGUAGE_NOT_SPECIFIED should be used to access the field value.

When the field is not translatable and the langcode attempted is not LANGUAGE_NOT_SPECIFIED an exception is thrown in strict mode, or NULL is returned in non-strict mode.

+++ b/core/modules/entity/lib/Drupal/entity/Entity.phpundefined
@@ -213,9 +217,30 @@ class Entity implements EntityInterface {
+   * @param $langcode
+   *   The language code attempting to be applied to the field.

This param is optional and should be documented as such:
(optional) The language code attempting to be applied to the field.

+++ b/core/modules/entity/lib/Drupal/entity/Entity.phpundefined
@@ -213,9 +217,30 @@ class Entity implements EntityInterface {
+   * @param $strict
+   *   Optional flag so that we can be differently strict for invalid langcodes

This param is optional so the description should start with (optional).

+++ b/core/modules/entity/lib/Drupal/entity/Entity.phpundefined
@@ -213,9 +217,30 @@ class Entity implements EntityInterface {
+   *   Optional flag so that we can be differently strict for invalid langcodes

I was thinking of:
(optional) TRUE if an exception should be thrown when an invalid langcode is used on a non-translatable field, FALSE if NULL should be returned. Depending on the context in which this function is used, we want to be more strict. For example, the EntityInterface set() uses $strict TRUE, and the EntityInterface get() uses $strict FALSE.

+++ b/core/modules/entity/lib/Drupal/entity/Entity.phpundefined
@@ -213,9 +217,30 @@ class Entity implements EntityInterface {
+   * @return
+   *   The langcode if appropriate, LANGUAGE_NOT_SPECIFIED as in the case of a
+   *   field shared among all language version of an entity, or NULL.

The langcode if appropriate, LANGUAGE_NOT_SPECIFIED for non-translatable fields, or NULL when an invalid langcode was used in non-strict mode.

It might be a good idea to document the data types on the @param and @return directives. For example:
@return string|null

This patch includes most of the suggestions from #21. I moved ... as in the case of a field shared among all language version of an entity... to the general description comment. Keeping that because I think it is part of what webchick asked for when she said to try and include some of the information from the graphic in #4. I looked at being more specific with specifying the types (http://drupal.org/node/1354#functions) but I wasn't clear on if most of the @param were just "strings"... and when I looked elsewhere in core for examples, like: core/modules/field/field.info.inc, I didn't find types specified for some of the non trivial params.

I agree we don't need to fix all of the problems of existing docs in a bugfix patch :) I think the documentation updates look good, so should be back at RTBC.

Explained this in IRC in more detail to webchick, sun and YesCT:

[10:30am] GaborHojtsy: webchick: sun: ok, start with the summary, the problem is that the getter uses the langcode of the entity first and foremost, so it is not possible to use the getter to retrieve non-translatable fields right now
[10:30am] GaborHojtsy: $this is the entity, not the field in the code
[10:30am] GaborHojtsy: so the comment and code does not match
[10:30am] GaborHojtsy: but the same code is used in both a getter and setter context
[10:32am] GaborHojtsy: webchick: sun: and fago wanted to keep the fallback method of the getter() so you can get('body') without knowing if you need to or not need to specify a langcode, or you can iterate through all getters without calling some with the entity langcode and others with NULL or LANGUAGE_NOT_SPECIFIED to get all the fields
[10:32am] GaborHojtsy: webchick: sun: so if we'd throw an exception from get() if you want to get a value in (an invalid) langcode for a non-translatable field, you'd *always* need to wrap your getter with logic to check field translatability FIRST, outside of the getter (although the getter already does this inside)
[10:33am] webchick: Hm ok. so it's primarily for DX, or?
[10:33am] drupol joined the chat room.
[10:33am] GaborHojtsy: webchick: sun: or architect your code around catching exceptions and calling get() back again with NULL or LANGUAGE_NOT_SPECIFIED
[10:33am] GaborHojtsy: webchick: right
[10:34am] GaborHojtsy: webchick: so if body is untranslatable, with get() you get back the same data regardless of which langcode you pass, but for set() you get an exceptions saying "no-no, that is not possible"
[10:34am] GaborHojtsy: webchick: if we *prefer* to throw an exception regardless, that is possible to do
[10:34am] sun: hrm.  the fact that it required 5 messages to explain the situation essentially means that we need to put that explanation into phpDoc 
[10:35am] webchick: No, I don't mean that necessarily. Just from reading the patch it's not clear a) *why* that was done (the "what" is there), and b) when I as a module developer know that I need to call with/witout that param
[10:35am] webchick: and yes, what sun said. 

It also does not apply due to the entity system being moved around.

Same patch rerolled.

Retitled for the problem and elevating bug since this is a pretty big deficiency, it would mean the entity getter does not work on single language sites for untranslatable fields, which is a pretty big portion of Drupal's user base.

Tried my best to improve the docs, but I'm not sure whether everything is correct.

Great improvements!

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -213,9 +216,42 @@ public function set($property_name, $value, $langcode = NULL) {
+   * - If a field's values are shared among all language versions of an entity,
+   *   LANGUAGE_NOT_SPECIFIED should be used to access them.

"field's values are shared among all language versions" is the same as "non-translatable field". We might want to use this wording even though its used again in the next paragraph just so that its absolutely clear.

I think its a great improvement either way, and you might just wanted to avoid repetition, so its fine as-is too.

Updated patch to fix the exception name in the test as well and correct the phpdoc description as explained above.

The fail is due to #1779638: Unexplained test failure in LocaleFileImportStatus->testBulkImportUpdateExisting(), testing again.

#31: drupal8.entity-getfieldlangcode.31.patch queued for re-testing.

#31: drupal8.entity-getfieldlangcode.31.patch queued for re-testing.

Finally no unrelated fails, should be back to RTBC.

@Gabor:

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -213,9 +216,43 @@ class Entity implements EntityInterface {
+   * - If the entity is language-specific and the requested field is
+   *   translatable, the entity's language code should be used to access the
+   *   field value.

I think we are missing "when no language is explicitly provided."

That was easy :)

Thanks :)

Small nit to be incorporated if this patch is re-rolled.

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -213,9 +216,43 @@ class Entity implements EntityInterface {
+   *   Field the language code is being determined for.
+   * @param string|null $langcode
+   *   (optional) The language code attempting to be applied to the field.
+   * @param bool $strict
+   *   (optional) When $strict is TRUE, an exception is thrown if the field is
+   *   not translatable and the langcode is not LANGUAGE_NOT_SPECIFIED. When
+   *   $strict is FALSE, NULL is returned and no exception is thrown. For
+   *   example, EntityInterface::set() passes TRUE, since it must not set field
+   *   values for invalid langcodes. EntityInterface::get() passes FALSE to
+   *   determine whether any field values exist for a specific langcode.

Both of these @param directives for optional variables need to document what their default values are.

@Lars: Thanks, can you help with that?

At the moment, I cannot roll any patches @Gabor.

Let's get this in then and people can tweak this however they want in the future.

I'll do

Thanks @platch. Those additions look good.

Awesome; great work on the docs in this patch, folks. That's much clearer now.

Committed and pushed to 8.x.

Not on sprint anymore. Thanks all!

Removing Avoid commit conflicts tag

try #2