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.

CommentFileSizeAuthor
#243 language.entity.kerneltest.patch307 bytessun
#235 interdiff.txt1.06 KBGábor Hojtsy
#235 1966436-default-content-entity-language-234.patch37.03 KBGábor Hojtsy
#234 interdiff.txt1.76 KBGábor Hojtsy
#234 1966436-default-content-entity-language-234.patch37.06 KBGábor Hojtsy
#233 1966436-default-content-entity-language-233.patch36.95 KBGábor Hojtsy
#233 interdiff.txt822 bytesGábor Hojtsy
#230 1966436-default-content-entity-language-230.patch36.15 KBGábor Hojtsy
#228 1966436-default-content-entity-language-228.patch36.15 KBBerdir
#219 1966436-diff-215-219.txt3.99 KBvijaycs85
#219 1966436-default-content-entity-language-219.patch36.13 KBvijaycs85
#215 1966436-default-content-entity-language-215-interdiff.txt890 bytesnaveenvalecha
#215 1966436-default-content-entity-language-215.patch36.12 KBnaveenvalecha
#212 1966436-default-content-entity-language-212.patch35.49 KBnaveenvalecha
#211 1966436-default-content-entity-language-211.patch35.8 KBnaveenvalecha
#208 1966436-default-content-entity-language-208-interdiff.txt1.71 KBBerdir
#208 1966436-default-content-entity-language-208.patch36.59 KBBerdir
#206 1966436-default-content-entity-language-206.patch36.57 KBBerdir
#200 1966436-default-content-entity-language-200.patch35.51 KBcesarmiquel
#198 1966436-default-content-entity-language-198.patch28.15 KBcesarmiquel
#195 1966436-default-content-entity-language-195-interdiff.txt1.47 KBBerdir
#195 1966436-default-content-entity-language-195.patch37.72 KBBerdir
#194 1966436-default-content-entity-language-194-interdiff.txt11.21 KBBerdir
#194 1966436-default-content-entity-language-194.patch42.17 KBBerdir
#192 1966436-default-content-entity-language-192-interdiff.txt3.09 KBBerdir
#192 1966436-default-content-entity-language-192.patch25.78 KBBerdir
#188 1966436-default-content-entity-language-188-interdiff.txt731 bytesBerdir
#188 1966436-default-content-entity-language-188.patch22.69 KBBerdir
#186 1966436-default-content-entity-language-186.patch17.29 KBpenyaskito
#186 1966436-default-content-entity-language.168-186.txt581 bytespenyaskito
#182 interdiff.txt1.63 KBGábor Hojtsy
#182 1966436-default-content-entity-language-182.patch21.97 KBGábor Hojtsy
#178 1966436-default-content-entity-language-178-interdiff.txt5.25 KBBerdir
#178 1966436-default-content-entity-language-178.patch21.98 KBBerdir
#168 patch-diff-158-168.txt1.71 KBmr.york
#168 1966436-default-content-entity-language-168.patch16.73 KBmr.york
#158 1966436-diff-157-158.txt1.61 KBvijaycs85
#158 1966436-default-content-entity-language-158.patch16.83 KBvijaycs85
#156 interdiff-154-156.txt1.32 KBmr.york
#156 1966436-156.patch16.77 KBmr.york
#154 1966436-154.patch16.52 KBkfritsche
#154 1966436-154.interdiff.txt6.22 KBkfritsche
#152 1966436-152.interdiff.txt3.74 KBkfritsche
#152 1966436-152.patch14 KBkfritsche
#149 1966436-149.patch14.33 KBkfritsche
#149 1966436-149.interdiff.txt3.8 KBkfritsche
#148 interdiff.txt1.03 KBjlbellido
#148 1966436-148.patch14.08 KBjlbellido
#146 1966436-146.patch14.04 KBjlbellido
#146 interdiff.txt2.02 KBjlbellido
#142 1966436-142.patch13.66 KBjlbellido
#142 interdiff.txt1.85 KBjlbellido
#137 1966436-137.patch11.82 KBjlbellido
#129 1966436-128.patch5.42 KBsegi
#126 interdiff-126-119.txt1.55 KBvasi1186
#126 1966436-126.patch11.8 KBvasi1186
#119 1966436-diff-115-119.txt970 bytesvijaycs85
#119 1966436-default-content-entity-language-119.patch11.79 KBvijaycs85
#115 interdiff-106-115.txt777 bytesvasi1186
#115 1966436-115.patch11.55 KBvasi1186
#106 1966436-default-language-105.patch10.79 KBLeksat
#104 interdiff.txt6.74 KBLeksat
#104 1966436-default-language-104.patch10.76 KBLeksat
#98 interdiff.txt689 bytesGábor Hojtsy
#98 1966436-default-language-96.patch11.01 KBGábor Hojtsy
#95 interdiff.txt7.48 KBGábor Hojtsy
#95 1966436-default-language-95.patch11.02 KBGábor Hojtsy
#86 interdiff-82-85.txt1.81 KBSchnitzel
#86 1966436-default-language-85.patch11.25 KBSchnitzel
#82 1966436-default-language-82.patch13.05 KBkfritsche
#82 1966436-default-language-82.interdiff.txt3.45 KBkfritsche
#75 1966436-default-language-66-75.interdiff.txt7.6 KBgrisendo
#75 1966436-default-language-75.only-test.must-fail.patch5.88 KBgrisendo
#75 1966436-default-language-75.patch11.24 KBgrisendo
#71 1966436-default-language-71.patch9.31 KBvijaycs85
#69 1966436-default-language-66-test-only.patch4.64 KBvijaycs85
#69 1966436-default-language-66.patch1.06 KBvijaycs85
#69 1966436-diff-63-66.txt1.06 KBvijaycs85
#66 1966436-default-language-63-66.interdiff.txt3.77 KBgrisendo
#66 1966436-default-language-66.only-test.must-fail.patch4.63 KBgrisendo
#66 1966436-default-language-66.patch10.01 KBgrisendo
#63 1966436-default-language-55-63.interdiff.txt6.22 KBgrisendo
#63 1966436-default-language-63.only-test.must-fail.patch4.63 KBgrisendo
#63 1966436-default-language-63.patch9.3 KBgrisendo
#55 1966436-default-language-53-55.interdiff.txt723 bytesgrisendo
#55 1966436-default-language-55.patch4.57 KBgrisendo
#53 1966436-default-language-50-53-interdiff.txt835 bytesgrisendo
#53 1966436-default-language-53.patch4.31 KBgrisendo
#50 1966436-default-language-47-50-interdiff.txt2.61 KBkfritsche
#50 1966436-default-language-50.patch3.49 KBkfritsche
#47 1966436-default-language-10-47.interdiff.txt1.35 KBgrisendo
#47 1966436-default-language-47.patch900 bytesgrisendo
#40 1966436-default-language-40.patch2.35 KBAron Novak
#37 1966436-default-language-37.patch1.68 KBvictor-shelepen
#37 interdiff-1966436-34-37.txt451 bytesvictor-shelepen
#34 1966436-default-language-34.patch1.69 KBvictor-shelepen
#32 upgrade_path-failed_tests-2032033-6.patch721 bytesvictor-shelepen
#24 interdiff.txt822 bytespenyaskito
#24 1966436-default-language-24.patch1006 bytespenyaskito
#22 1966436-default-language-22.patch846 bytesvijaycs85
#22 1966436-16-22.txt535 bytesvijaycs85
#16 drupal-1966436-default-language-16.patch845 byteskfritsche
#10 d8_langauge_default.patch1.08 KBfago
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

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?

