Updated: Comment #58

Problem/Motivation

The problem is caused when the language defaults for any content type is missing. Any entity that is created programmatically will not set the default language.

Steps to replicate

  1. Create a new content type.
  2. Specify the machine name for content type.
  3. Choose default language as Not Specified from language settings tabs.
  4. Programatically try to create the entity, the default language will be missing

Proposed resolution

  1. The correct place to set the default language for EntityNG would be LanguageItem::applyDefault().
  2. The easiest way would be a if language module exists check there
  3. if module exists then set language code from the entity using language_get_default_langcode else it will set the language code to LANGCODE_NOT_SPECIFIED.
  4. There is a possibilty of function setValue is getting moved into the language manager service defined under core/lib/Drupal/Core/Entity/Plugin/DataType/LanguageItem.php

Remaining tasks

  1. The patch is ready for review.
  2. also #122 needs addresses.

http://drupal.org/node/1869562#comment-7280616.

Original report by @fago

I just ran into a problem caused by language defaults missing, see http://drupal.org/node/1869562#comment-7280616.

I think that language module should use hook_entity_create() to set the default language, that way any programmatically created entity will get the usual defaults as well.

Files: 
CommentFileSizeAuthor
#158 1966436-diff-157-158.txt1.61 KBvijaycs85
#158 1966436-default-content-entity-language-158.patch16.83 KBvijaycs85
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,630 pass(es).
[ View ]
#156 1966436-156.patch16.77 KBmr.york
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,012 pass(es).
[ View ]

Comments

Well, recently in #1947814: New config entities often save as langcode: und incorrectly we introduced a different method to set default config entity language, but it can depend on the form as well if the entity had a UI. So not sure what kind of info do we have in the create hook? HOw do you imagine this would work?

The create hook sets the programmatic defaults. I guess it should resemble the default values of the form, as people would expect a programmatically created entity to match them?

http://api.drupal.org/api/drupal/core!includes!entity.api.php/function/h...

The form defaults depend on language settings though (eg. per vocabulary, per content type, etc.).

Field API defaults depend on configuration also, that's ok imho. We could just use language_get_default_langcode() as I had to do it over at http://drupal.org/node/1869562#comment-7280616?

I agree we should do this: language_get_default_langcode() is all we need to implement hook_entity_create() properly. Depending on any form workflow would be just wrong IMHO.

I believe that is only implemented for content entities so far?

Well, I think we should put our efforts in making config entities behave as content entities on this specific side. Since we have an API that's perfectly suited to set language default we should totaly leverage it :)
This would have the additional advantage of allowing the entity form controller to automatically provide the language selector instead of forcing us to manually add it for each entity type. AAMOF it would just need to rely on the language_select form element and entity language default, no dependency on the Language module of any kind. I can see a lot of potential in this solution and I've been mulling on this for a while now.

Yeah, so as I attempted to point this out, this is a bit larger a scope then just using a hook :D

Status:Active» Needs review
StatusFileSize
new1.08 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8_langauge_default.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here is a patch that would implement it based upon #1777956: Provide a way to define default values for entity fields - that way the default would be added in by the language field.

Status:Needs review» Needs work

The last submitted patch, d8_langauge_default.patch, failed testing.

