Problem/Motivation

While working on #1966436: Default *content* entity languages are not set for entities created with the API we figured that Drupal 8 misses the 'language' entity key, as it got introduced for Drupal 7 in #1495648: Introduce entity language support.

Right now Drupal 8 hard-codes "langcode" as language key, but it should respect the defined 'language' key as we do for the other keys.

Proposed resolution

Check for what language key is defined.

Remaining tasks

None

User interface changes

None

API changes

Only the langcode entity key API addition.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because we have a language entity key in D7, hence this fixes a regression.
Issue priority Major because all D7 sites exploiting the language entity key could not upgrade properly to D8 without schema changes or ugly workarounds. Not critical because this should not be a very frequent use case.
Prioritized changes The main goal of this issue is fixing a regression and removing hardcoded assumptions in the whole Entity API, hence this reduces fragility and provides a more stable API to work with.
Disruption This may require a few patches concerning the entity or language system to be rerolled. A minor API break is introduced by this issue: entity type providers will need to add a langcode key in the entity type definition, if the entity type is language aware.
CommentFileSizeAuthor
#155 2143729-language-entity-key-155.patch85.35 KBplach
#155 2143729-language-entity-key-155.interdiff.txt2.07 KBplach
#151 2143729-language-entity-key-151.patch84.75 KBplach
#151 2143729-language-entity-key-151.interdiff.txt3.49 KBplach
#150 2143729-language-entity-key-150.patch81.27 KBplach
#150 2143729-language-entity-key-150.interdiff.txt1.95 KBplach
#149 2143729-language-entity-key-149.patch83.21 KBplach
#149 2143729-language-entity-key-149.interdiff.txt9.9 KBplach
#145 2143729-language-entity-key-145.interdiff-131.txt37.18 KBplach
#145 2143729-language-entity-key-145.interdiff.txt54.78 KBplach
#145 2143729-language-entity-key-145.patch77.24 KBplach
#140 2143729-language-entity-key-140.patch105.71 KBplach
#140 2143729-language-entity-key-140.interdiff.txt65.63 KBplach
#138 2143729-language-entity-key-138.patch40.6 KBplach
#131 2143729-language-entity-key-131.interdiff.txt2.73 KBplach
#131 2143729-language-entity-key-131.patch41.17 KBplach
#129 2143729-language-entity-key-129-interdiff.txt2.11 KBBerdir
#129 2143729-language-entity-key-129.patch40.3 KBBerdir
#119 2143729-language-entity-key-119.patch38.75 KBplach
#117 2143729-language-entity-key-116.interdiff.txt1.17 KBplach
#116 ct-metadata_fields-1916790-112.interdiff.txt611 bytesplach
#116 2143729-language-entity-key-116.patch38.75 KBplach
#114 2143729-language-entity-key-114.patch37.83 KBplach
#114 2143729-language-entity-key-114.interdiff.txt1.98 KBplach
#112 2143729-language-entity-key-112.patch36.86 KBplach
#112 2143729-language-entity-key-112.interdiff.txt3.73 KBplach
#109 2143729-language-entity-key-109.patch33.13 KBplach
#109 2143729-language-entity-key-109.interdiff.txt1017 bytesplach
#108 2143729-language-entity-key-108.patch33.34 KBplach
#108 2143729-language-entity-key-108.interdiff.txt13.57 KBplach
#106 2143729-language-entity-key-106.patch21.34 KBandypost
#103 2143729-103-language-entity-key.patch30.18 KBBerdir
#100 2143729-100-language-entity-key-interdiff.txt405 bytesBerdir
#100 2143729-100-language-entity-key.patch30.76 KBBerdir
#98 2143729-98-language-entity-key-interdiff.txt814 bytesBerdir
#98 2143729-98-language-entity-key.patch30.76 KBBerdir
#88 2143729-88-language-entity-key.patch33.18 KBtstoeckler
#88 2143729-86-88-interdiff.txt745 byteststoeckler
#86 2143729-86-entity-language-key.patch33.14 KBtstoeckler
#86 2143729-83-86-interdiff.txt5.91 KBtstoeckler
#83 2143729-83-language-entity-key.patch32.86 KBtstoeckler
#83 2143729-78-83-interdiff.txt5.26 KBtstoeckler
#78 2143729-78-language-entity-key.patch33.71 KBjsbalsera
#73 interdiff.txt1.08 KBjsbalsera
#73 2143729-73-language-entity-key.patch33.57 KBjsbalsera
#71 2143729-71-language-entity-key.patch33.49 KBtstoeckler
#71 2143729-69-71-interdiff.txt850 byteststoeckler
#69 2143729-69-language-entity-key.patch33.49 KBtstoeckler
#69 2143729-61-69-interdiff.txt594 byteststoeckler
#65 2143729-64-language-entity-key.patch30.47 KBtstoeckler
#64 2183231-127-entity-schema-auto.patch241.07 KBtstoeckler
#61 interdiff.txt1.71 KBjsbalsera
#61 2143729-61-entity-language-key.patch33.49 KBjsbalsera
#58 2143729-58-entity-language-key.patch33.21 KBjsbalsera
#58 interdiff.txt9.5 KBjsbalsera
#56 interdiff.txt1.22 KBjsbalsera
#56 2143729-56-entity-language-key.patch30.92 KBjsbalsera
#54 interdiff.txt10.36 KBjsbalsera
#54 2143729-54-entity-language-key.patch29.7 KBjsbalsera
#52 2143729-52-entity-language-key.patch30.3 KBjsbalsera
#52 interdiff.txt4 KBjsbalsera
#45 2143729-45-entity-language-key.patch31.25 KBmauzeh
#40 2143729-40-entity-language-key.patch30.52 KBtstoeckler
#40 2143729-38-40-interdiff.txt2.28 KBtstoeckler
#38 2143729-38-entity-language-key.patch30.22 KBtstoeckler
#38 2143729-36-38-interdiff-2.txt931 byteststoeckler
#38 2143729-36-38-interdiff.txt3.67 KBtstoeckler
#36 2143729-36-entity-language-key.patch31.55 KBtstoeckler
#36 2143729-34-36-interdiff.txt689 byteststoeckler
#34 2143729-34-entity-language-key.patch31.55 KBtstoeckler
#34 2143729-33-34-interdiff.txt13.55 KBtstoeckler
#33 2143729-33-language-entity-key.patch25.74 KBtstoeckler
#32 2143729-33-language-entity-key.patch29.68 KBtstoeckler
#31 2143729-30-language-entity-key.patch34.86 KBtstoeckler
#31 2143729-28-30-interdiff.txt9.37 KBtstoeckler
#29 2143729-29-entity-language-key.patch26.22 KBtstoeckler
#27 2143729-27-entity-language-key.patch19.05 KBtstoeckler
#27 2143729-24-27-interdiff-1.txt719.46 KBtstoeckler
#25 2143729-22-24-interdiff.txt1.17 KBtstoeckler
#24 2143729-24-entity-language-key.patch14.78 KBtstoeckler
#22 2143729-22-entity-language-key.patch14.24 KBtstoeckler
#22 2143729-20-22-interdiff.txt4.05 KBtstoeckler
#20 2143729-20-entity-language-key.patch12.15 KBtstoeckler
#20 2143729-17-20-interdiff.txt9.48 KBtstoeckler
#17 2143729-17-language-entity-key.patch13.02 KBtstoeckler
#17 2143729-14-17-interdiff.txt1.02 KBtstoeckler
#14 2143729-14.patch13.14 KBtstoeckler
#14 2143729-13-14-interdiff.txt843 byteststoeckler
#13 2143729-13.patch226.13 KBtstoeckler
#13 2143729-7-13-interdiff.txt5.59 KBtstoeckler
#10 2143729-9-language-entity-key.patch534 byteststoeckler
#10 2143729-7-9-interdiff.txt5.75 KBtstoeckler
#7 2143729.patch8.15 KBdamiankloip
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