fago’s picture

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...

Gábor Hojtsy’s picture

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

fago’s picture

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?

plach’s picture

plach’s picture

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.

Gábor Hojtsy’s picture

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

plach’s picture

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.

Gábor Hojtsy’s picture

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

fago’s picture

Status: Active » Needs review
FileSize
1.08 KB

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.

andypost’s picture

+++ 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

plach’s picture

Assigned: Unassigned » plach

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

plach’s picture

Issue tags: +sprint
kfritsche’s picture

Assigned: plach » kfritsche
kfritsche’s picture

Assigned: kfritsche » Unassigned
Status: Needs work » Needs review
FileSize
845 bytes

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.)

plach’s picture

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.

itarato’s picture

Assigned: Unassigned » itarato
itarato’s picture

Assigned: itarato » Unassigned
plach’s picture

@itarato:

Are you still working on this?

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
535 bytes
846 bytes

update, as per #17

penyaskito’s picture

Status: Needs work » Needs review
FileSize
1006 bytes
822 bytes

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

penyaskito’s picture

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.

Gábor Hojtsy’s picture

Title: Language module should use hook_entity_create() to set default language » Default entity languages are not set for entities created with the API
Priority: Normal » Major
vijaycs85’s picture

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

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

andypost’s picture

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

victor-shelepen’s picture

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

