All content entities should inherit baseFieldDefinitions

1. +++ b/core/modules/aggregator/src/Entity/Item.php
   @@ -69,10 +65,6 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   -    $fields['langcode'] = BaseFieldDefinition::create('language')
   -      ->setLabel(t('Language code'))
   -      ->setDescription(t('The feed item language code.'));
   -
   

   This will now get a language form potentially, which is not necessarily bad. Just let's think us of the consequences for a bit.

2. +++ b/core/modules/contact/src/Entity/Message.php
   @@ -130,24 +130,7 @@ public function getPersonalRecipient() {
   -    $fields['contact_form'] = BaseFieldDefinition::create('entity_reference')
   -      ->setLabel(t('Form ID'))
   -      ->setDescription(t('The ID of the associated form.'))
   -      ->setSetting('target_type', 'contact_form')
   -      ->setRequired(TRUE);
   -
   

   This removal should cause an issue ...

Seems fair for me. I would like to RTBC it, but I'm not sure whether this causes no issues with entities that want to be updated, but it seems not (as we have a test for that).

For #5.2, I wonder if having the bundle field as read-only will cause any problems for #1997692: Create contact form block, but it's easy enough to change it back whenever we would need to.

1. MenuLinkContent, Shortcut and Term entity types have a 'langcode' field that we can remove now.

   And the same for their bundle field?

2. +++ b/core/modules/user/src/Entity/User.php
   @@ -425,21 +425,7 @@ public static function getAnonymousUser() {
   -    $fields['langcode'] = BaseFieldDefinition::create('language')
   -      ->setLabel(t('Language code'))
   -      ->setDescription(t('The user language code.'))
   -      ->setTranslatable(TRUE);
   

   I think we should remove the view and form display options for this field.

Nice, looks good to me now.

Third time's the charm :)

+++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
@@ -233,23 +233,12 @@ public static function preDelete(EntityStorageInterface $storage, array $entitie
+
+    $fields['bundle']
       ->setSetting('max_length', EntityTypeInterface::BUNDLE_MAX_LENGTH)
-      ->setSetting('is_ascii', TRUE)
-      ->setReadOnly(TRUE);
+      ->setSetting('is_ascii', TRUE);

Is there a reason to not have those two settings by default in the base class? At least for fieldable entity types, we definitely don't support non-ascii or longer bundles.

And if someone wants to do something crazy, they still can change it, but seems like a better default?

To get an answer to #15

Also will this result in some changes on the labels of those fields which in turn can show up in views and forms?

We discussed the ID and I think agreed that just ID is better in the original issue. So I think it's OK to stick with that. I think label changes are OK for minor updates.

Right, possible schema changes due to those settings is indeed a problem. I'd be OK with that being a follow-up but as far as I know, the only case in core that doesn't use entity types/reference for bundles beside menu are test entities. So as far as core is concerned, it wouldn't actually be a schema change for any real entity type.

Contrib is a bit a different story, but this shows an interesting problem anyway. Adding the baseFieldDefinitions() method was unproblematic, as it was completely opt-in. But making changes or additions there will immediately and automatically result in changes for them. Changes that they however have to manually update to in update functions. So when exactly will they do that? after 8.2.0 is released, with a dependency on that version?

We opened #2713545: [policy, no patch] Are changes to ContentEntityBase base field definitions API breaking to discuss this further. Lets avoid changes that could be API/schema changes here.

Hmm I'm not really convinced by changing the labels here either - this issue hasn't had any usability review for a start.

What about restoring the label overrides here, then a separate issue to use the default ones which can then be reviewed for the string/interface changes?

1. +++ b/core/modules/aggregator/src/Entity/Feed.php
   @@ -127,16 +127,14 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
   -      ->setDescription(t('The aggregator feed UUID.'))
   
   +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -144,35 +144,14 @@ public function delete() {
   -      ->setDescription(t('The custom block UUID.'))
   
   +++ b/core/modules/comment/src/Entity/Comment.php
   @@ -208,16 +208,14 @@ public function permalink() {
   -      ->setDescription(t('The comment UUID.'))
   -      ->setReadOnly(TRUE);
   
   +++ b/core/modules/contact/src/Entity/Message.php
   @@ -130,24 +130,11 @@ public function getPersonalRecipient() {
   -      ->setDescription(t('The message UUID.'))
   
   +++ b/core/modules/file/src/Entity/File.php
   @@ -214,19 +214,13 @@ public static function preDelete(EntityStorageInterface $storage, array $entitie
   -      ->setDescription(t('The file UUID.'))
   
   +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -233,23 +233,15 @@ public static function preDelete(EntityStorageInterface $storage, array $entitie
   -      ->setDescription(t('The content menu link UUID.'))
   
   +++ b/core/modules/shortcut/src/Entity/Shortcut.php
   @@ -105,22 +105,11 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
   -      ->setDescription(t('The UUID of the shortcut.'))
   
   +++ b/core/modules/taxonomy/src/Entity/Term.php
   @@ -101,33 +101,14 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
   -      ->setDescription(t('The term UUID.'))
   

   Is it okay to drop the UUID description?

2. +++ b/core/modules/aggregator/src/Entity/Item.php
   @@ -47,11 +47,14 @@ public function label() {
      public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   

   For minimal BC break I would vote to add a new issue to add the UUID field to Item but remove it for now explitly

I feel bad for even mentioning it! :(

+++ b/core/modules/contact/src/Entity/Message.php
@@ -130,24 +130,11 @@ public function getPersonalRecipient() {
-    $fields['uuid'] = BaseFieldDefinition::create('uuid')
...
-      ->setDescription(t('The message UUID.'))

That one is missing

No worries. Thank you for the quick patching

Committed d7b430a and pushed to 8.2.x. Thanks!