Title: Entity definition miss a language key » Entity definitions miss a language key
fago’s picture

Title: Entity definitions miss a language key » Entity definitions miss a language entity key
Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-content

Add some D8MI tags.

Gábor Hojtsy’s picture

Title: Entity definitions miss a language entity key » Regression: Entity definitions miss a language entity key
Issue tags: +Regression

Also this is a regression based on #1495648: Introduce entity language support in Drupal 7.

damiankloip’s picture

Assigned: Unassigned » damiankloip

Working on this now.

Berdir’s picture

FYI: There are a few hardcoded assumptions in ContentEntityBase on langcode. And a possible conflict if it would be named language.

damiankloip’s picture

Assigned: damiankloip » Unassigned
FileSize
8.15 KB

Made a start on this the other day, but no time since then.

Here is that start.

tstoeckler’s picture

Assigned: Unassigned » tstoeckler
Issue tags: +D8MA, +SprintWeekend2014
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php
@@ -274,7 +267,7 @@ public function create(array $values) {
+    $values += array($this->langcodeKey => language_default()->id);
...
-    $values += array('langcode' => language_default()->id);

The config system hardcodes 'langcode' everywhere including outside of config entities, i.e. in normal config. It should enforce that the langcode key is 'langcode', otherwise all hell breaks loose.

Will take a stab at this now.

damiankloip’s picture

Then we might as well not bother with this patch?

tstoeckler’s picture

Status: Active » Needs review
FileSize
5.75 KB
534 bytes

Here we go. This removes most of the references to 'langcode' in the entity system.

There was some code in the EntityManager to check that entity key fields, such as bundle, langcode, etc. are not translatable, but that was broken. Fixed that because I had to touch that code anyway.

Let's see how badly this breaks.

damiankloip’s picture

The new patch that you have posted is nothing like #7 and is nothing like the issue title. At all. Makes it basically pointless.

tstoeckler’s picture

Re #8: Well we should still allow this for content entities, only for config entities it doesn't make any sense as far as I'm aware.

Re #10: Sorry, the patch is (obviously?) completely wrong. The interdiff is (almost) correct vs. what I had on my system. Can you elaborate on why the interdiff is problematic/wrong?

tstoeckler’s picture

Assigned: tstoeckler » Unassigned
FileSize
5.59 KB
226.13 KB

So here's the patch and interdiff I actually wanted to post.

Also unassigning for now. I'd like to continue on this, but I'd like some feedback from @damiankloip first, because I don't understand his reservations yet.

I'll try to explain my position a little better:
The language support of the configuration system is built in a way that it checks the 'langcode' property of a config item and the it checks for possible langcode.*. overrides. Since the low-level config system doesn't differentiate between simple config files and config entities this rules applies to all configuration. We *could* change the config system to account for $entity_type->getKey('langcode') but that would introduce an inconsistency between config entities and regular config files as the latter wouldn't have such a facility to configure this. Therefore I think we should not allow this for config entities.

tstoeckler’s picture

FileSize
843 bytes
13.14 KB

Damn it, forgot to merge 8.x. Also removed obsolete @todo, see interdiff. Sorry for the noise.

The last submitted patch, 13: 2143729-13.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 14: 2143729-14.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
1.02 KB
13.02 KB

Let's see, this is re-rolled for the entityInfo() -> entityType() change.

Also, I was a bit too ambitious with EntityManager::getFieldDefinitions() change. We should exclude entity keys such as 'label' or 'weight' from that code path.

Status: Needs review » Needs work

The last submitted patch, 17: 2143729-17-language-entity-key.patch, failed testing.

Gábor Hojtsy’s picture

Hm, "Failed to run tests: failed to login to test site." is not very good :O

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
9.48 KB
12.15 KB

Here we go. This adapts the approach based on the 'uuid' key one.

Status: Needs review » Needs work

The last submitted patch, 20: 2143729-20-entity-language-key.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
4.05 KB
14.24 KB

So as we no longer have a default value for the 'language' entity key (as the key doesn't make sense for non-translatable entities) we need to provide it for the respective entities. This is consistent with the 'revision' key, for example.

Also PhpStorm failed to rename one of the instances of $this->langcode_key.

Status: Needs review » Needs work

The last submitted patch, 22: 2143729-22-entity-language-key.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
14.78 KB
+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -262,7 +255,8 @@ public function access($operation = 'view', AccountInterface $account = NULL) {
+    $langcode = $this->{$this->entityInfo()->getKey('language')};

This doesn't exist anymore.

Fixing that and also providing a default $langcode in ConfigEntityBase.

Let's see how much that fixes.

tstoeckler’s picture

FileSize
1.17 KB

Huh, I'm certain I attached the interdiff.

Status: Needs review » Needs work

The last submitted patch, 24: 2143729-24-entity-language-key.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
719.46 KB
19.05 KB

The aggregator hunks got list in the interdiff, because I merged 8.x in between, sorry. They're just added hunks, though.

The first hunk of the interdiff was a merge error with the config-entity uuid-key patch.

The second one fixes aggregator module and took me quite a while to figure out. Let's see how many fails. This should at least install.

Status: Needs review » Needs work

The last submitted patch, 27: 2143729-27-entity-language-key.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
26.22 KB

Here's another update. Sorry, no interdiff as I had to merge 8.x a bunch of times. The most important change is that I added a 'language' => 'langcode' key to Comment. I then dug around for a *very* long time because content_translation was severely broken with this patch. I couldn't really figure out why but I noticed it had something to do with ContentEntityBase::init() so this new patch includes #2095919: Kill ContentEntityBase::init() . This fixes the problem at least in local testing. Let's see where we're at.

Status: Needs review » Needs work

The last submitted patch, 29: 2143729-29-entity-language-key.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
9.37 KB
34.86 KB

So unless I inadvertently broke some other tests somewhere with these fixes, this should actually be green. Yay!!

tstoeckler’s picture

Assigned: Unassigned » tstoeckler
FileSize
29.68 KB

Yay, that took way longer than it should have, but green is green.

Now extracting the various (hopefully) unrelated changes that I made in my many futile attempts to make tests pass:

No interdiff, as I literally removed the relevant hunks manually from the patch file.

tstoeckler’s picture

FileSize
25.74 KB

So now that we're green, trying to extract #2095919: Kill ContentEntityBase::init() again to see if it's actually a hard dependency here.

tstoeckler’s picture

FileSize
13.55 KB
31.55 KB

Hmm... that's weird. But all the better.

This one implements the @todo introduced in the patches above of adding a 'default_language' key. Hardcoding the "default_" prefix would really be weird and break our standard of providing keys for that. And it doesn't make sense to do that in another follow-up.

I also brought some consistency into the definition of the entity keys. That slightly increases the patch size - sorry - but the previous patch made the situation even worse.

In case this is green this is now ready for some (final?) reviews!! :-)

Status: Needs review » Needs work

The last submitted patch, 34: 2143729-34-entity-language-key.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
689 bytes
31.55 KB

Aww, annotations are.... ..hard?!

Berdir’s picture

Bah, I just lost my review, so just a few quick comments.

- Content entity handling should get easier with #2182239: Improve ContentEntityBase::id() for better DX. This will also conflict with #2134857: PHPUnit test the entity base classes
- I don't understand why the order of entity keys needs to be changed here but I just looked at the last patch.
- Should the key be language or langcode, as we're actually assuming it is a langcode?
- I like langcode(). Why not make language() use langcode() instead of the other way round? Should be easier and faster?
- Why are the overrides in Feed necessary? having a language key should *not* imply that it can be translated IMHO, having entities with a language but without being able to translate it should be a supported functionality without those hacks? That's what isTranslatable() is for?

tstoeckler’s picture

FileSize
3.67 KB
931 bytes
30.22 KB

This needed a reroll.

Moved the langcode() method to the interface and removed the Feed and Item overrides. In return I added proper language code keys. You're probably right that it will work right. I originally added it because I thought only *translatable* entity types need a language key, but that's not the case.

Regarding langcode() calling language(): Because of the $language_manager->language() call in language(), language() and by extension langcode() currently always returns a valid language/langcode on the system. If langcode() would just return $this->{$this->languageKey} it might be an invalid langcode. (Not sure if that's really such a realistic use-case that we need to prevent, however).

Status: Needs review » Needs work

The last submitted patch, 38: 2143729-38-entity-language-key.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
2.28 KB
30.52 KB

So the field translatability checking got updated/fixed in the meantime. Let's see.

Regarding the name of the key, i.e. language vs. langcode:
I don't really care. We're currently very inconsistent anyway as we have 'id', but 'revision' (but not 'revision_id').

mauzeh’s picture

Assigned: tstoeckler » mauzeh

Starting work on this.

plach’s picture

Assigned: mauzeh » tstoeckler
Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityType.php
    @@ -52,8 +52,11 @@ public function getConfigPrefix() {
    +      'language' => 'langcode',
    

    Spoke with @Gabor we both agree that for consistency this should be called 'langcode' key, sorry :)

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -692,9 +700,14 @@ public function addTranslation($langcode, array $values = array()) {
    +
    

    Bogus empty line

  3. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -294,6 +295,13 @@ public function language() {
    +  public function langcode() {
    

    Do we really want to have both
    ::language() and ::langcode()? And then maybe replace all the occurences out there (thus adding a method call for each of those)? Are you sure we want to go this way? :)

    I am afraid this would not be a big DX win, and we would pay it in terms of performance and karma ;)

plach’s picture

Assigned: tstoeckler » Unassigned

@mauzeh:

Crosspost, sorry, I can't assign you back :(

mauzeh’s picture

Assigned: Unassigned » mauzeh
mauzeh’s picture

This should fix the failing test. Will ask @tstoeckler on the next steps.

mauzeh’s picture

Temporarily changing status to "needs review" to trigger the test bot.

mauzeh’s picture

Status: Needs work » Needs review
mauzeh’s picture

Status: Needs review » Needs work

The last submitted patch, 40: 2143729-40-entity-language-key.patch, failed testing.

The last submitted patch, 45: 2143729-45-entity-language-key.patch, failed testing.

jsbalsera’s picture

Assigned: mauzeh » jsbalsera

After talk with @tstoeckler I assign this to me and I will work on this.

jsbalsera’s picture

Status: Needs work » Needs review
FileSize
4 KB
30.3 KB

Rerolled and langcode function remove (also from the interface), and changed the language key to langcode.

Status: Needs review » Needs work

The last submitted patch, 52: 2143729-52-entity-language-key.patch, failed testing.

jsbalsera’s picture

Status: Needs work » Needs review
FileSize
29.7 KB
10.36 KB

There were a lot of languages to change for langcode still there. Anyway, this patch will fail, but I upload it as a WIP step.

Status: Needs review » Needs work

The last submitted patch, 54: 2143729-54-entity-language-key.patch, failed testing.

jsbalsera’s picture

Status: Needs work » Needs review
FileSize
30.92 KB
1.22 KB

Some tests needed to know about the language key...

tstoeckler’s picture

Status: Needs review » Needs work
  1. I noticed that we are missing documentation on the langcode and default_language in EntityTypeInterface::getKeys()
  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityType.php
    @@ -93,8 +93,11 @@ public function getConfigPrefix() {
    +    // Always add default 'uuid' and 'language' keys.
    

    This should be 'langcode' now.

  3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -80,6 +80,13 @@
       /**
    +   * The language entity key.
    +   *
    +   * @var string
    +   */
    +  protected $languageKey;
    

    I suppose we should call this $langcodeKey now. If you don't use PhpStorm or a similar IDE, please ping me and I will do that rename. I wouldn't suggest doing that manually.

  4. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
    @@ -244,7 +244,11 @@ public function isFieldDataCacheable() {
    +      'langcode' => '',
    +    );
    

    This is missing the 'default_language' key.

  5. +++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php
    @@ -948,6 +947,13 @@ public function language() {
       /**
        * {@inheritdoc}
        */
    +  public function langcode() {
    +    return $this->storage->langcode();
    +  }
    

    This should be removed again. (Sorry for telling you to add this, I was a bit confused in Szeged. :-/)

jsbalsera’s picture

Status: Needs work » Needs review
FileSize
9.5 KB
33.21 KB

Ok, changes made, and also EntityTypeInterface::getKeys() documentation is a little more polished now, like @tstoeckler and I talked in IRC.

Let's see if it still passes :-)

tstoeckler’s picture

Awesome, @jsabalsera, almost there. I have two super minor nitpicks left.

  1. +++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
    @@ -83,24 +83,32 @@ public function getClass();
    -   *   - label: The name of the property that contains the entity label. For
    +   *   - label: (optional) The name of the property that contains the entity label. For
    

    Sorry, but this is >80 chars now.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
    @@ -83,24 +83,32 @@ public function getClass();
    +
    

    This seems to be an accident?!

Status: Needs review » Needs work

The last submitted patch, 58: 2143729-58-entity-language-key.patch, failed testing.

jsbalsera’s picture

Status: Needs work » Needs review
FileSize
33.49 KB
1.71 KB

Status: Needs review » Needs work

The last submitted patch, 61: 2143729-61-entity-language-key.patch, failed testing.

tstoeckler’s picture

Issue tags: +sprint

Just noticed this is not on the D8MI focus board.

tstoeckler’s picture

tstoeckler’s picture

FileSize
30.47 KB

Oops, wrong patch :-)

Status: Needs review » Needs work

The last submitted patch, 65: 2143729-64-language-entity-key.patch, failed testing.

tstoeckler’s picture

I'm sorry, I completely messed this up. At this point someone should block my user account for trolling...
So not only did I upload a completely wrong patch in #64 I also uploaded a re-roll of #40 instead of #61. #61 doesn't even need a re-roll. So I literally spammed this issue with complete non-sense. I'm very sorry.

Will look into some of the fails now. Hope that's OK, even though you're still assigned, @jsbalsera.

tstoeckler’s picture

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
594 bytes
33.49 KB

Oh, sometimes PHP fatal messages are just not helpful at all. Let's see where this gets us.

Status: Needs review » Needs work

The last submitted patch, 69: 2143729-69-language-entity-key.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
850 bytes
33.49 KB

I declare this week trivial test fail week.

Status: Needs review » Needs work

The last submitted patch, 71: 2143729-71-language-entity-key.patch, failed testing.

jsbalsera’s picture

Status: Needs work » Needs review
FileSize
33.57 KB
1.08 KB
tstoeckler’s picture

Awesome, thanks @jsbalsera! Anyone want to RTBC now?

tstoeckler’s picture

tstoeckler’s picture

Status: Needs review » Needs work

The last submitted patch, 73: 2143729-73-language-entity-key.patch, failed testing.

jsbalsera’s picture

Status: Needs work » Needs review
FileSize
33.71 KB

Reroll

Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -463,7 +471,7 @@ protected function getTranslatedField($name, $langcode) {
       public function set($name, $value, $notify = TRUE) {
         // If default language or an entity key changes we need to react to that.
    -    $notify = $name == 'langcode' || in_array($name, $this->getEntityType()->getKeys());
    +    $notify = $name == $this->langcodeKey || in_array($name, $this->getEntityType()->getKeys());
         $this->get($name)->setValue($value, $notify);
    

    langcode is a key now, so we shouldn't need to check both things now?

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -571,7 +579,7 @@ public function language() {
       protected function setDefaultLangcode() {
         // Get the language code if the property exists.
    -    if ($this->hasField('langcode') && ($item = $this->get('langcode')) && isset($item->language)) {
    +    if ($this->hasField($this->langcodeKey) && ($item = $this->get($this->langcodeKey)) && isset($item->language)) {
           $this->defaultLangcode = $item->language->id;
         }
    

    I'd love to optimize this based on similar logic as the other entity keys now have in __construct(), which try avoid to create field objects if possible.

  3. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Entity/Shortcut.php
    @@ -35,8 +35,10 @@
      *     "uuid" = "uuid",
    + *     "langcode" = "langcode",
    + *     "default_language" = "default_langcode",
      *     "bundle" = "shortcut_set",
    

    Having to add this is IMHO a bit ugly, as default_langcode is more an implementation detail of the database storage rather than an API.

    Not what to do with this, did @plach review this already?

  4. +++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php
    @@ -938,7 +938,6 @@ public function toArray() {
         return $this->storage->toArray();
       }
     
    -
       /**
        * {@inheritdoc}
    

    Maybe there were other changes before in this file but now this is the only thing left, so you should probably undo the change, the patch has enough noise already :)

tstoeckler’s picture

3. I agree. I had thought about this in the meantime, too, but at hoped no one would bring this up, so we can do it in a follow-up :-) Should be fairly easy, though, to simply move this to a property on the storage. That way people need to override the storage, if they want to change the name of that database column. But that makes sense, IMO.

plach’s picture

I agree default_langcode does not belong to the entity type keys as well as the various table keys. Not sure if adding it here will make this easier to commit, but we should definitely plan at least one last iteration to remove all entity type properties that are just SQL storage implementation details.

Berdir’s picture

A key in the entity storage class sounds like a good idea to me.

tstoeckler’s picture

This should address #79.

@Berdir please see if I adressed #79.2 sufficiently.

tstoeckler’s picture

will fix the following once the testbot gets back:

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -737,8 +750,13 @@ public function addTranslation($langcode, array $values = array()) {
    +
    

    Unnecessary.

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
    @@ -52,6 +52,15 @@ class ContentEntityDatabaseStorage extends ContentEntityStorageBase {
    +   * Has the value FALSE if this entity does not have multilingual support.
    

    This is no longer true.

    Should be something like "This is only used if the entity has multilingual support."

    This will be properly cleaned up once we split the storages depending on translatability/revisionability

  3. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
    @@ -244,7 +244,12 @@ public function isFieldDataCacheable() {
    +      'default_language' => '',
    

    obsolete

  4. +++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
    @@ -83,24 +83,31 @@ public function getClass();
    +   *   - default_language: (optional) The name of the database column that
    +   *     stores whether a language variant belongs to the default language of
    +   *     the entity. This entry can be omitted if this entity type is not
    +   *     translatable.
    

    obsolete

  5. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityTypeTest.php
    @@ -82,9 +82,9 @@ public function testHasKey($entity_keys, $expected) {
    -      array(array(), array('revision' => '', 'bundle' => '')),
    -      array(array('id' => 'id'), array('id' => 'id', 'revision' => '', 'bundle' => '')),
    -      array(array('bundle' => 'bundle'), array('bundle' => 'bundle', 'revision' => '')),
    +      array(array(), array('revision' => '', 'bundle' => '', 'langcode' => '', 'default_language' => '')),
    +      array(array('id' => 'id'), array('id' => 'id', 'revision' => '', 'bundle' => '', 'langcode' => '', 'default_language' => '')),
    +      array(array('bundle' => 'bundle'), array('bundle' => 'bundle', 'revision' => '', 'langcode' => '', 'default_language' => '')),
    

    This should now be reverted (will fail tests).

Status: Needs review » Needs work

The last submitted patch, 83: 2143729-83-language-entity-key.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
5.91 KB
33.14 KB

Oops, setting the default value always was apparently not a good idea. Let's see.

Also adds a comment, fixes #84 and renames defaultLanguageKey to defaultLangcodeKey. I still think that defaultLanguageKey would actually be a better name, since this does *not* actually store a langcode but a boolean, but since it's consistently called 'default_langcode' so far in core, let's just stick with that.

Status: Needs review » Needs work

The last submitted patch, 86: 2143729-86-entity-language-key.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
745 bytes
33.18 KB

I should really know better than to inadvertantly change unrelated stuff by now.

I would have thought removing the hasField() check would be safe, but $this->langcodeKey can be an empty string sometimes, which this apparently is a safeguard for?!

Let's see.

sun’s picture

defaultLanguageKey would actually be a better name, since this does *not* actually store a langcode but a boolean

Sorry, but that states several problems in one go:

  1. It's neither a language nor a langcode.
  2. It does not denote/hold/return the langcode of the entity.
  3. It's a Boolean that denotes whether the entity has a langcode.

Boolean states should conform to the is* or has* norm:

  • If this is a method, hasLangcode().
  • If this is a declarative key/property, has_langcode.

Didn't look at anything else in this patch; just saw the comment in #86 flying by.

Status: Needs review » Needs work

The last submitted patch, 88: 2143729-88-language-entity-key.patch, failed testing.

tstoeckler’s picture

It's a Boolean that denotes whether the entity has a langcode.

That's not really true. It's a boolean whether or not the current language variant belongs to the default language or not. It's a denormalization of base_table.langcode = data_table.langcode. For revisions we add a similar property - isDefaultRevision - directly as a query expression but for languages we want to be able to retrieve data without joining on the base table, which is why we have that denormalization.

jsbalsera’s picture

Assigned: jsbalsera » Unassigned
vijaycs85’s picture

Assigned: Unassigned » vijaycs85
Issue tags: +MANCHESTER_2014_APRIL

Working on test failures in #88

vijaycs85’s picture

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

one of the failures is the profile image is not getting uploaded on test (works fine in manual testing). both message & file entity annotations missing 'langcode' entity key, adding them doesn't fix the test fails.

Unassigning for now if anyone want to have a look. I will work on this later.

vijaycs85’s picture

Status: Needs review » Needs work
Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 88: 2143729-88-language-entity-key.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
30.76 KB
814 bytes

Re-roll, added langcode to the file entity.

Status: Needs review » Needs work

The last submitted patch, 98: 2143729-98-language-entity-key.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
30.76 KB
405 bytes

Ups.

Status: Needs review » Needs work

The last submitted patch, 100: 2143729-100-language-entity-key.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: -sprint

It is a lie this is on a sprint, nobody is working on this :(

Berdir’s picture

Status: Needs work » Needs review
FileSize
30.18 KB

Yeah, yeah :)

Status: Needs review » Needs work

The last submitted patch, 103: 2143729-103-language-entity-key.patch, failed testing.

dawehner’s picture

Just a quick drive-by review.

  1. +++ b/core/modules/content_translation/content_translation.module
    @@ -250,8 +250,18 @@ function _content_translation_menu_strip_loaders($path) {
    +  $access = $access && empty($entity->getUntranslated()->language()->locked);
    +  $access = $access && \Drupal::languageManager()->isMultilingual();
    +  $access = $access && $entity->isTranslatable();
    +  $access = $access && (
    

    Did you considered using $access &= ...; ?

  2. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
    @@ -737,7 +740,7 @@ protected function setUpEntityWithFieldDefinition($custom_invoke_all = FALSE, $f
           ->method('getKeys')
    -      ->will($this->returnValue(array()));
    

    Tip: Just use ->willReturn($entity_keys); instead.

andypost’s picture

Re-roll, also cleaned-up the patch

Status: Needs review » Needs work

The last submitted patch, 106: 2143729-language-entity-key-106.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
13.57 KB
33.34 KB

Some additional clean up.

plach’s picture

FileSize
1017 bytes
33.13 KB

One more

The last submitted patch, 108: 2143729-language-entity-key-108.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 109: 2143729-language-entity-key-109.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
3.73 KB
36.86 KB

This should fix unit test failures.

Status: Needs review » Needs work

The last submitted patch, 112: 2143729-language-entity-key-112.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
1.98 KB
37.83 KB

This should fix some failures.

Status: Needs review » Needs work

The last submitted patch, 114: 2143729-language-entity-key-114.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
38.75 KB
611 bytes

Let's try this.

plach’s picture

Patch is correct, I picked the wrong interdiff.

Status: Needs review » Needs work

The last submitted patch, 116: 2143729-language-entity-key-116.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
38.75 KB

I've run a few failing tests locally and everything is green, let's try with a plain reroll.

Status: Needs review » Needs work

The last submitted patch, 119: 2143729-language-entity-key-119.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 119: 2143729-language-entity-key-119.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 119: 2143729-language-entity-key-119.patch, failed testing.

plach’s picture

Weird, I'm getting a different set of failures each time and I can't reproduce them locally...

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 119: 2143729-language-entity-key-119.patch, failed testing.

jerdnas’s picture

What a mess, I don't understand what's about 119patch? May I

Berdir’s picture

Status: Needs work » Needs review
FileSize
40.3 KB
2.11 KB

Not sure how this didn't fail for you :)

This should give a better error and fix at least some of them.

Status: Needs review » Needs work

The last submitted patch, 129: 2143729-language-entity-key-129.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
41.17 KB
2.73 KB

Well, I did not run all the failing tests locally, but the ones I tried were green...

This fixes a bug this patch uncovers and adjusts the related test accordingly: it's currently wrong because user 0 and 1 always get English language, since they are created before the Language module is enabled.

Gábor Hojtsy’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -473,10 +473,7 @@ public function language() {
    -      // If the entity references a language that is not or no longer available,
    -      // we return a mock language object to avoid disrupting the consuming
    -      // code.
    -      $language = isset($this->languages[$this->defaultLangcode]) ? $this->languages[$this->defaultLangcode] : new Language(array('id' => $this->defaultLangcode));
    +      $language = $this->languages[$this->defaultLangcode];
    

    So what will happen now if you delete a language and then still have entities in that language? Why is moving this code to setDefaultLangcode() ok? Do we have tests for attempting to ask for a language of an entity that is in a language no longer available?

  2. +++ b/core/modules/system/src/Tests/Installer/InstallerTranslationTest.php
    @@ -53,9 +53,11 @@ public function testInstaller() {
    -    $this->assertEqual($account->language()->getId(), 'de', 'Anonymous user is German.');
    +    $this->assertEqual($account->language()->getId(), 'en', 'Anonymous user is English.');
         $account = User::load(1);
    -    $this->assertEqual($account->language()->getId(), 'de', 'Administrator user is German.');
    +    $this->assertEqual($account->language()->getId(), 'en', 'Administrator user is English.');
    +    $account = $this->drupalCreateUser();
    +    $this->assertEqual($account->language()->getId(), 'de', 'New user is German.');
    

    VERY good catch :)

plach’s picture

So what will happen now if you delete a language and then still have entities in that language?

Code managing them will still be able to reference their language code, although it'll be unknown to the system. Maybe we should add also a fake language name, like: Unknown (it).

Why is moving this code to setDefaultLangcode() ok?

Because that's the earliest place where we know the default langcode, it's called during entity object construction.

Do we have tests for attempting to ask for a language of an entity that is in a language no longer available?

All the translated installer tests are actualy covering this case (that's how I discovered it), because English is not available but user 0 and 1 have English language. We could add explicit coverage though.

Berdir’s picture

Title: Regression: Entity definitions miss a language entity key » Entity definitions miss a language entity key
Gábor Hojtsy’s picture

Thanks for cleaning this up. As for the tests, its fine if there is already coverage IMHO :) Thanks!

Gábor Hojtsy’s picture

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

Reviewed the patch in its entirety (not just the last interdiff as above). I think it looks good. Also, it has the signoff of entity system maintainer Berdir.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

We have no test coverage of an entity type which does have langcode as a key.

plach’s picture

Status: Needs work » Needs review
FileSize
40.6 KB

Just a reroll for now.

YesCT’s picture

Issue summary: View changes

@alexpott Did you mean does *not* have 'langcode' as the key?

plach’s picture

This should improve test coverage. I could not fix all the failures yet.

alexpott’s picture

re #139 - Yes indeed - it is amazing how I can think one thing and type another.

Quick review of #140

+++ b/core/modules/system/src/Tests/Entity/EntityFieldTest.php
@@ -519,9 +527,10 @@ protected function doTestIterator($entity_type) {
-    foreach (entity_test_entity_types() as $entity_type) {
-      $this->doTestDataStructureInterfaces($entity_type);
-    }
+//    foreach (entity_test_entity_types() as $entity_type) {
+//      $this->doTestDataStructureInterfaces($entity_type);
+//    }
+    $this->doTestDataStructureInterfaces('entity_test_mul_langcode_key');
...
 

Huh?

Also why all the unrelated change of $entity_type to $entity_type_id?

plach’s picture

Huh?

This is still a work in progress, I was debugging only the new entity type.

Also why all the unrelated change of $entity_type to $entity_type_id?

I wll revert them, I needed to "free up" the $entity_type variable to store a definition, but it's adding too much noise.

The last submitted patch, 138: 2143729-language-entity-key-138.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 140: 2143729-language-entity-key-140.patch, failed testing.

plach’s picture

Here we go, this should be hopefully green.

I'm attaching also an interdiff with #131 for reviewers' convenience.

@alexpott:

Great point about test coverage! This allowed to spot at least two bugs in the previous version. Now we should have coverage for all the most common entity operations.

plach’s picture

Issue summary: View changes

Added beta evaluation.

plach’s picture

Gábor Hojtsy’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/src/Tests/Entity/EntityTranslationTest.php
    @@ -294,11 +296,25 @@ protected function _testMultilingualProperties($entity_type) {
    +   * Executes the Entity Translation API for the given entity type.
    +   *
    +   * @param string $entity_type
    +   *   The entity type to run the tests with.
    +   */
    +  protected function doTestEntityTranslationAPI($entity_type) {
    

    Minor: "API *tests*" (word missing)

    Major: this method does not seem to be called neither in this interdiff nor the patch applied to core

  2. +++ b/core/modules/system/src/Tests/Entity/EntityTranslationTest.php
    @@ -460,6 +476,19 @@ function testEntityTranslationAPI() {
    +  protected function doTestLanguageFallback($entity_type) {
    
    @@ -574,7 +602,20 @@ function testFieldDefinitions() {
    +  protected function doTestLanguageChange($entity_type) {
    

    Major: these are not invoked either, just defined.

  3. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMulLangcodeKey.php
    @@ -0,0 +1,58 @@
    + * Defines the test entity class.
    

    Minor: incorrect phpdoc, should be more specific.

plach’s picture

Status: Needs work » Needs review
FileSize
9.9 KB
83.21 KB

Fixed #148.

plach’s picture

Rerolled after that #2230637: Create a Language field widget and the related formatter went in (yay!) and removed some outdated @todos. Let's see what the bot thinks.

plach’s picture

plach’s picture

Issue tags: -Needs tests
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, adds the missing test coverage, cleans up @todo comments referring to this issue :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -190,6 +199,12 @@ protected function typedDataManager() {
    +      // If the entity references a language that is not or no longer available,
    +      // we return a mock language object to avoid disrupting the consuming
    +      // code.
    +      if (!isset($this->languages[$this->defaultLangcode])) {
    +        $this->languages[$this->defaultLangcode] = new Language(array('id' => $this->defaultLangcode));
    +      }
    

    This seems odd that it is possible. Also how does it relate to this patch?

  2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
    @@ -877,11 +877,13 @@ protected function initializeDataTable(ContentEntityTypeInterface $entity_type)
         $id_key = $entity_type->getKey('id');
     
    +    if (!$entity_type->getKey('langcode')) {
    +      throw new \Exception(String::format('Missing langcode entity key for entity type "@entity_type_id"', array('@entity_type_id' => $entity_type->getKey('langcode'))));
    +    }
    

    Should we not throw a more specific exception? Also do we really need this - we assume we have an ID key.

  3. +++ b/core/modules/system/src/Tests/Entity/EntityFieldTest.php
    @@ -541,7 +547,9 @@ protected function doTestDataStructureInterfaces($entity_type) {
    -    $this->assertEqual($strings, $target_strings, format_string('%entity_type: All contained strings found.', array('%entity_type' => $entity_type)));
    +    asort($strings);
    +    asort($target_strings);
    +    $this->assertEqual(array_values($strings), array_values($target_strings), format_string('%entity_type: All contained strings found.', array('%entity_type' => $entity_type)));
    

    Is this change necessary?

  4. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
    @@ -710,11 +710,14 @@ public function testGetFieldDefinitionsProvider() {
    -  protected function setUpEntityWithFieldDefinition($custom_invoke_all = FALSE, $field_definition_id = 'id') {
    +  protected function setUpEntityWithFieldDefinition($custom_invoke_all = FALSE, $field_definition_id = 'id', $entity_keys = array()) {
    

    We have two calls to this method which already pass a third parameter

      public function testGetFieldStorageDefinitionsWithCaching() {
        $field_definition = $this->setUpEntityWithFieldDefinition(TRUE, 'id', 0);
    
      public function testGetFieldDefinitionsWithCaching() {
        $field_definition = $this->setUpEntityWithFieldDefinition(FALSE, 'id', 0);
    
plach’s picture

1: This uncovers a bug in the installer: when a language different form English is chosen, user 0 and 1 are still created with language 'en' because the Language module is not enabled yet; when it is eventually enabled, the English language is disabled in favor of the chosen one and things start breaking. This is now covered by the regular installer tests: I had to fix it to make them pass.
2: Good point, removed the exception.
3: Yep, the value of the language key affects the order in which strings are collected, so we need to sort arrays and remove keys to compare them properly.
4: Fixed

plach’s picture

Status: Needs work » Needs review

#154.1 can happen also if a language is disabled after being assigned to any content entity.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Green, back to RTBC

plach’s picture

Issue summary: View changes
webchick’s picture

Assigned: Unassigned » alexpott

Alex has been all over this one, so assigning to him.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed b778bfd and pushed to 8.0.x. Thanks!

  • alexpott committed b778bfd on 8.0.x
    Issue #2143729 by tstoeckler, plach, jsbalsera, Berdir, mauzeh,...
plach’s picture

Issue tags: -sprint

Yay, thanks!

Status: Fixed » Closed (fixed)

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