victor-shelepen’s picture

I duplicate the patch file.

victor-shelepen’s picture

Status: Needs work » Needs review
victor-shelepen’s picture

fago’s picture

Status: Needs review » Needs work

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

fago’s picture

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.

victor-shelepen’s picture

Status: Needs work » Needs review
FileSize
451 bytes
1.68 KB

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

Gábor Hojtsy’s picture

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.

Aron Novak’s picture

Assigned: Unassigned » Aron Novak

trying to adjust it now

Aron Novak’s picture

Assigned: Aron Novak » Unassigned
FileSize
2.35 KB

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.

andypost’s picture

Status: Needs work » Needs review
victor-shelepen’s picture

@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

andypost’s picture

Berdir’s picture

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.

grisendo’s picture

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

grisendo’s picture

Rerolled from #10 and adapted.

grisendo’s picture

Status: Needs work » Needs review
kfritsche’s picture

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.

kfritsche’s picture

Status: Needs work » Needs review
grisendo’s picture

Status: Needs work » Needs review
FileSize
4.31 KB
835 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

grisendo’s picture

Status: Needs work » Needs review
FileSize
4.57 KB
723 bytes

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

grisendo’s picture

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

So, I'll start to work on tests.

Berdir’s picture

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.

leeotzu’s picture

Title: Default entity languages are not set for entities created with the API » Updated 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.

Gábor Hojtsy’s picture

Title: Updated issue summary from the comment logs » Default 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.

grisendo’s picture

Still working on tests right now

plach’s picture

@grisendo:

Are you still on it?

grisendo’s picture

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

grisendo’s picture

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 :)

Berdir’s picture

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.

grisendo’s picture

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.
vasi1186’s picture

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).

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.06 KB
1.06 KB
4.64 KB

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'.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
9.31 KB

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

grisendo’s picture

Assigned: grisendo » Unassigned
das-peter’s picture

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.

grisendo’s picture

Assigned: Unassigned » grisendo

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

grisendo’s picture

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.

das-peter’s picture

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.

grisendo’s picture

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.

pfrenssen’s picture

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.

mmilano’s picture

Assigned: grisendo » mmilano

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

Gábor Hojtsy’s picture

Assigned: mmilano » Unassigned
Issue tags: -sprint, -language-config, -language-content, -Entity Field API
Gábor Hojtsy’s picture

Tag monster.

Gábor Hojtsy’s picture

Issue summary: View changes

Update the issue summary as per the issue log

kfritsche’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
3.45 KB
13.05 KB

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.

kfritsche’s picture

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?

Gábor Hojtsy’s picture

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?!

Schnitzel’s picture

Status: Needs work » Needs review
FileSize
11.25 KB
1.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.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

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

Qualify the scope of this issue is about content entities.

fago’s picture

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.

yched’s picture

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'...)

Gábor Hojtsy’s picture

@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?

fago’s picture

Regarding the language key problem - I opened #2143729: 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.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
11.02 KB
7.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.

Gábor Hojtsy’s picture

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

Schnitzel’s picture

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

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

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? :/

fago’s picture

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....

Gábor Hojtsy’s picture

@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.

Gábor Hojtsy’s picture

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.

Leksat’s picture

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).

Leksat’s picture

Leksat’s picture

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()

$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:

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).

Gábor Hojtsy’s picture

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.

roderik’s picture

Taking this ( @ global Sprint Weekend ) ...

roderik’s picture

Status: Needs work » Needs review
roderik’s picture

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?

vasi1186’s picture

Assigned: Unassigned » vasi1186
vasi1186’s picture

FileSize
11.55 KB
777 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?

vasi1186’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

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

yched’s picture

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

vijaycs85’s picture

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

Gábor Hojtsy’s picture

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

fago’s picture

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.

fago’s picture

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.

vasi1186’s picture

vasi1186’s picture

Status: Needs work » Needs review
FileSize
11.8 KB
1.55 KB

Seems that a method name changed.

segi’s picture

Assigned: vasi1186 » segi
Issue tags: +Needs reroll

I'm rerolling this patch.

segi’s picture

Status: Needs review » Needs work
segi’s picture

Status: Needs work » Needs review
FileSize
5.42 KB

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

segi’s picture

Status: Needs work » Needs review

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

segi’s picture

Issue tags: -Needs reroll
YesCT’s picture

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

I think it's needs work for #122