+++ b/core/lib/Drupal/Core/Entity/Field/Type/LanguageField.phpundefined
@@ -20,11 +20,19 @@ class LanguageField extends Field {
   public function applyDefaultValue() {
...
+      if (module_exists('language') && $entity = $this->parent) {
+        $langcode = language_get_default_langcode($entity->entityType(), $entity->bundle());

there's no applyDefaultValue() method anymore

Assigned:Unassigned» plach

I hope to be able to provide a new patch for this soon.

Issue tags:+sprint

Assigned:plach» kfritsche

Assigned:kfritsche» Unassigned
Status:Needs work» Needs review
StatusFileSize
new845 bytes
FAILED: [[SimpleTest]]: [MySQL] 58,029 pass(es), 67 fail(s), and 0 exception(s).
[ View ]

I currently having some problems with this.
I started implementing hook_entity_create() in the language module.
It uses language_get_default_langcode() and sets this as the language of this new entity.

Problem is that multiple tests are failing and I'm not sure if this should be changed or not.
Most importantly "core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationTest.php":
It directly checks that the language is not defined at, but we are now directly setting it. I don't know why it was intended this way in the test, but won't change it without further feedback.

Also as we set a language at the beginning following test are failing too, as the entity is already a language-specific one. If the test are right in the current way, we would have to replace more or less Language::LANGCODE_DEFAULT with language_get_default_langcode() in EntityNG.
Or I got something completely wrong here... (Still attaching the failing patch with the hook.)

I think those tests do not take into account this use case. It would be good to update them to match the behavior implement here. That seems the correct place.

+++ b/core/modules/language/language.module
@@ -433,6 +434,14 @@ function language_node_type_update(NodeTypeInterface $type) {
+  $entity->set('langcode', $langcode);

There are thounsand of ways to do this now, but I think the preferred one currently is
$entity->langcode->value = $langcode.

Status:Needs review» Needs work

The last submitted patch, drupal-1966436-default-language-16.patch, failed testing.

Assigned:Unassigned» itarato

Assigned:itarato» Unassigned

@itarato:

Are you still working on this?

Status:Needs work» Needs review
StatusFileSize
new535 bytes
new846 bytes
FAILED: [[SimpleTest]]: [MySQL] 55,025 pass(es), 46 fail(s), and 182,819 exception(s).
[ View ]

update, as per #17

Status:Needs review» Needs work

The last submitted patch, 1966436-default-language-22.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1006 bytes
FAILED: [[SimpleTest]]: [MySQL] 57,560 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
new822 bytes

We shouldn't try to override the language of the entity if this was already set.

Status:Needs review» Needs work

The last submitted patch, 1966436-default-language-24.patch, failed testing.

Spent a lot of time with this exceptions, the problem seems to be that contact_message has no langcode property when we try to assign the default langcode.

Title:Language module should use hook_entity_create() to set default languageDefault entity languages are not set for entities created with the API
Priority:Normal» Major

Status:Needs work» Needs review
Issue tags:-D8MI, -sprint, -language-config, -language-content

#24: 1966436-default-language-24.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+D8MI, +sprint, +language-config, +language-content

The last submitted patch, 1966436-default-language-24.patch, failed testing.

Config entities probably affected too, for example vocabularies

related: we have not enough tests for upgrade path for language defaults - #2049465: Upgrade of image styles and effects broken

According to last test log. Drupal\contact\Tests\ContactUpgradePathTest fails.

It seems resolved here. It is need to be applied to pass next test.
https://drupal.org/node/2032033#comment-7708661

StatusFileSize
new721 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,472 pass(es).
[ View ]

I duplicate the patch file.

Status:Needs work» Needs review

StatusFileSize
new1.69 KB
FAILED: [[SimpleTest]]: [MySQL] 57,902 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Status:Needs review» Needs work

The default language should be set via the respective language field class, or an overridden variant.

hm, one moment: Should the default language apply to all language fields, e.g. user prefferred language etc? If so, it shuold be the field's default value, else probably not.

Status:Needs work» Needs review
StatusFileSize
new451 bytes
new1.68 KB
PASSED: [[SimpleTest]]: [MySQL] 58,028 pass(es).
[ View ]

langcode is a code(scting). In the previous versions people used it as the object language. So I need review.

Status:Needs review» Needs work
Issue tags:+Needs tests

How is the upgrade hunk related to this patch? Otherwise would be great to see test coverage for this, eg. creating two entities with the API and the UI respectively and check langcodes. There may be existing tests we can easily extend.

Assigned:Unassigned» Aron Novak

trying to adjust it now

Assigned:Aron Novak» Unassigned
StatusFileSize
new2.35 KB
FAILED: [[SimpleTest]]: [MySQL] 55,948 pass(es), 660 fail(s), and 136,037 exception(s).
[ View ]

What I see so far:
We must not set the not specified constant in the Entity class, otherwise the language module will never face with the situation of not set / empty langcode.

"langcode is a code(scting). In the previous versions people used it as the object language"

Even after the patch I attached, the condition never applies cause:

dpm(Drupal::entityManager()->getFieldDefinitions('node'));

This says that the langcode is a language_field, so it means \Drupal\Core\Entity\Plugin\DataType\LanguageItem .
Then, before create hook is called, LanguageItem::applyDefaultValue kicks in.

For the reference, I used this piece for testing:

$e = entity_create('node', array(
  'type' => 'article',
  'title' => 'moo',
));
$e->save();

And it was executed via drush.

Hopefully i can continue this stuff later next week.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1966436-default-language-40.patch, failed testing.

@andypost I see it is related with entities. Could give an advice if a test exists that provides a test to check the default language possibility? #1966436-38: Default *content* entity languages are not set for entities created with the API

The correct place to set the default language for EntityNG would be LanguageItem::applyDefault(). The easiest way would be a if module exists check there, but language.module could also alter the typed data language_field and replace the used class.

Entity::save() is not the correct place to check this, that would need to go into the save methods of the storage controllers. $entity->save() is just an optional shortcut, it's not guaranteed to be called.

According to @Berdir, the path from work from is #10. It still can be applied with Git, but it generates errors. Also:

+++ b/core/lib/Drupal/Core/Entity/Field/Type/LanguageField.php
@@ -20,11 +20,19 @@ class LanguageField extends Field {
     if (isset($this->definition['settings']['default_value'])) {
       $value = $this->definition['settings']['default_value'];
     }
...
+        $langcode = language_get_default_langcode($entity->entityType(), $entity->bundle());

I think those lines are covered by language_get_default_langcode

StatusFileSize
new900 bytes
FAILED: [[SimpleTest]]: [MySQL] 58,438 pass(es), 30 fail(s), and 1 exception(s).
[ View ]
new1.35 KB

Rerolled from #10 and adapted.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1966436-default-language-47.patch, failed testing.

StatusFileSize
new3.49 KB
FAILED: [[SimpleTest]]: [MySQL] 58,968 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new2.61 KB

Seems fine to me.

I adjusted the tests because these are now not accurate anymore.

Added simple test which testing if it is the default language, if you create a entity without giving any language.
Added to all the other tests the LANGCODE_NOT_SPECIFIED as language so we don't have to touch the test results.

This should go green now.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1966436-default-language-50.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.31 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1966436-default-language-53.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new835 bytes

Test fails in core/modules/content_translation/lib/Drupal/content_translation/Plugin/views/field/TranslationLink.php line 63, because in that test, user 2 should have no language and the link should not be visible. But after the patch, probably user 2 has language, so maybe that's why link is visible.

I send a patch that take this in account, forcing user 2 to have Language::LANGCODE_NOT_SPECIFIED

Status:Needs review» Needs work

The last submitted patch, 1966436-default-language-53.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.57 KB
PASSED: [[SimpleTest]]: [MySQL] 58,503 pass(es).
[ View ]
new723 bytes

Ok, forgot to add "use Drupal\Core\Language\Language;" ...

Assigned:Unassigned» grisendo
Status:Needs review» Needs work

So, I'll start to work on tests.

Issue tags:+Entity Field API
  1. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/LanguageItem.php
    @@ -89,7 +89,13 @@ public function setValue($values, $notify = TRUE) {
    +    if (module_exists('language') && $entity = $this->getParent()->getEntity()) {

    You can call $this->getEntity() directly.

  2. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/LanguageItem.php
    @@ -89,7 +89,13 @@ public function setValue($values, $notify = TRUE) {
    +      $langcode = language_get_default_langcode($entity->entityType(), $entity->bundle());

    I think this function is getting moved into the language manager service, so it's OK to add the check here.

Title:Default entity languages are not set for entities created with the APIUpdated issue summary from the comment logs

Problem/Motivation

The problem is caused when the language defaults for any content type is missing. Any entity that is created programmatically will not set the default language.

Steps to replicate

  1. Create a new content type.
  2. Specify the machine name for content type.
  3. Choose default language as Not Specified from language settings tabs.
  4. Programatically try to create the entity, the default language will be missing

Proposed resolution

  1. The correct place to set the default language for EntityNG would be LanguageItem::applyDefault().
  2. The easiest way would be a if language module exists check there
  3. if module exists then set language code from the entity using language_get_default_langcode else it will set the language code to LANGCODE_NOT_SPECIFIED.
  4. There is a possibilty of function setValue is getting moved into the language manager service defined under core/lib/Drupal/Core/Entity/Plugin/DataType/LanguageItem.php

Remaining tasks

  1. The patch is ready for review.
  2. Tests needs to written for the patch 1966436-default-language-55.patch, submitted at comment #55

http://drupal.org/node/1869562#comment-7280616.

Title:Updated issue summary from the comment logsDefault entity languages are not set for entities created with the API

Set the title back. Note that the "Issue title" edits the main issue title, it is *not* the title of the comment.

Still working on tests right now

@grisendo:

Are you still on it?

Yes. I'll upload patch later, as soon as tests don't break.

Status:Needs work» Needs review
StatusFileSize
new9.3 KB
FAILED: [[SimpleTest]]: [MySQL] 59,031 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new4.63 KB
FAILED: [[SimpleTest]]: [MySQL] 58,854 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new6.22 KB

Things added:

Second consideration from @Berdir I don't understand what does it mean, can you explain it more in detail?
For now, let's see if we have a green :)

Status:Needs review» Needs work

The last submitted patch, 1966436-default-language-63.patch, failed testing.

We usually try to avoid dependencies to module code in Drupal\Core code, but this method is supposed to move to the Language Manager service, so that can be ignored for now. Just wanted to comment on it, no changes necessary.

Status:Needs work» Needs review
StatusFileSize
new10.01 KB
FAILED: [[SimpleTest]]: [MySQL] 59,291 pass(es), 2 fail(s), and 66 exception(s).
[ View ]
new4.63 KB
FAILED: [[SimpleTest]]: [MySQL] 59,187 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new3.77 KB

There was a problem saving default language code on first time the content type is created. Next times it is edited, it was saved correctly.

Changes:

  • Changed module_exists('language') to \Drupal::moduleHandler()->moduleExists('language').
  • Added hook_node_type_insert in language module to get the language options saved since first time the content type is created.
  • Changed test to use spanish by default in one of the demo content types, since english is the default language and may be (and there were) bad positives there.

The code looks good to me, if the tests pass, I think it can be set soon to rtbc (after some reviews from other people probably).

Status:Needs review» Needs work

The last submitted patch, 1966436-default-language-66.patch, failed testing.

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new1.06 KB
new1.06 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1966436-default-language-66_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new4.64 KB
FAILED: [[SimpleTest]]: [MySQL] 59,079 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Fixing tests to work with current code fixes. Basically, we get site default language for an entity, if a bundle created with language code 'und'.

Status:Needs review» Needs work

The last submitted patch, 1966436-default-language-66.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.31 KB
PASSED: [[SimpleTest]]: [MySQL] 59,095 pass(es).
[ View ]

Wrong patch file (test only and diff file are fine).

Assigned:grisendo» Unassigned

Status:Needs review» Needs work

Just did a visual review and found some minor things:

  1. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/LanguageItem.php
    @@ -89,7 +89,13 @@ public function setValue($values, $notify = TRUE) {
    +    if (module_exists('language') && $entity = $this->getEntity()) {

    We should use \Drupal::moduleHandler()->moduleExists('language') instead module_exists()

  2. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/LanguageItem.php
    @@ -89,7 +89,13 @@ public function setValue($values, $notify = TRUE) {
    +    else {
    +      $langcode = Language::LANGCODE_NOT_SPECIFIED;
    +    }

    Can't we use $langcode = Language::LANGCODE_NOT_SPECIFIED; as default and get rid of the else? (Very personal preference)

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationDefaultLanguageTest.php
    @@ -0,0 +1,144 @@
    +  function setUp() {
    ...
    +  function testEntityTranslationDefaultLanguageViaCode() {
    ...
    +  function createContentType($name, $langcode) {
    ...
    +  function createNodeViaCode($type, $langcode = NULL) {

    Method scope modifiers not specified (public).

  4. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationDefaultLanguageTest.php
    @@ -0,0 +1,144 @@
    +    // Activate Spanish language, so there are two languages activated.
    +    $this->activateSpanishLanguage();
    ...
    +  /**
    +   * Activates Spanish language.
    +   */
    +  function activateSpanishLanguage() {
    +    $edit = array(
    +      'predefined_langcode' => 'es',
    +    );
    +    $this->drupalPostForm('admin/config/regional/language/add', $edit, t('Add language'));
    +  }

    Do we really need a helper for that? It's called once and the code piece is rather small.

  5. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationDefaultLanguageTest.php
    @@ -0,0 +1,144 @@
    +  /*
    ...
    +  /*

    Missing asterisk in the doc block opening.

  6. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationDefaultLanguageTest.php
    @@ -0,0 +1,144 @@
    +    // With language module activated, language of content type is site default
    +    // (en this case) language.

    How about

    // With language module activated, the language of this content type matches the
        // site default language (en this case).
  7. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationDefaultLanguageTest.php
    @@ -0,0 +1,144 @@
    +    // With language module activated, content type "en" by default.

    How about // With language module activated, this content type has the language "en" by default.

  8. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationDefaultLanguageTest.php
    @@ -0,0 +1,144 @@
    +    // With language module disabled, content type "und" by default.

    How about // With language module disabled, this content type has the language "und" by default.

  9. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationDefaultLanguageTest.php
    @@ -0,0 +1,144 @@
    +    // With language module disabled, content type "en" by default.

    How about // With language module disabled, this content type has the language "en" by default.

  10. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationDefaultLanguageTest.php
    @@ -0,0 +1,144 @@
    +   * Creates a new node content type.
    +   *
    +   * @param $type
    +   *   The node content type.

    Parameter documentation doesn't match the method signature.

  11. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationDefaultLanguageTest.php
    @@ -0,0 +1,144 @@
    +   *   If blank, it will be determined by existing content type configuration.

    I would replace "existing" with "the": If blank, it will be determined by the content type configuration.

  12. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationDefaultLanguageTest.php
    @@ -0,0 +1,144 @@
    +    $title = $this->randomName();
    ...
    +      'title' => $title,

    Do we need a variable here? It's not re-used anywhere.

Assigned:Unassigned» grisendo

I'll check. The thing about module_exists you comment, it was already in #66
Maybe lost in #71?

Status:Needs work» Needs review
StatusFileSize
new11.24 KB
PASSED: [[SimpleTest]]: [MySQL] 59,273 pass(es).
[ View ]
new5.88 KB
FAILED: [[SimpleTest]]: [MySQL] 59,172 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new7.6 KB

I moved back to #66 since entity content type default language is not saved correctly in #71, it get's to site_default (en) even when it's forced to "und" or some other language.

Fixed suggestions from @das-peter. Suggestions 6, 7, 8 and 9 are also fixed, but the comment is not well, that's not what I wanted to explain there, but I agree is quite confusing. I changed them and provided a more detailed explanation in the comments.

[edit]
Also checked if (!empty($type->language_configuration)) { when saving content type language configuration. Was so useless to check if language module exists in a language module hook... and also was breaking tests on #66 because $type->language_configuration was undefined.

Awesome, looks pretty nice.
However, there's new code in the patch that wasn't there in #71 and isn't in the interdiff - because the interdiff is 66-75 and not 71-75. That's quite confusing!

+++ b/core/modules/language/language.module
@@ -396,6 +396,15 @@ function language_get_default_configuration_settings_key($entity_type, $bundle)
/**
+ * Implements hook_node_type_insert().
+ */
+function language_node_type_insert(NodeTypeInterface $type) {
+  if (!empty($type->language_configuration)) {
+    language_save_default_configuration('node', $type->id(), $type->language_configuration);
+  }
+}

This doesn't look right. This should be properly handled in language_configuration_element_submit(), right? I think that if this doesn't work, we should try and fix it in there and not create a dependency on node. However, the language module already implements language_node_type_update(), so it seems like there's already a dependency :|
Does anybody know why?
Leaving "needs review" as I don't have enough information to set it to RTBC / needs work.

Confusion comes to me also because #69 and #71 are supposed to come from #66, but they don't. They remove that lines, but is not reflected on interdiff.
And you are right, that's why I put hook_node_type_insert in that file, because hook_node_type_update is just below.

Status:Needs review» Needs work

I looked through the history of how this patch evolved and it also looks to me that the language_node_type_insert() does not belong in this issue. In #66 a test was added, using content types as an example entity, and to make that test pass the hook_node_type_insert() was implemented.

However, the goal of this issue is to make this work out of the box across all entities, without having to implement any entity specific hooks.

Assigned:grisendo» mmilano

@grisendo, I'm at the badcamp sprint. I'll be digging into this one.

Assigned:mmilano» Unassigned
Issue tags:-sprint, -language-config, -language-content, -Entity Field API

Tag monster.

Issue summary:View changes

Update the issue summary as per the issue log

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new3.45 KB
new13.05 KB
FAILED: [[SimpleTest]]: [MySQL] 60,325 pass(es), 19 fail(s), and 2 exception(s).
[ View ]

I removed now both the hook_node_type_insert and hook_node_type_update. This is now handled in the elements submit function. Tested it manually for custom block types and node types.
The problem was that on creation with the form there was no bundle name set yet, so this was always empty.

Hope we can get further here now.

Status:Needs review» Needs work

The last submitted patch, 82: 1966436-default-language-82.patch, failed testing.

Okay, I'm lost here now. In Menu and Vocabulary the controller ($form_state['controller']->getEntity()) still has the old not updated entity. In CustomBlockTypes and NodeTypes it already has the new entity here.

When I'm thinking now about it, the Menu and Vocabulary behavior makes from the old D7 logic more sense, as while the submit is happening nothing got saved yet. But makes this somehow much more complicated to achieve there. But nevertheless I'm not sure if this is still regarding to this issue or if we getting here to some other problem. Maybe we should go with #75 and continue in another issue? So at least the default language is set for entities created by the API, what is the main reason for THIS issue?

Well, #75 has language_node_type_insert() for example, which did not look right. So not sure how can we get back there in a useful way?!

Status:Needs work» Needs review
StatusFileSize
new11.25 KB
PASSED: [[SimpleTest]]: [MySQL] 59,120 pass(es).
[ View ]
new1.81 KB

Discussed this with Gabor and decided that we rescope this Issue to be for Content Entities only. Because there is actually a question how we should handle Config Entites and their Default Language.

Because of this, I reintroduced language_node_type_update again (will maybe be removed later) and also removed the additional code in language_configuration_element_submit which probably is not really necessary?

Should be green now, as it already worked for Content Entities before.

The last submitted patch, 86: 1966436-default-language-85.patch, failed testing.

Title:Default entity languages are not set for entities created with the APIDefault *content* entity languages are not set for entities created with the API

Qualify the scope of this issue is about content entities.

Status:Needs review» Needs work

hm, I'm not sure about checking for a module in the core component. So I'd rather go for the hook implementation or even swapping out the class. (discussing with yched)

  1. +++ b/core/modules/views/lib/Drupal/views/Tests/Entity/EntityTranslationDefaultLanguageTest.php
    @@ -0,0 +1,155 @@
    +      'name' => 'Entity Translation default language',

    I'd suggest just "Entity default language" - it's not about the default language from translations, right? Same for the class name.

  2. +++ b/core/modules/views/lib/Drupal/views/Tests/Entity/EntityTranslationDefaultLanguageTest.php
    @@ -0,0 +1,155 @@
    +    // With language module activated, and content type that is configured to
    +    // have language "und" by default, a new node of this type will have "und"

    instead of "have language "und" by default - couldn't we just say it has no language specified? I mean that is what und is about.

    Then, it should be "and a content type"

  3. +++ b/core/modules/views/lib/Drupal/views/Tests/Entity/EntityTranslationDefaultLanguageTest.php
    @@ -0,0 +1,155 @@
    +    // With language module activated, and content type that is configured to

    "and a content type"

  4. +++ b/core/modules/views/lib/Drupal/views/Tests/Entity/EntityTranslationDefaultLanguageTest.php
    @@ -0,0 +1,155 @@
    +    // With language module activated, and content type that is configured to

    and a ...

  5. +++ b/core/modules/views/lib/Drupal/views/Tests/Entity/EntityTranslationDefaultLanguageTest.php
    @@ -0,0 +1,155 @@
    +    $this->container->get('module_handler')->uninstall(array('language'), FALSE);

    Could we move the module handler to $this->moduleHandler ? Usage of the container smells.

  6. +++ b/core/modules/views/lib/Drupal/views/Tests/Entity/EntityTranslationDefaultLanguageTest.php
    @@ -0,0 +1,155 @@
    +    $this->drupalPostForm('admin/structure/types/add', $edit, t('Save content type'));

    Is there a reason we do this via the web test? I mean couldn't that be a DrupalUnitTestCase as a whole as it is about testing the API, not the interface?

  7. +++ b/core/modules/views/lib/Drupal/views/Tests/Entity/EntityTranslationDefaultLanguageTest.php
    @@ -0,0 +1,155 @@
    +   *   (optional) Language code of the node to create.
    +   *   If blank, it will be determined by the content type configuration.

    Imho this should say it's the langcode to pass to entity_create(), while by default it does not pass a langcode.

  8. +++ b/core/modules/views/lib/Drupal/views/Tests/Entity/EntityTranslationDefaultLanguageTest.php
    @@ -0,0 +1,155 @@
    +   * @return \Drupal\node\Entity

    Should type hint to node interface.

  9. +++ b/core/modules/views/lib/Drupal/views/Tests/Entity/EntityTranslationDefaultLanguageTest.php
    @@ -0,0 +1,155 @@
    +  protected function createNodeViaCode($type, $langcode = NULL) {

    I'd suggest to just call it createNode() - every function is in code, so that "viaCode" confused me a bit initially. The comment should be enough to clarify it's using the API not the interface.

  10. +++ b/core/modules/views/lib/Drupal/views/Tests/Entity/EntityTranslationDefaultLanguageTest.php
    @@ -0,0 +1,155 @@
    +    $data = array(

    $data usually is $values.

Agreed with @fago that hardcoding some "if (module_exists('language') { call a function in language.module }" logic in a Drupal/Core library is not ideal.

hook_entity_create() seems perfectly suited for this kind of things (alter the default field values used in an entity_create())

Besides, the approach in the current patch would affect *all* fields using the LanguageItem class, i.e all fields of type "language" (i.e, for example the "user preferred language" field), while we only want to target "the field that holds the language of an entity".

So, hook_entity_create() looks like the right approach - it would need to act just on the 'langcode' field.

(it seems the name of "the field that holds the language of an entity" is currently hardcoded to 'langcode'...)

@fago/@yched: Yeah the inherent problem with the hook_entity_create() approach is we need to have a default that is enough for the altering to find out the language was *not properly filled in yet* so the entity language is not overwritten by an overzealous entity_create implementation. Using any of the configured language codes for that would be a problem because how do you tell if those are not deliberately chosen by a user from a UI?! Then picking some other funky language code is a problem because then how do you ensure if the langugae module is not enabled that the language will become something sane even then...

So maybe we need a one-off property on each entity then for only this use case? $entity->languageUninitiliazed = TRUE :D? Better ideas?

Regarding the language key problem - I opened #2143729: Regression: Entity definitions miss a language entity key.

Yeah the inherent problem with the hook_entity_create() approach is we need to have a default that is enough for the altering to find out the language was *not properly filled in yet* so the entity language is not overwritten by an overzealous entity_create implementation.

Yes, the comment says hook_entity_create() is for applying default values but actually we don't know whether we have to apply the default. So, to fix that we could just pass on the $values with which the entity is created to the hook as well, what imo makes sense.

So let's use hook_entity_create() and fix it.

Status:Needs work» Needs review
StatusFileSize
new11.02 KB
PASSED: [[SimpleTest]]: [MySQL] 59,115 pass(es).
[ View ]
new7.48 KB

Addressed the direct code feedback from @fago on #90 1-10 (all of them), except:

6. The reason this is using the web UI (and therefore webtest) is that the language config element is not on the node type API, its altered in. So we could save it on a different API, but this is all in one, easier to see :)

7-10: I actually removed this in favour of drupalCreateNode() (which is also only implemented on the webtestbase heh). I think that should be fine with langcode NULL but I did not try. There is also in fact a drupalCreateContentType() but that in itself would not save the config for the language, so I did not use that. Even if we would use that, it would appear we don't use any UI, but the drupalCreate* methods are on the webtestbase :)

I did not address the hook entity create stuff yet.

BTW it is also fascinating that the test is in views module. Why not language module? :D

Reviewed the Code Changes, looks good. Let's see what Testbot thinks

StatusFileSize
new11.01 KB
FAILED: [[SimpleTest]]: [MySQL] 59,135 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
new689 bytes

Moved to language module, not in views module anymore :D Also moved to language tests namespace (from system tests namespace :D).

The last submitted patch, 98: 1966436-default-language-96.patch, failed testing.

Erm, I don't see how #95 would pass and #98 would fail :/ I just renamed the test file. Did it not even run before? :/

well, afaik this is supposed to test the API, so it's strange to bother with the UI then?

As I understood the issue the problem is that node-creation API does not cover the language, thus it's an API problem. Consequently, I'd expect to see an API being tested not the UI on top of it. If it goes via the node form it could be altered in by something, i.e. a regular API usage might not work as expected.

Thus, imo it would be best to do it as unit-test case. I don't think it's a big deal if you have to set content type language defaults somehow differently - that's the API we have....

@fago: right, drupalCreateNode() and drupalCreateContentType() are only on the web test base though. So we need one-off custom copies of that then on unit test base then. :/ They don't use the UI they use the API already in webtestbase.

So looks like remaining tasks are:

- figure out why the test fails now
- restore the node creation method but with a nicer name and all of #90 6-10 points addressed
- convert the test to a unit test
- move the logic from the language type to the language module entity_create hook passing on the default values, so they can be inspected and the right default can be added.

I'll not be able to work on this more this week, so feel free to continue.

StatusFileSize
new10.76 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1966436-default-language-104.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new6.74 KB

I made all improvements pointed in #103, except moving logic to hook_entity_create. The reason is that we don't have initial $values array in this hook, so don't know which values was passed in, and which were created from defaults.

For the test I used DrupalUnitTestBase. I also moved the file from the UnitTest namespace (probably that was why test failed).

The last submitted patch, 104: 1966436-default-language-104.patch, failed testing.

StatusFileSize
new10.79 KB
FAILED: [[SimpleTest]]: [MySQL] 63,052 pass(es), 5 fail(s), and 1 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 106: 1966436-default-language-105.patch, failed testing.

When I worked on the patch I executed only EntityDefaultLanguageTest on my local machine, but when I submitted my patch, two other tests had failed. I tried to quickly find out why they fail, but I dived so deep in the code and issues, so I need a lifebuoy now :)

Here are some thoughts that came in my mind during my research.

TranslationLinkTest fails because of this change made during #2076445: Make sure language codes for original field values always match entity language regardless of field translatability.
When we change the langcode of the second user in TranslationLinkTest::setUp()

<?php
$user
->langcode = Language::LANGCODE_NOT_SPECIFIED;
?>

the ContentEntityBase::onChange() throws an InvalidArgumentException.
I don't know if this is a correct behavior.

(I did not check why the second test fails.)

Regarding the issue about should we use hook_entity_create or LanguageItem::applyDefaultValue, I think, that hook_entity_field_info_alter could be used as well to set default value in the langcode field definition. But the hook could be used only after #2114707: Allow per-bundle overrides of field definitions is done.
Here is what I tried, and it worked for me:

<?php
function language_entity_field_info_alter(&$info, $entity_type) {
  if (isset(
$info['definitions']['langcode'])) {
   
$langcode = language_get_default_langcode($entity_type, 'page');
   
$info['definitions']['langcode']->setSetting('default_value', $langcode);
  }
}
?>

IMO it's a good place, because the hook called only once, and then cached (probably we'll need to clear that cache on default language setting update).

Just marked #2156945: Clean up installer entity defaults following bugfix a duplicate. That briefly made HEAD fail due to another patch :/ Let's make sure we have test coverage properly for creating entities without languages and then loading them and ensuring they work.

Taking this ( @ global Sprint Weekend ) ...

Status:Needs work» Needs review

Submitting Leksat's #106 for review without changes.
(It turns out I've just been using it for some self training on installing PHPUnit / running tests for the first time. Being able to learn from practical examples is nice :) )

Hoping to sum up current status of this issue:

#109 : "Let's make sure we have test coverage properly for creating entities without languages..." - the code reads like Leksat has everything covered as per #105

#108 -1 : local tests produce no failures to I'm assuming the encountered issue is gone

#108 -2 : using hook_entity_field_info_alter() reads like a good idea, right? So do we wait for #2114707: Allow per-bundle overrides of field definitions to be committed?

Status:Needs review» Needs work

The last submitted patch, 106: 1966436-default-language-105.patch, failed testing.

Assigned:Unassigned» vasi1186

StatusFileSize
new11.55 KB
PASSED: [[SimpleTest]]: [MySQL] 63,093 pass(es).
[ View ]
new777 bytes

Hi,

after spending some time on debugging, I've found that the issue comes from content_translation_load_translation_metadata(), and more specifically because a call to $entity->initTranslation($langcode) for the 'und' langcode is made. As Lekstat says, this has the root in TranslationLinkTest::setUp()

$user->langcode = Language::LANGCODE_NOT_SPECIFIED;

The general issue was that you could not assign anymore the Language::LANGCODE_NOT_SPECIFIED to an entity which had the translation enabled. In this case, the translation was enabled in the ContentTranslationTestBase::enableTranslation().
And the exception was thrown in ContentEntityBase::onChange() (when you tried to change the langcode of the entity) becasue the $entity->initTranslation($langcode)call in content_translation_load_translation_metadata() inserted an entry in the $entity->translations array for the 'und' langcode.

But, as I could find out, the issue was coming from ContentEntityBase::setDefaultLangcode(). The defaultLancode property was set there always to Language::LANGCODE_NOT_SPECIFIED when it could not be set from the language of the item. So, what I did, was to add this:

if (empty($this->defaultLangcode)) {
  if (\Drupal::moduleHandler()->moduleExists('language')) {
    $this->defaultLangcode = language_get_default_langcode($this->entityType(), $this->bundle());
  }
}

I think it makes sense that the setDefaultLangcode method uses the language_get_default_langcode() if possible before Language::LANGCODE_NOT_SPECIFIED (unless there was a good reason why to not use here).

Let's see if the test bot will give us a green for the tests.

The next thing would be that this code:

if (\Drupal::moduleHandler()->moduleExists('language')) {
  $this->defaultLangcode = language_get_default_langcode($this->entityType(), $this->bundle());
}

is now used in two more places (at least 2 which are introduced in this patch). So, should we think to another solution for this?

Status:Needs work» Needs review

Yeah the theory was definitely that it would be enough to set in the LanguageItem. @fago, @yched?

I'm afraid I'm a bit lost here :-( - @plach / @fago would probably know better.

StatusFileSize
new11.79 KB
FAILED: [[SimpleTest]]: [MySQL] 55,472 pass(es), 121 fail(s), and 4 exception(s).
[ View ]
new970 bytes

Test is green in #115. Updating it with small code fix. Do we still have any item to address here?

I think #117 still needs to be addressed. Doing this at two places was not something @fago liked back then :/

Yeah, I think berdir in #45 pointed it out quite well:

The correct place to set the default language for EntityNG would be LanguageItem::applyDefault().
The easiest way would be a if module exists check there, but language.module could also alter the typed data language_field and replace the used class.

Yeah, problem is LanguageItem is provided by the Entity component. Couldn't we just move LanguageItem to the language module? I don't think it has to be in the component?

Also, the default logic shouldn't be duplicated - yes. Not sure what's the problem here, but we should fix the real problem instead.

uhm - as berdir pointed out in IRC language.module is optional (I forgot), so that's a bad idea then. Else, I think it should implement the field_info_alter hook and swap out the language class with a variant that the default value.

We go with that pattern already in entityreference module, which swaps out the default entityreference class.

Status:Needs review» Needs work

The last submitted patch, 119: 1966436-default-content-entity-language-119.patch, failed testing.

The last submitted patch, 119: 1966436-default-content-entity-language-119.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new11.8 KB
PASSED: [[SimpleTest]]: [MySQL] 63,664 pass(es).
[ View ]
new1.55 KB

Seems that a method name changed.

Assigned:vasi1186» segi
Issue tags:+Needs reroll

I'm rerolling this patch.

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new5.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,762 pass(es).
[ View ]

Here it is the rerolled patch. I only rewrite the patch in comment #126.

Status:Needs review» Needs work

The last submitted patch, 129: 1966436-128.patch, failed testing.

Status:Needs work» Needs review

129: 1966436-128.patch queued for re-testing.

Issue tags:-Needs reroll

Issue summary:View changes
Status:Needs review» Needs work

I think it's needs work for #122

Issue tags:+Needs reroll

We also lost the test file in the latest re-roll, that needs to be re-added from the previous patch.

Assigned:segi» Unassigned

rerolling #126

Status:Needs work» Needs review
StatusFileSize
new11.82 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,724 pass(es), 16 fail(s), and 0 exception(s).
[ View ]

Attached a new patch rerolling #126 again. Test is inclued into the patch.

Status:Needs review» Needs work

The last submitted patch, 137: 1966436-137.patch, failed testing.

Assigned:Unassigned» jlbellido

I'll work on this

Assigned:jlbellido» Unassigned

ContentEntityBaseUnitTest fails because of this:

--- a/core/lib/Drupal/Core/Entity/ContentEntityBase.php
+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -549,8 +549,13 @@ protected function setDefaultLangcode() {
       $this->defaultLangcode = $item->language->id;
     }
     if (empty($this->defaultLangcode)) {
-      // Make sure we return a proper language object.
-      $this->defaultLangcode = Language::LANGCODE_NOT_SPECIFIED;
+      if (\Drupal::moduleHandler()->moduleExists('language')) {

This test was added in #2134857: PHPUnit test the entity base classes, i don't know how to manage this. How it should be managed?

Issue tags:-Needs reroll

It still applies

Status:Needs work» Needs review
StatusFileSize
new1.85 KB
new13.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,730 pass(es).
[ View ]

Updated ContentEntityBaseUnitTest to add $moduleHandler as MockObject and solve the testing errors.

Thanks a lot to @Berdir for help me here.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -542,8 +542,13 @@ protected function setDefaultLangcode() {
+      if (\Drupal::moduleHandler()->moduleExists('language')) {

Can we inject the module handler?

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationTest.php
@@ -44,6 +44,13 @@ protected function _testEntityLanguageMethods($entity_type) {
+    $this->assertEqual($entity->language()->id, language_default()->id, format_string('%entity_type: Entity created with API has default language.', array('%entity_type' => $entity_type)));

language_default() is deprecated

Assigned:Unassigned» jlbellido
Status:Needs review» Needs work

Thanks for review, i'll do.

We could use the same pattern for the module handler with a getModuleHandler() method within ContentEntityBase but before you spend too much time on that, we should verify if this is actually the correct approach.

Status:Needs work» Needs review
StatusFileSize
new2.02 KB
new14.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,740 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Injected \Drupal::moduleHandler() at ContentEntityBase Class and fixed language_default() call.

Status:Needs review» Needs work

The last submitted patch, 146: 1966436-146.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new14.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,741 pass(es).
[ View ]
new1.03 KB

Fixed errors at EntityTranslationTest, sorry for noise.

StatusFileSize
new3.8 KB
new14.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,873 pass(es).
[ View ]

Done multiple things:
* Re-roll
* Fixed fago's comment from #122, therefor introduced new language item in language module, similar to entity_reference, which overwrite the original languageItem. Probably we will rename this on? Don't know exactly how to name it at best?
* Found at the end the reason, why we had to introduce setting the default two times. The defaultLangcode in ContentEntityBase is only changed in the onChange method. But the FieldItemList calls applyDefaultValue($notify = FALSE) and therefor the onChange method will not be called and the defaultLangcode will not be changed.

+++ b/core/modules/language/language.module
@@ -730,3 +730,11 @@ function language_system_regional_settings_form_submit($form, &$form_state) {
+function language_field_info_alter(&$info) {
+  // Change the default behavior of language field.
+  $info['language']['class'] = '\Drupal\language\DefaultLanguageItem';
+}
+++ b/core/modules/language/lib/Drupal/language/DefaultLanguageItem.php
@@ -0,0 +1,40 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\language\DefaultLanguageItem
+ */
+
+namespace Drupal\language;
+
+use Drupal\Core\Field\Plugin\Field\FieldType\LanguageItem;
+use Drupal\Core\Language\Language;
+
+/**
+ * Alternative plugin implementation of the 'language' field type.
+ *
+ * Replaces the Core 'language' entity field type implementation, changes the
+ * default values used.
+ *
+ * Required settings are:
+ *  - target_type: The entity type to reference.
+ *
+ * @see language_field_info_alter().
+ */
+class DefaultLanguageItem extends LanguageItem {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function applyDefaultValue($notify = TRUE) {

So this switches in a different FieldItem class for 'language' in order to be able to override FieldItem::applyDefaultValue().

Meanwhile, #1979260: Automatically populate the author default value with the current user introduces a one-off OwnerFieldDefinition class class to be able to override FieldDefinition::getDefaultValue().

We should really try to come with a unified and more flexible way of specifying custom runtime behavior for default values for base fields (configurable fields allow "default value callbacks" already). If I get things right, that should be addressed in #2226267: Improve default value handling of fields to be consistent.

Status:Needs review» Needs work

This is mostly looking good, I have some remarks though:

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -169,6 +169,16 @@ protected function typedDataManager() {
    +   * Returns the module handler.
    +   *
    +   * @return \Drupal\Core\Extension\ModuleHandlerInterface
    +   *   The module handler.
    +   */
    +  protected function moduleHandler() {
    +    return \Drupal::moduleHandler();
    +  }
    +
    +  /**

    Eeew, luckily it seems this is no longer needed :)

  2. +++ b/core/modules/language/lib/Drupal/language/DefaultLanguageItem.php
    @@ -0,0 +1,40 @@
    + * Contains \Drupal\language\DefaultLanguageItem

    Missing trailing dot :)

  3. +++ b/core/modules/language/lib/Drupal/language/DefaultLanguageItem.php
    @@ -0,0 +1,40 @@
    +      $langcode = language_get_default_langcode($entity->getEntityTypeId(), $entity->bundle());

    Can we put this call in a separate method so we can unit test this class by mocking the method?

  4. +++ b/core/modules/language/tests/Drupal/language/Tests/EntityDefaultLanguageTest.php
    @@ -0,0 +1,145 @@
    +  public function testEntityTranslationDefaultLanguageViaCode() {

    This test looks really good :)

  5. +++ b/core/modules/language/tests/Drupal/language/Tests/EntityDefaultLanguageTest.php
    @@ -0,0 +1,145 @@
    +    $content_type = entity_create('node_type', array(
    ...
    +    $node = entity_create('node', $values);
    +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationTest.php
    @@ -44,6 +44,13 @@ protected function _testEntityLanguageMethods($entity_type) {
    +    $entity = entity_create($entity_type, array(
    @@ -149,7 +156,7 @@ protected function _testMultilingualProperties($entity_type) {
    +    $entity = entity_create($entity_type, array('name' => $name, 'user_id' => $uid, 'langcode' => Language::LANGCODE_NOT_SPECIFIED));

    Please let's use the entity storage directly instead of calling the procedural wrappers from OO code.

Status:Needs work» Needs review
StatusFileSize
new14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,905 pass(es).
[ View ]
new3.74 KB

Just fixed all the stuff mentioned in #152.

Status:Needs review» Needs work

+++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
@@ -136,12 +144,17 @@ public function setUp() {
-
+    $this->moduleHandler = $this->getMock('\Drupal\Core\Extension\ModuleHandlerInterface');
+    $this->moduleHandler->expects($this->any())
+      ->method('module_exists')
+      ->with('language')
+      ->will($this->returnValue(FALSE));
     $container = new ContainerBuilder();
     $container->set('entity.manager', $this->entityManager);

The unit test changes should no longer be needed then?

I guess we should now also be able to clean up some manual code that we currently have, like NodeController::add()? Checked Term and CustomBlock and they apparently don't do this currently.

Status:Needs work» Needs review
StatusFileSize
new6.22 KB
new16.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,878 pass(es).
[ View ]

Thanks Berdir for pointing this out.
I removed this from the test.

Searched for "get_default_langcode" in core/ and found that is was used for setting default values in:
* ContentEntityNormalizer::denormalize
* NodeController::add
* ShortcutController::addForm
* TaxonomyController::addForm

I removed this from all of these.

  1. +++ b/core/modules/language/lib/Drupal/language/DefaultLanguageItem.php
    @@ -0,0 +1,45 @@
    + * Replaces the Core 'language' entity field type implementation, changes the

    Just field type, not entity field type. Wondering if it would be clearer if you would simply write "Replaces \Drupal\Core\Field\Plugin\Field\FieldType\LanguageItem ...". It's a bit a long namespace...

  2. +++ b/core/modules/language/lib/Drupal/language/DefaultLanguageItem.php
    @@ -0,0 +1,45 @@
    + * @see language_field_info_alter().

    No "." at the end of a @see.

  3. +++ b/core/modules/language/lib/Drupal/language/DefaultLanguageItem.php
    @@ -0,0 +1,45 @@
    +
    +  public function getDefaultLangcode(EntityInterface $entity) {
    +    return language_get_default_langcode($entity->getEntityTypeId(), $entity->bundle());
    +  }

    Method is missing some basic documentation for the argument and return value.

Didn't have a close look at the tests but other than the points above, this looks good to me, nice work.

StatusFileSize
new16.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,012 pass(es).
[ View ]
new1.32 KB

Done.

Status:Needs review» Needs work

+++ b/core/modules/language/lib/Drupal/language/DefaultLanguageItem.php
@@ -38,6 +38,15 @@ public function applyDefaultValue($notify = TRUE) {
+   * @param EntityInterface $entity

The type hint class/interface needs to be fully qualified with it s namespace, sorry :(

Status:Needs work» Needs review
StatusFileSize
new16.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,630 pass(es).
[ View ]
new1.61 KB

Updated...

Assigned:jlbellido» Unassigned

I picked this up and tried to reproduce. Having trouble creating the node programmatically. I wrote a small script that contains the following:

<?php
$e = entity_create('node', array(
  'type' => 'foo',
  'title' => 'moo',
  'langcode' => NULL,
));
$e->save();

and I try to execute it using drush eval creating-entity.php. Nothing happens. No errors, No entity created. Nothing.

When I used drush scr creating-entity.php, I at least get an error: .........

In the middle of writing this, I went back and ran it again and what do you know... drush scr creating-entity.php worked. It's creating entity.

I run @dags code with devel/php, works for me.

There is a test-only patch in #75 (original in #69) that I'm trying to run locally but having trouble. The test fails saying, "Invalid permission administer content types." and "Invalid permission bypass node access." .. which ultimately causes $this->drupalLogin() to fail. Anyone know why this would be happening?

well,

ag "'administer content types'" core | grep -v "Test"

says the permission is defined in
core/modules/node/node.module:735: 'administer content types' => array(

In the patch in #75

+class EntityTranslationDefaultLanguageTest extends WebTestBase {
+
+  /**
+   * Modules to enable.
+   *
+   * @var array
+   */
+  public static $modules = array('language');
+
+  public static function getInfo() {
+    return array(
+      'name' => 'Entity Translation default language',
+      'description' => 'Test that entities are created with correct language code.',
+      'group' => 'Entity API',
+    );
+  }
+
+  public function setUp() {
+    parent::setUp();
+
+    // Create a new administrator user for the test.
+    $admin = $this->drupalCreateUser(
+      array(
+        'administer content types',
+        'administer languages',
+        'bypass node access',
+      )
+    );

so maybe node module needs to be required in $modules, and maybe what ever defines bypass node access?

Exactly, node is no longer required, so you need to enable it in your test.

#1541298: Remove Node module dependency from Testing profile was committed on Feb 13 2014. So patches with tests that require node, that are older than that might all need a similar update.