ContentEntityBase should provide field definitions for key fields

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -1117,6 +1117,52 @@ protected function getEntityKey($key) {
+    $bundle_entity_type_id = $entity_type->getBundleEntityType();
+    if ($bundle_entity_type_id && $entity_type->hasKey('bundle')) {
+      $fields[$entity_type->getKey('bundle')] = BaseFieldDefinition::create('entity_reference')
+        ->setLabel(t('Type'))
+        ->setSetting('target_type', $bundle_entity_type_id)
+        ->setReadOnly(TRUE);
+    }

if there is no bundle entity type but a bundle key, could we provide a string field?

+1 for less boilerplate!

We don't really have a way of getting a label for the bundle field, so the best we can do is guess and tell children to change it if it's not precise.

We do :)

$entity_type->getBundleLabel()

Note that the EntityTest bundle field had setRequired(TRUE) on the type definition, do we need to do that in the base class?

Yes, definitely. The bundle key field should always be required.

Also, the base class is calling ->setRevisionable(TRUE) on the langcode, does that do anything if the parent entity is not revisionable?

No that's nonsensical and definitely an oversight/leftover/...

While my initial thought was this should better stay a trait, I guess it makes sense in the base class. It's a reasonable default and if you want to do it different you are free to not call the parent. so +1 on moving it into the base class.

This is probably all we need.

There we go, hopefully.

Let's see whether this works out.

There we go.

+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTest.php
@@ -109,6 +109,10 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
       ));
+    if (isset($fields['langcode'])) {
+      $fields['langcode']->setDisplayOptions('view', []);
+      $fields['langcode']->setDisplayOptions('form', []);
+    }

why not do this by default?

+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMulLangcodeKey.php
@@ -50,14 +50,4 @@
-  public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
-    $fields = parent::baseFieldDefinitions($entity_type);
-    $fields['custom_langcode_key'] = $fields['langcode'];
-    unset($fields['langcode']);
-    return $fields;

It's kind of funny how we now have a test that makes sure we can have a custom named langcode key that's automatically generated ;)

why not do this by default?

It breaks a couple of tests, but for sure, we could adapt the, as well.

+      $fields['langcode']->setDisplayOptions('view', []);
+      $fields['langcode']->setDisplayOptions('form', []);

Setting the options in that way does not actually enable the default view and form display determined by the default formatter and widget, so while it might fix tests it does not have any impact on the actual display. So I would like to avoid having those calls in the default implementation because I find them confusing. Not sure how others see that...

Maybe we can also adapt the tests to not require these calls in the first place?

Ok, if it doesn't actually do anything, then removing it and updating tests to not look for it seems like the best choice to me as well.

I agree

There we go

Agreed, RTBC.

.

Looks great. Committed/pushed to 8.1.x, thanks!

+++ b/core/modules/node/src/Entity/Node.php
@@ -325,41 +325,7 @@ public function setRevisionAuthorId($uid) {
-      ->setDescription(t('The node ID.'))
...
-      ->setDescription(t('The node UUID.'))
...
-      ->setDescription(t('The node revision ID.'))
...
-      ->setDescription(t('The node type.'))
...
-      ->setDescription(t('The node language code.'))

Is it a regression that node's base fields do not have a description anymore?

This issue is missing a change record. (We should add change records for noteworthy API additions.)

I'll write one for all of the 4 entity related ones: https://www.drupal.org/node/2709511

Added stuff to the change record. Feel free to improve it.