Berdir’s picture

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.

segi’s picture

Assigned: segi » Unassigned
jlbellido’s picture

rerolling #126

jlbellido’s picture

Status: Needs work » Needs review
FileSize
11.82 KB

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

jlbellido’s picture

Assigned: Unassigned » jlbellido

I'll work on this

jlbellido’s picture

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?

ehegedus’s picture

Issue tags: -Needs reroll

It still applies

jlbellido’s picture

Status: Needs work » Needs review
FileSize
1.85 KB
13.66 KB

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

Thanks a lot to @Berdir for help me here.

ianthomas_uk’s picture

+++ 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

jlbellido’s picture

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

Thanks for review, i'll do.

Berdir’s picture

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.

jlbellido’s picture

Status: Needs work » Needs review
FileSize
2.02 KB
14.04 KB

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

jlbellido’s picture

Status: Needs work » Needs review
FileSize
14.08 KB
1.03 KB

Fixed errors at EntityTranslationTest, sorry for noise.

kfritsche’s picture

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.

yched’s picture

+++ 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.

plach’s picture

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.

kfritsche’s picture

Status: Needs work » Needs review
FileSize
14 KB
3.74 KB

Just fixed all the stuff mentioned in #152.

Berdir’s picture

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.

kfritsche’s picture

Status: Needs work » Needs review
FileSize
6.22 KB
16.52 KB

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.

Berdir’s picture

  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.

mr.york’s picture

FileSize
16.77 KB
1.32 KB

Done.

plach’s picture

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 :(

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
16.83 KB
1.61 KB

Updated...

jlbellido’s picture

Assigned: jlbellido » Unassigned
dags’s picture

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.

mr.york’s picture

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

dags’s picture

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?

YesCT’s picture

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?

Berdir’s picture

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

YesCT’s picture

#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.

Gábor Hojtsy’s picture

mr.york’s picture

mr.york’s picture

Status: Needs work » Needs review
penyaskito’s picture

YesCT’s picture

fails
MigrateAggregatorFeedTest.php 52
MigrateAggregatorItemTest.php 79
MigrateCommentTest.php 79
MigrateCustomBlockTest.php 62

those fails are from Migrate asserts like:
$this->assertEqual($feed->language()->id, Language::LANGCODE_NOT_SPECIFIED);
$this->assertEqual(Language::LANGCODE_NOT_SPECIFIED, $comment->language()->id);

which assume that und is what the langcode is supposed to be.
but with this patch, they are getting en for the langcode.

what do we want it to be?

Gábor Hojtsy’s picture

Defaults for content entities in D8 is the site default language unless otherwise configured. So if the test is asserting otherwise, that is not the expected behaviour in D8. All content entities should be created with the site default language unless otherwise setup in the test. Not und by default. So looks like we want/need to fix the test.

Berdir’s picture

*With* this patch yes, not yet without, right now, it's only for *certain content entities created in the UI where the code explicitly sets the langcode*. Now it's everything. So yes, those tests need to be updated as they rely on the default value.

What's a bit weird is that only the combined test failed, not the single ones... ah, that does make sense, because the separate ones don't have language.module enabled. Because the above is only true if language.module is enabled :)

Which nicely shows the problem that you will have in trying to fix this, the behavior is different with/without language.module and the same test runs in both cases, with the same code.

Suggestion: create a $this->expectedDefaultLangcode = LanguageInterface::LANGCODE_NOT_SPECIFIED in the migrate base class, use that for the expected value in the test, then change it to en in the run-everything test.

Gábor Hojtsy’s picture

I'd much rather fix this to apply the site default language even if language module is not enabled which is our idea for D8 instead of working around this bug in the test.

Berdir’s picture

It is only a bug if the expectation is that it should be en even without language.module :)

We discussed it, so yes, the expectation is that it should always be the default language by default, in that case, we want to switch the hardcoded not specified in LanguageItem:.applyDefaultValue() to \Drupal::languageManager()->getCurrentLanguage()->id() and fix the migrate tests to assert for en instead of und.

This will likely cause a bunch of additional test failures, but we can fix those in a second step.

Berdir’s picture

Expecting a bunch of new test fails now, but should be easy to fix...

vijaycs85’s picture

Fatal error: Call to undefined method Drupal\Core\Language\Language::id() in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/LanguageItem.php on line 95
Drush command terminated abnormally due to an unrecoverable error.       [error]
Error: Call to undefined method Drupal\Core\Language\Language::id()
in
/var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/LanguageItem.php,
line 95
)
Berdir’s picture

