All content entities that extend ContentEntityBase should inherit fields from ContentEntityBase::baseFieldDefinitions().

This allows us to have more consistent code, more consistent entities, more consistent fields, and sets a better example.

The patch updates the following entity types:
- Feed
- Item
- BlockContent
- Comment
- Message
- File
- MenuLinkContent
- Shortcut
- Term
- User

Comments

timmillwood created an issue. See original summary.

Status: Needs review » Needs work

The last submitted patch, inherit-basefielddefinitions.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
12.51 KB

The Bundle field on MenuLinkContent was slightly different to the one set in ContentEntityBase, therefore this new patch still pulls everything from the parent (ContentEntityBase), but then overrides the Bundle field with the original.

Status: Needs review » Needs work

The last submitted patch, 3: all_content_entities-2707255-3.patch, failed testing.

dawehner’s picture

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

timmillwood’s picture

Status: Needs work » Needs review

Re-running tests, \Drupal\node\Tests\NodeTypeTranslationTest passes locally.

1) The Item entity doesn't have a form.
2) This is the bundle entity reference, the only changes will be it will lose it's description and become read only.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

amateescu’s picture

Status: Reviewed & tested by the community » Needs work

For #5.2, I wonder if having the bundle field as read-only will cause any problems for #1997692: Regression: category selector missing on site wide contact form, 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.

timmillwood’s picture

timmillwood’s picture

Issue summary: View changes
Status: Needs work » Needs review
amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Nice, looks good to me now.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: all_content_entities-2707255-9.patch, failed testing.

timmillwood’s picture

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

MenuLinkContent bundle needs max_length and is_ascii settings.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Third time's the charm :)

Berdir’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

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?

timmillwood’s picture

#15 - We could add this to the base class. That would however update the schema, which is something I was trying to avoid in this issue. Maybe we could do that in a follow up issue?

#16- Yes, unfortunately some of the labels will change, things like "Language Code" will change to "Language", or "{Entity type label} ID" will change to "ID".

For ID maybe we could update ContentEntityBase to use the following for the ID field label:
->setLabel(new TranslatableMarkup("@label ID", ['@label' => $entity_type->getLabel()]))

Berdir’s picture

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?

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

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.

catch’s picture

Status: Reviewed & tested by the community » Needs review

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?

timmillwood’s picture

Updated based on #20.

Added labels and descriptions back in where appropriate.

dawehner’s picture

  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

timmillwood’s picture

Currently I only kept the description if I kept an ID. I don't mind keeping description or not.

#2149851: Remove todo about GUID field on the 'aggregator_item' entity is the open issue for an Item UUID.

timmillwood’s picture

Added all the descriptions back in as suggested by @dawehner in #22.

dawehner’s picture

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

timmillwood’s picture

Damn!

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

No worries. Thank you for the quick patching

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed d7b430a and pushed to 8.2.x. Thanks!

  • alexpott committed d7b430a on 8.2.x
    Issue #2707255 by timmillwood, dawehner, amateescu, Berdir: All content...

Status: Fixed » Closed (fixed)

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