Yep, should be getId(), not id(), sorry :)

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
21.97 KB
1.63 KB

Fixed that and another issues in the test.

Gábor Hojtsy’s picture

Based on the fails, it looks like tests are chock full of asserts for und spcecifically :D 412 fail(s), and 21 exception(s) which seems to be all over the place. I'm not sure that is viable to solve here, so it would be good to get back to the dynamic solution for the aggregator migration test then as per the suggestion in #175. Start again from #168 and fix only this for this issue. Otherwise we'll never be done with this.

Not going to do this though as its more involved than what I have capacity for now. Any takers? :)

Berdir’s picture

Ouch. Yeah, that's worse than I expected because it's not just und/en mismatches, a lot tests are falling apart. I suspect most are related to DrupalUnitTestBase or even WebTestBase not properly setting up the current language inside the test? So maybe it should use the site default language instead of current?

Will have a look, it's possible that most fails are caused by that.

penyaskito’s picture

This patch is based on #168.

My view on this is that locale module should not be enabled on migrate test, as during migrate D6-D8 first sprint multilingual was not considered. Removing it makes the tests green locally.

If in any case this is not the desired behavior (which from my POV it is), we should post a bug in the queue, but it should not block to get this patch in.

chx’s picture

#186 will very likely fail on MigrateLocaleConfigsTest $this->assertIdentical($config->get('cache_string'), 1); because the only reason that's not '1' is because the locale module provides config schema and in turn Config casts it based on the schema.

Berdir’s picture

@penyaskito: That just avoids the test failures, not the underlying problem that the default should be en, not und by default, unless you chose it to be that. Based on what Gabor said and what we're doing for the default user and so on in user_install() for example.

I'd still like to pursue the other approach for a moment, let's see how this goes. I'm not exactly sure what the correct behavior is, probably set it to not specified if there is no langcode and default if there is a langcode field? The problem is that by the time setDefaultLangcode() is called, the default values haven't calculated yet..

But first, let's see how many fails will be left with this...

Gábor Hojtsy’s picture

@Berdir: default to site default language unless language module is enabled in which case it may have other settings and intervene should be the overall rule. I'm not sure it will not explode the scope too much to fix that here, but I'm confident if someone could do it, it would be you :)

Gábor Hojtsy’s picture

BTW http://drupalcode.org/project/drupal.git/commitdiff/902098c just added a @todo in user_install on this issue, heh :D

Berdir’s picture

Fixing the unit tests so we can better see what's left, but the test fails in the log looked much better...

Berdir’s picture

Assigned: Unassigned » plach
Status: Needs work » Needs review
FileSize
42.17 KB
11.21 KB

@Gabor: Well, in case language module is not installed, the current language is always the default language is always en, no need to special case something there.

The question is more about entity types without langcode*, they should really be treated as NOT_SPECIFIED and not 'en'. Will look into that when the patch is green. Also asked @plach for his opinion/feedback, so assigning to him so that he doesn't forget...

* Yes, there are perfectly valid use cases for content entities to not have a language. translation jobs and job items from TMGMT for example as a source and target language but no language itself :)

Berdir’s picture

Let's try this.

Could really use some reviews :)

Gábor Hojtsy’s picture

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -574,9 +575,16 @@ protected function setDefaultLangcode() {
+      if ($this->hasField('langcode')) {
+        $this->defaultLangcode = $this->languageManager()->getCurrentLanguage()->getId();
+      }
+      else {
+        $this->defaultLangcode = LanguageInterface::LANGCODE_NOT_SPECIFIED;
+      }

I think the logic makes sense as we discussed earlier that entities without language support would not take default language from language manager, that would be odd :D

cesarmiquel’s picture

I'm at Austin Sprint and this patch is failing due to the PSR-4 change. I'm re-rolling it.

cesarmiquel’s picture

I re-rolled the patch following https://groups.drupal.org/node/424758. I resolved two conflicts: one in core/modules/text/src/Tests/TextWithSummaryItemTest.php and the other in core/modules/serialization/src/Tests/EntitySerializationTest.php. One was a function which was no longer in TextWithSummaryItemTest.php and the other one extra key 'default_langcode' in testSerialize() which is also no longer there.

cesarmiquel’s picture

I seem to have found the problem: two files were removed from the patch. I re-rolled and I'm submitting the fixed patch.

cesarmiquel’s picture

Status: Needs work » Needs review

I seem to have found the problem: two files were removed from the patch. I re-rolled and I'm submitting the fixed patch.

cesarmiquel’s picture

Status: Needs work » Needs review
Berdir’s picture

Status: Needs review » Needs work

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
36.57 KB

There wasn't much that didn't conflict in this patch...

Status: Needs review » Needs work

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

Berdir’s picture

Hm, wasn't able to figure out that test fail yet.

Status: Needs review » Needs work

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

Gábor Hojtsy’s picture

Issue tags: -language-config

Remove config tag, this does not deal with config anymore but that was broken out to #2144249: Write test that Config Entites created via API are created in Site Default language in #86-#89.

naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
35.8 KB

Rerolled the above patch #208 patch .As the patch was not applied so I applied it manually. So no diff attached.
Also the Book test passed locally.Looking for the testbot what it shows.

Thanks
Naveen Valecha

naveenvalecha’s picture

Rerolled the above patch after fixing some of the space issues.
Thanks
Naveen Valecha

Status: Needs review » Needs work

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

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

naveenvalecha’s picture

Added the fix for the BookApitest.php

Thanks
Naveen Valecha

naveenvalecha’s picture

Status: Needs work » Needs review

Changed status

Status: Needs review » Needs work

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

naveenvalecha’s picture

Hi
If I will enable the user module in EntityDefaultLanguageTest.php .Then these test fails.

Value 'en' is equal to value 'und'.	Other	EntityDefaultLanguageTest.php	87	Drupal\language\Tests\EntityDefaultLanguageTest->testEntityTranslationDefaultLanguageViaCode()	Fail
Value 'en' is equal to value 'und'.	Other	EntityDefaultLanguageTest.php	98	Drupal\language\Tests\EntityDefaultLanguageTest->testEntityTranslationDefaultLanguageViaCode()

Looking forward to hear regarding the above errors fix.

Thanks
Naveen Valecha

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
36.13 KB
3.99 KB

Reroll+test case update...

Gábor Hojtsy’s picture

Assigned: plach » Unassigned
fago’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -588,9 +588,16 @@ protected function setDefaultLangcode() {
    +        $this->defaultLangcode = $this->languageManager()->getCurrentLanguage()->getId();
    +      }
    

    Once the default value handling has been moved to the new approach (see below), it should be possible to get default value from the "langcode"'s field definition without instantiating the respective field object, thus without duplicating the logic here.

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/LanguageItem.php
    @@ -91,8 +91,8 @@ public function setValue($values, $notify = TRUE) {
    -    // Default to LANGCODE_NOT_SPECIFIED.
    -    $this->setValue(array('value' => LanguageInterface::LANGCODE_NOT_SPECIFIED), $notify);
    +    // Default to the current site language.
    +    $this->setValue(array('value' => \Drupal::languageManager()->getCurrentLanguage()->getId()), $notify);
    

    As noted in #2318605: uuid, created, changed, language default value implementations need to be updated this default value logic is deprecated. We should use the new one and implement a respective FieldItemList class which can process the default value to the right value (unless specified otherwise).

jhodgdon’s picture

Question: I have been noticing that even in the UI, if you create an entity before the Language module is installed, the language is unset. See #2323935: Translations page for content created prior to enabling shows "unpublished" for English versions... Is this the same as the current issue, or is it a separate issue?

Gábor Hojtsy’s picture

@jhodgdon: it is theoretically a different issue, but being resolved here as it is technically closely related. Its the same code. See below:

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -588,9 +588,16 @@ protected function setDefaultLangcode() {
         if (empty($this->defaultLangcode)) {
    -      // Make sure we return a proper language object.
    -      $this->defaultLangcode = LanguageInterface::LANGCODE_NOT_SPECIFIED;
    +      // Make sure we return a proper language object, if the entity has a
    +      // langcode field, default to the current language.
    +      if ($this->hasField('langcode')) {
    +        $this->defaultLangcode = $this->languageManager()->getCurrentLanguage()->getId();
    +      }
    +      else {
    +        $this->defaultLangcode = LanguageInterface::LANGCODE_NOT_SPECIFIED;
    +      }
    
    +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/LanguageItem.php
    @@ -91,8 +91,8 @@ public function setValue($values, $notify = TRUE) {
       public function applyDefaultValue($notify = TRUE) {
    -    // Default to LANGCODE_NOT_SPECIFIED.
    -    $this->setValue(array('value' => LanguageInterface::LANGCODE_NOT_SPECIFIED), $notify);
    +    // Default to the current site language.
    +    $this->setValue(array('value' => \Drupal::languageManager()->getCurrentLanguage()->getId()), $notify);
         return $this;
    

    These change the default behavior to create entities in the language of the page instead of not specified. So without the language module they will all be English.

  2. +++ b/core/modules/language/language.module
    @@ -680,3 +680,11 @@ function language_system_regional_settings_form_submit($form, FormStateInterface
    +function language_field_info_alter(&$info) {
    +  // Change the default behavior of language field.
    +  $info['language']['class'] = '\Drupal\language\DefaultLanguageItem';
    +}
    
    +++ b/core/modules/language/src/DefaultLanguageItem.php
    @@ -0,0 +1,54 @@
    +  public function applyDefaultValue($notify = TRUE) {
    +    // Default to LANGCODE_NOT_SPECIFIED.
    +    $langcode = Language::LANGCODE_NOT_SPECIFIED;
    +    if ($entity = $this->getEntity()) {
    +      $langcode = $this->getDefaultLangcode($entity);
    +    }
    +    // Always notify otherwise default langcode will not be set correctly.
    +    $this->setValue(array('value' => $langcode), TRUE);
    +    return $this;
    +  }
    

    These ensure that when the language module is enabled, it then uses the configured default language for each entity/bundle.

The without-language-module changes are necessary to do here because the defaults would be very different with the language module if you use the API. So the defaults need to be aligned for both cases.

Status: Needs review » Needs work

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

jhodgdon’s picture

This issue is blocking progress on several other issues... what still needs to be done?

Gábor Hojtsy’s picture

Issue tags: +Needs reroll

@jhodgdon: todo: 1. reroll 2. feedback in #221 as far as I see.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
36.15 KB

Re-roll.

Status: Needs review » Needs work

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
36.15 KB

Hand edited language_entity to configurable_language. The internal entity type name changed since the last patch.

Status: Needs review » Needs work

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

jhodgdon’s picture

I looked carefully through the current patch... aside from the 1 test failure (which is hopefully easy to address), it mostly looks great!

So... I'm a bit confused at the disconnect in the default settings for language, between when language module is enabled and not:

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
+      if ($this->hasField('langcode')) {
+        $this->defaultLangcode = $this->languageManager()->getCurrentLanguage()->getId();
+      }
+      else {
+        $this->defaultLangcode = LanguageInterface::LANGCODE_NOT_SPECIFIED;
+      }
...
// Note that getCurrentLanguage() returns the language for LanguageInterface::TYPE_INTERFACE if
// called with no argument.
...
+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/LanguageItem.php
+    // Default to the current site language.
+    $this->setValue(array('value' => \Drupal::languageManager()->getCurrentLanguage()->getId()), $notify);
...
+++ b/core/modules/language/src/DefaultLanguageItem.php
+  public function getDefaultLangcode(EntityInterface $entity) {
+    return language_get_default_langcode($entity->getEntityTypeId(), $entity->bundle());
+  }
// language_get_default_langcode() is getting the bundle's default language setting, or
// 'site_default' if this is not set.

So it looks like sometimes the default is the site default language, and sometimes it is the UI language. Shouldn't these be consistent?

Then in core/modules/language/tests/src/EntityDefaultLanguageTest.php -- it seems to be testing that you get site default language without Language module installed, and a configured definite language if Language module is installed... which is the defined behavior from above. But it doesn't seem to test configuration for other choices of bundle language I think, such as "author's preferred language" etc. Are there tests elsewhere for that?

Then:

+++ b/core/modules/system/src/Tests/Entity/EntityTranslationTest.php
+    $this->assertEqual($entity->language()->getId(), $this->languageManager->getCurrentLanguage(LanguageInterface::TYPE_CONTENT)->id, format_string('%entity_type: Entity created with API has default language.', array('%entity_type' => $entity_type)));

I think this should be testing that the language matches LanguageInterface::TYPE_INTERFACE, not TYPE_CONTENT? I know they would normally match, but...

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
822 bytes
36.95 KB

First fixing the fail. We now have tests for admin language negotiation, which assumed the admin preferred language on the user is not initialised to any meaningful value until the user picks something. This is the expectation. Now with the language field defaulting to things (which is great otherwise), the test failed. The fix is easy, we provide an empty default for this field in the base fields definition.

Gábor Hojtsy’s picture

@jhodgdon: the difference in defaults looks like semantics. It is default to the page negotiated language until the language module is turned on when it defaults to the site default language. But if you consider what may be the page negotiated language before the language module is turned on, that would also be the site default language. So the defaults are *practically* the same AFAIS, the semantic difference is we use different code to retrieve them. This is getCurrentLanguage() implemented in the default LanguageManager (used before language module is enabled):

  public function getCurrentLanguage($type = LanguageInterface::TYPE_INTERFACE) {
    return $this->getDefaultLanguage();
  }

We can make this more explicit by using the getDefaultLanguage() method instead for the default cases. This should lead to the same results (holding breath :D).

Your other two points to be addressed later.

Gábor Hojtsy’s picture

@jhodgdon: as for your two remaining points EntityDefaultLanguageTest.php is indeed not testing all combinations but LanguageConfigurationElementTest already tests all variations of configuring with the dynamic language settings and that they end up with the right values returned from language_get_default_langcode().

And finally for the misuse of TYPE_CONTENT, we could just as well check for the site default language there, since we don't specifically configure the type for any other behaviour. Patch updated.

Gábor Hojtsy’s picture

@fago/#221: I went to #2318605: uuid, created, changed, language default value implementations need to be updated and to #2226267: Improve default value handling of fields to be consistent from there and found no change notice or explanation as to what before code should be what after code. I would really appreciate some help figuring this out because I have no idea.

Or alternatively fix the issue and do the conversion differently, since this is a bug in the code regardless of conversion (the bug would be there even if converted). Not trying to do multiple things at once in an issue is always something I value for reviewability and committability without arguments.

fago’s picture

@fago/#221: I went to #2318605: uuid, created, changed default value implementations need to be updated and to #2226267: Improve default value handling of fields to be consistent from there and found no change notice or explanation as to what before code should be what after code. I would really appreciate some help figuring this out because I have no idea.

yeah, I guess I was a bit too quick with #221 - please ignore it ;) We should work on #2318605 first and make sure we've figured out how we do field-type default values and then apply the same pattern here and to other field type modues. There is no reason for language having to be first.

I took a short look at the code and the default value logic used follow the same pattern as other existing field types, thus imo is fine for now.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +blocker

Looks good to me! This is blocking other issues... has tests... makes things more rational... let's get it in.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Looked through and couldn't find anything to complain about. Very nice and well-commented test coverage added.

+++ b/core/modules/language/tests/src/EntityDefaultLanguageTest.php
@@ -0,0 +1,151 @@
+  protected function createContentType($name, $langcode) {
...
+  protected function createNode($type, $langcode = NULL) {

This is not a show-stopper by any means, but it's very strange to me to declare such generic helper functions in one arbitrary test class vs. a base class that other tests can take advantage of. Maybe we could have a follow-up to discuss moving them to KernelTestBase?

Elsewise, looks good to go.

Committed and pushed to 8.x. Thanks!

  • webchick committed 9c537ec on 8.0.x
    Issue #1966436 by naveenvalecha, cesarmiquel, Berdir, mr.york, jlbellido...
jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

"Fixed" status didn't stick. ?!? Thanks webchick!

Gábor Hojtsy’s picture

@webchick: the methods added here make creating content types / nodes easier with language settings. Not sure those specifics should be on base classes.

sun’s picture

Status: Fixed » Needs review
FileSize
307 bytes

Quick follow-up: The new kernel test was added in the unit tests directory.

andypost’s picture

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for catching that!

Committed and pushed to 8.x.

  • webchick committed 84dfd06 on 8.0.x
    Issue #1966436 by follow-up by sun: Move tests to proper directory.
    
Berdir’s picture

Note: drupalCreateNode() still hardcodes langcode => und: #2333731: WebTestBase::drupalCreateNode() should not hardcode default values, so many tests are not actually running with real life scenarios now...

Berdir’s picture

Issue tags: -sprint

Status: Fixed » Closed (fixed)

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

andyceo’s picture

Found reference to this issue in user.install file:

/**
 * Implements hook_install().
 */
function user_install() {
  $storage = \Drupal::entityManager()->getStorage('user');
  // @todo Rely on the default value for langcode in
  //   https://drupal.org/node/1966436
  $langcode = \Drupal::languageManager()->getDefaultLanguage()->getId();

Is there a followup somewhere?

Gábor Hojtsy’s picture

@andyceo: I am not aware of such an issue. Can you open one?

andypost’s picture