Add an EntityOwnerTrait to standardize the base field needed by EntityOwnerInterface

Could we instead simply handle this in \Drupal\Core\Entity\ContentEntityBase::baseFieldDefinitions()?

For a second I thought that we add the uid field in ContentEntityBase, but actually we don't. I am sorry. A new trait EntityOwnerTrait is fine then.

First pass at this. Not sure if we need new tests or if the existing coverage is enough, as this is just a refactor?

Somehow testbot tested Payment module instead of core?!

A silly copy-paste typo caused most of those errors.

So this appears to be running into two main problems:

1. The field definitions for uid across entities vary quite a bit; some are revisionable, some are translatable, others are not, and most have a default value callback except file.
2. I added an entity key "uid", but for some entities this changes the definition from NULL to NOT NULL, this appears to be a symptom of #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field

The two above problems cause the various update tests to fail because the entity definitions are no longer identical. Is it worth unifying the fields here so they all have the same definition, or is that out of scope?

> 1. The field definitions for uid across entities vary quite a bit; some are revisionable, some are translatable, others are not, and most have a default value callback except file.

That's true for many fields defined in the ContentEntity base class (for revisionable/translatable), we can easily set them dynamically. We can't unify at least revision/translatable, default value we might have to.

> 2. added an entity key "uid", but for some entities this changes the definition from NULL to NOT NULL, this appears to be a symptom of

Yes, that is a known issue. The issue you linked isn't the sympton, just a way to provide an API to deal with that. The real problem is that all entity types are automatically considered to be required in the storage, which is wrong but really hard to change (if we change it, then we are affecting all existing and custom entity keys in the opposite way).

+++ b/core/lib/Drupal/Core/Entity/EditorialContentEntityBase.php
@@ -22,6 +23,9 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
     $fields += static::revisionLogBaseFieldDefinitions($entity_type);
 
+    // Add the owner field.
+    $fields += static::ownerBaseFieldDefinitions($entity_type);
+
     // Add the published field.
     $fields += static::publishedBaseFieldDefinitions($entity_type);

I don't think we should do this. This automatically expands the existing base class with new functionality and will throw an exception when the key is not defined.

Instead, each entity type should explicitly use the trait.

Thanks, I also came to the same realisation just now. I've done some more rearranging and fixed some other issues, hopefully this is a bit more successful in the tests.

I don't understand the HAL+JSON and REST fails, unless it is something to do with fragile ordering in $patchProtectedFieldNames?

Not really sure why I need to change EntityReferenceItemTest to make it pass.

Looks good, only one little query:

+++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
@@ -500,6 +500,8 @@ public function setDefaultValue($value) {
+    // Setting a fixed value should override any default value callback.
+    unset($this->definition['default_value_callback']);

One could argue the opposite.

Any callback takes precedence over the literal value already, but maybe cleaning up the definition array is still worth doing?

    // Allow custom default values function.
    if ($callback = $this->getDefaultValueCallback()) {
      $value = call_user_func($callback, $entity, $this);
    }
    else {
      $value = $this->getDefaultValueLiteral();
    }

Node already has uid (for a performance optimization), I guess that's why we went with that.. We also introduced a new one for status/published, so agreed it makes sense here too. Luckily adding a second key for the same field doesn't result in a schema change.

When we introduced the EntityPublishedTrait we used the entity key 'published' even though node already used 'status', which means node now has 'status' and 'published', but newly publishable entity types like BlockContent only have 'published'. We could do the same with the 'owner' entity key.

I did wonder whether to use 'uid' or something like 'owner' but as Berdir says I reused 'uid' because it was already there. 'owner' does have more meaning though, so will work on that next.

Copy-paste error.

Thanks for working on this, great refactor for core but also a huge productivity boost for projects which spin out a lot of entity types. Review as follows.

1. +++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
   @@ -500,6 +500,8 @@ public function setDefaultValue($value) {
        $this->definition['default_value'] = $value;
   +    // Setting a fixed value should override any default value callback.
   +    unset($this->definition['default_value_callback']);
        return $this;
   

   I think #21 and #23 still need to be addressed for this change.

2. +++ b/core/modules/content_moderation/tests/src/Functional/Update/ContentModerationUpdateTest.php
   @@ -0,0 +1,43 @@
   +      __DIR__ . '/../../../../../system/tests/fixtures/update/drupal-8.4.0.bare.standard.php.gz',
   +      __DIR__ . '/../../../fixtures/update/drupal-8.4.0-content_moderation_installed.php',
   +      __DIR__ . '/../../../fixtures/update/drupal-8.default-cms-entity-id-2941736.php',
   

   We shouldn't need the last dump here. These were created to apply on top of each other cleanly (and optionally), so drupal-8.4.0-content_moderation_installed.php should be enough.

3. +++ b/core/modules/content_moderation/tests/src/Functional/Update/ContentModerationUpdateTest.php
   @@ -0,0 +1,43 @@
   +    // Run updates.
   +    $this->runUpdates();
   
   +++ b/core/modules/media/tests/src/Functional/Update/MediaUpdateTest.php
   @@ -53,4 +53,22 @@ public function testBundlePermission() {
   +    // Run updates.
   +    $this->runUpdates();
   

   Super nit/style: it's pretty clear what is happening on these lines without the comments.

4. +++ b/core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceItemTest.php
   @@ -542,6 +542,7 @@ public function testAutocreateValidation() {
        $file = File::create([
          'filename' => $filename,
          'status' => 0,
   +      'uid' => NULL,
        ]);
   

   Perhaps this indicates a BC break. Was the default value of 'uid' NULL/empty before and is now 0? I wonder if this is important because of access control implications.

5. +++ b/core/modules/user/src/EntityOwnerTrait.php
   @@ -0,0 +1,89 @@
   +        ->setDefaultValueCallback(static::class . '::getCurrentUserId'),
   

   I always found [static::class, 'getCurrentUserId'] a bit more elegant, but this is purely style.

6. +++ b/core/modules/user/src/EntityOwnerTrait.php
   @@ -0,0 +1,89 @@
   +   * Default value callback for 'uid' base field definition.
   

   I think the comment in the trait should read 'owner' instead of 'uid'.

Also, it's a bit of a shame we can't add this to EditorialContentEntityBase. Authors/owners seem quite essential for editorial related things, but I agree adding it for all existing extending entity types would be disruptive.

Hey @longwave, hope you don't mind me picking up some of the points in the review. Here is a summary of some of the changes in the interdiff:

1. +++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
   @@ -500,8 +500,6 @@ public function setDefaultValue($value) {
   -    // Setting a fixed value should override any default value callback.
   -    unset($this->definition['default_value_callback']);
   
   @@ -542,7 +542,6 @@ public function testAutocreateValidation() {
        $file = File::create([
          'filename' => $filename,
          'status' => 0,
   -      'uid' => NULL,
        ]);
   
   +++ b/core/modules/file/src/Entity/File.php
   @@ -214,9 +214,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   -    $fields['uid']
   -      ->setDescription(t('The user ID of the file.'))
   -      ->setDefaultValue(0);
   +    $fields['uid']->setDescription(t('The user ID of the file.'));
   
   @@ -256,4 +254,11 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function getDefaultEntityOwner() {
   +    return NULL;
   +  }
   

   I believe this resolves the feedback around the default value callback as well as the BC concerns. The crux of this issue was: BaseFieldDefinition has no unsetDefaultValueCallback method, so previously where File was defaulting to NULL, this patch wanted it owned by the current user. This rejiggs things by naming the default value callback getDefaultEntityOwner which lets file entity safely override that to NULL which is correct in this circumstance.

2. +++ b/core/modules/content_moderation/tests/src/Functional/Update/ContentModerationUpdateTest.php
   @@ -18,7 +18,6 @@ protected function setDatabaseDumpFiles() {
        $this->databaseDumpFiles = [
          __DIR__ . '/../../../../../system/tests/fixtures/update/drupal-8.4.0.bare.standard.php.gz',
          __DIR__ . '/../../../fixtures/update/drupal-8.4.0-content_moderation_installed.php',
   -      __DIR__ . '/../../../fixtures/update/drupal-8.default-cms-entity-id-2941736.php',
   

   This will fail a test right now, but will pass once #2960054: content_moderation_post_update_update_cms_default_revisions fails if content_moderation was enabled but no entity types were being moderated is in.

3. +++ b/core/modules/user/src/EntityOwnerTrait.php
   @@ -54,6 +54,7 @@ public function getOwnerId() {
      public function setOwnerId($uid) {
        $key = $this->getEntityType()->getKey('owner');
        $this->set($key, $uid);
   +    unset($this->translatableEntityKeys['owner']);
   
   @@ -72,17 +73,18 @@ public function getOwner() {
      public function setOwner(UserInterface $account) {
        $key = $this->getEntityType()->getKey('owner');
        $this->set($key, $account->id());
   +    unset($this->translatableEntityKeys['owner']);
   

   This fixes NodeOwnerTest. Entity keys seem to have their own cache on ContentEntityBase (translatableEntityKeys and entityKeys). getEntityKey looks up those values first, hence the stale data in the test case.

Hm, I forgot about the SqlContentEntityStorageSchema nuance that all entity keys have NOT NULL applied to their storage scheme. To maintain BC on the file entity, we need to ensure uid can continue to be null.

Looks like Comment needs the same default value treatment.

Last test fix. I think this is ready for review. I think if everyone is happy with getDefaultEntityOwner, we should probably keep the existing getCurrentUserId static methods as deprecated on the individual entity classes. Excluding them from the trait will ensure it doesn't propagate to other entity classes and in general getDefaultEntityOwner is named more appropriately and thus can be overridden for different purposes like in the case of Comment and File.

Thanks for the review!

1. Hm, maybe the return value of these should be "mixed". What does [NULL] mean in field api land? A FieldItemList with a single FieldItem with a NULL value? The trait could also probably not return an array, it doesn't add owner as a multi-value field, so not sure why arrays are involved at all.
2. Good catch fixed.
3. Agree.

Hm, still need to add the getCurrentUserId methods back.

Had a good look through (without reading backscroll properly) and the only thing I came up with is that we need to keep getCurrentUserId, set them as deprecated, and call getDefaultEntityOwner.

Reintroducing the getCurrentUserId methods and adding deprecations + tests. Also reverting an unrelated test change.

1. +++ b/core/modules/comment/src/Entity/Comment.php
   @@ -260,12 +263,9 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    
   -    $fields['uid'] = BaseFieldDefinition::create('entity_reference')
   -      ->setLabel(t('User ID'))
   +    $fields['uid']
          ->setDescription(t('The user ID of the comment author.'))
   -      ->setTranslatable(TRUE)
   -      ->setSetting('target_type', 'user')
   -      ->setDefaultValue(0);
   +      ->setTranslatable(TRUE);
   

   I'm wondering if we want to get rid of that description while we touch it, it's misleading at best anyway (if we ever display the widget then it will show there but it will *not* be the id, it will be an autocomplete widget).

   Also, can't we dynamically make it translatable/revsionable like other such default field definitions? we have the entity type.

2. +++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
   @@ -37,6 +37,7 @@
     *     "uid" = "uid",
   + *     "owner" = "uid",
   

   wondering if we want to have a follow-up 9.x issue to remove the duplicate uid keys that we have in some places? Their usages we could already remove.

3. +++ b/core/modules/media/tests/src/Kernel/MediaTest.php
   @@ -8,6 +8,7 @@
     *
     * @group media
   + * @group legacy
     */
    class MediaTest extends MediaKernelTestBase {
   

   it's just one method that's legacy but I guess we have to put it in on the class?

   How do we differentiate between real legacy classes that we want to fully remove and those where we just want to remove a method?

4. +++ b/core/modules/user/src/EntityOwnerTrait.php
   @@ -0,0 +1,91 @@
   +    unset($this->translatableEntityKeys['owner']);
   

   I would expect that this happens automatically through \Drupal\Core\Entity\ContentEntityBase::onChange() ? Node didn't have to do this..

5. +++ b/core/modules/user/src/EntityOwnerTrait.php
   @@ -0,0 +1,91 @@
   +  public function setOwner(UserInterface $account) {
   +    $key = $this->getEntityType()->getKey('owner');
   +    $this->set($key, $account->id());
   +    unset($this->translatableEntityKeys['owner']);
   +
   +    return $this;
   +  }
   

   if we set it through the id anyway, we could just call setOwnerId(). However, I think maybe we should do set($key, $account), which works fine and $account could in theory be a new entity that hasn't been saved yet. We didn't do this either for node, but it wouldn't hurt.

Thank you for the review @Berdir!

1.1. I think we should aim to keep the field definitions 1 to 1 for the scope of this issue and file follow ups for definition changes.
1.2. I'm not sure what the impact of making uid translatable is. Different translations of one comment can have different authors? That may or may not be something we want to enforce in the trait, it may turn out to be something which broadly speaking doesn't make sense. Do all other entity types with authors also do this?
2. Sounds like a great idea.
3. I'm not entirely sure. Maybe we should add a new LegacyMediaTest to ONLY test deprecated methods, so the rest of the tests class is still covered by the deprecation listener?
4. Hm, definitely need to look into why this was required to make the tests green then.
5. This makes sense to me. Possibly need another test asserting setOwner works with unsaved entities too?

1.1. Fair enough, I'd just really like those descriptions gone, but I see it's also still there on node...

1.2: Yes, they do, in fact there is even a uid field added by content_translation if it doesn't exist. If the entity type is translatable, the expecation is that this field is too. and if you don't like it, you can still override it. The trait is about providing the best-possible default. See langcode in \Drupal\Core\Entity\ContentEntityBase::baseFieldDefinitions(). In fact, I even wondered if the trait could have an optional argument for setting the form/view display settings too.

5. Yeah, not a big deal I guess, afailk nobody ever complained about this not working, but either we can avoid the code duplication or we keep it separate on purpose and then a test wouldn't hurt.

Opened #2961627: Improve entity owner base field definitions as a follow up for #48.1.

1.2: Great, we'll make owner translatable for any translatable entity types.
5. I'll check this out in some more detail shortly.

@longwave hope you don't mind me grabbing the assigned status.

3. Not *exactly* the same, but see #6.2 on #2961691: Change SYMFONY_DEPRECATIONS_HELPER back to strict, that just adds a comment, so when we'll go through @legacy tests to remove them, we'll remember to not remove the whole thing. I think that's fine here too?

I've logged #2961983: Allow entity keys to be deprecated for #48.2.

Re: #53, I didn't realise you could add the @legacy annotation to individual test methods. In our case, since we're adding a dedicated test method for our deprecated code, the whole thing will be safe to remove when we remove the deprecated code. So I don't think a comment is even necessary here. No other code inadvertently calls the deprecated methods.

Logging #2961986: The ContentEntityBase entity key cache is purged incorrectly when two keys exist for one field. for #48.4.

Uploading a progress patch which addresses:

• Making the field translatable (can't see an issue with this being translatable for non translatable entity types, so applying unconditionally).
• Moves the @legacy annotation to our dedicated legacy test methods.
• Removes the unsetting of the keys cache as per above. This will currently fail some tests, but should be fixed after the blocker is resolved.

This still leaves todo:

if we set it through the id anyway, we could just call setOwnerId(). However, I think maybe we should do set($key, $account), which works fine and $account could in theory be a new entity that hasn't been saved yet. We didn't do this either for node, but it wouldn't hurt.

Sorry for the comment spam, quite a bit going on with this issue :)

This addresses the last todo based on @Berdir's feedback. I think we should support setting an unsaved user entity with setOwner as suggested. We want the trait to be broadly useful, so I think it should support the same features as the field system and what you'd get with a standard $entity->set makes sense.

Also adding a combined patch with #2961986: The ContentEntityBase entity key cache is purged incorrectly when two keys exist for one field. and marking this issue as blocked.

Fixing tests. The owner on files are non-translatable by default.

+++ b/core/modules/file/src/Entity/File.php
@@ -214,7 +214,9 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
       ->setDescription(t('The file language code.'));
 
-    $fields['uid']->setDescription(t('The user ID of the file.'));
+    $fields['uid']
+      ->setTranslatable(FALSE)
+      ->setDescription(t('The user ID of the file.'));

Don't set it unconditionally on the trait. Just like the example I referenced, you need to do it *only* if $entity_type->isTranslatable(), then this isn't necessary.

I believe File entities are translatable, just not the uid field.

>>> print_r(\Drupal::entityTypeManager()->getDefinition('file')->isTranslatable());
=> true

I tested this and there didn't seem to be any consequences of setting a field to be translatable on a non translatable entity type, which led me to believe any additional logic would be unnecessary. Let me know if that isn't the case.

Nope, they are not :)

>>> print_r(\Drupal::entityTypeManager()->getDefinition('file')->isTranslatable());
=> true
>>> \Drupal::entityTypeManager()->getDefinition('file')->isTranslatable();
=> false

That true is the return value of print_r(), which "prints" false and then returns TRUE that it did that successfully :)

As discussed, this should be updated.

Good catch! Sorry about that confusion, total brain fart on my behalf. Lets see how this goes.

The fails in #59 prove we have implicit test coverage for this. If you think we need more explicit coverage, EntityTest already implements the interface and the blocker already gives us control over the entity keys, we could switch the translatability of the entity type and check the field. I do think that is a bit overkill though.

Blocker is in.

Nice work!

1. +++ b/core/modules/file/src/FileStorageSchema.php
   @@ -26,6 +26,11 @@ protected function getSharedTableFieldSchema(FieldStorageDefinitionInterface $st
        }
   +    // Entity keys automatically have not null assigned to TRUE, but for the
   +    // file entity, NULL is a valid value for uid.
   +    if ($field_name === 'uid') {
   +      $schema['fields']['uid']['not null'] = FALSE;
   +    }
   

   Nice work to remove potential breakage

2. +++ b/core/modules/media/src/Entity/Media.php
   @@ -492,10 +464,14 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
        return [\Drupal::currentUser()->id()];
   
   +++ b/core/modules/node/src/Entity/Node.php
   @@ -410,10 +381,14 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
        return [\Drupal::currentUser()->id()];
   
   +++ b/core/modules/user/src/EntityOwnerTrait.php
   @@ -0,0 +1,90 @@
   +  public static function getDefaultEntityOwner() {
   +    return [\Drupal::currentUser()->id()];
   +  }
   

   I'm curious whether this should call out to the "parent" method.

3. +++ b/core/modules/user/src/EntityOwnerTrait.php
   @@ -0,0 +1,90 @@
   +    return [
   +      $entity_type->getKey('owner') => BaseFieldDefinition::create('entity_reference')
   +        ->setLabel(new TranslatableMarkup('User ID'))
   +        ->setSetting('target_type', 'user')
   +        ->setTranslatable($entity_type->isTranslatable())
   +        ->setDefaultValueCallback(static::class . '::getDefaultEntityOwner'),
   +    ];
   

   Maybe a naive question: Is there a reason this is not display/form configurable by default?

Thank you for reviewing!

1. :D
2. Do you mean the deprecated getCurrentUserId methods should call out to getDefaultEntityOwner? If that's the case, I think the whole point of renaming it in the trait and allowing consumers to override it was the default owner !== the current user. They just happen to be the same thing in these cases.
3. Media and Node are configurable while File, Comment and ContentModerationState are not, so I think it really comes down to what offers the best DX. Personally I usually have the owner hidden from forms and display for most custom entity types I've written, so I'd opt to keep them hidden, but I don't feel that strongly about it.

Media and Node are configurable while File, Comment and ContentModerationState are not, so I think it really comes down to what offers the best DX. Personally I usually have the owner hidden from forms and display for most custom entity types I've written, so I'd opt to keep them hidden, but I don't feel that strongly about it.

I mean I get the point why it is hidden by default, but it this entire is display configurable thing seems to be some limitation for sitebuilders. We never know their particular usecases.

Perhaps the trait should encourage it, developers do have the choice of altering the field definition if they like anyway.

One case for leaving it off could be security. For all access handlers which use the owner to make decisions, could making this editable be an issue in those cases? For Node and Media I think the owner field is wrapped around an "administer this thing" permission, but we aren't providing that by default here.

I was wondering something similar in #50, however based on an argument passed to the field definition method. I don't think it should do it by default. One thing is that you can *not* mix defining it yourself and defining the widget, things will fatal due to unexpected form structures.

None of our existing traits/base classes define form/view display configuration so far.

I'm not sure I like the idea of adding extra params to ownerBaseFieldDefinitions. The entities in core are already making additional customisations to the field definition, why would some of those customisations be in the form of params to the helper and some be in the form of directly accessing the base field definition and making changes?

Yeah, it's not something we did so far, agreed.

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -323,6 +322,13 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+  /**
+   * {@inheritdoc}
+   */
+  public static function getDefaultEntityOwner() {
+    return 0;

This might be the current behavior, but I actually think this is a bug, e.g. in the context of REST. See also #2860259: Move the comment hostname default value to a default value callback, I thought there was an issue about comment too but I couldn't find it.

I see the argument of not changing any existing behavior, but also wondering if we shouldn't just fix it as we have to touch it again afterwards. But I guess it will require some test changes/cleanup, so more out of scope changes.

There's no way that the current behavior of the file entity is by design for example. e.g. file_save_data() does set it to the current user, as does \Drupal\comment\CommentForm::buildEntity(). It's really quite unfortunate that we have to add a bunch of code and tests for that code just to ensure that we remain the current broken behavior. But I guess that is the way.. a follow-up maybe?

A follow-up sounds reasonable because the issues in #77 should impact the trait in any way as far as I can tell. I think as far as BC goes, if we were to handle those issues down the track, we can comfortably update the return value of getDefaultEntityOwner if we're indeed changing the default owner.

Unassigning for the moment.

+++ b/core/modules/user/src/EntityOwnerTrait.php
@@ -0,0 +1,90 @@
+  public static function getDefaultEntityOwner() {
+    return [\Drupal::currentUser()->id()];
+  }

Just a minor point: It's not actually necessary to return an array here. It doesn't break anything, but we can return anything that set() accepts. So I think in terms of naming and clarity I think it would make more sense to rename it to ....OwnerId() and just return the ID directly without wrapping it in an array.

#80: If it supports everything that set() supports, doesn't it do more than simply accept and ID? You could also return an entity in there too, no?

As far as wrapping the IDs in an array is concerned, @amateescu requested that in #41 for consistency with the docblock. I suppose hinting array is the best we can do, when in reality the field system is way more flexible than that.

Filing follow ups for the investigate the feasibility of fixing comment/file to behave different with regards to the owner here:

#2975218: Update the default file entity owner to the current user
#2975217: Update the default comment entity owner to the current user

Beyond those follow ups I don't think there are any more points of feedback, unless I've missed something. Anyone interested in another review of this issue?

Reolling

I'm also not quite sure about the feedback from @tstoeckler, so I'll leave it to him to deploy on that.

The test results in both those follow-ups look pretty encouraging, I suppose I'm still secretly hoping that we could do that directly instead first keeping the column/default value as NULL and then changing it ;)

Re #81: Looking at #41 it seems that the complaint was just that the docs didn't match the documentation. So I don't think @amateescu would object to simplifying the method, as long as we update the docs, as well.

On the other hand, this is a very minor point so I don't feel strongly at all about it and we shouldn't hold anything up on it. I just think it's a way to both simplify the code and make it more readable.

Looked through the patch and it really does look great to me, nice work! One thing I noticed: We are converting all entity types directly here except for the newly introduced Workspace and EntityTest. Should we fix those as well, right off the bat? EntityTest could be annoying because we then have to add the owner key to about a billion entity types.

So in reality the docblock is "mixed" because of everything field API supports right? From there we can return an int to simplify the code, as well as indicate with a comment the range of values that are acceptable. I'd be happy to take that approach.

With regards to Workspace and EntityTest, I'd be happy to delegate those to follow-ups, simply to keep the scope of this issue manageable. I think the entity types that @longwave already converted prove the API sufficiently. Same preference with the follow-ups in #82.

I don't think we have to explicitly document everything Field API supports (i.e. @mixed). I think we just choose one that works and document what it is. I was just suggesting to use the simplest one possible.

Fine with the followups for the other entity types, makes sense.

Does that reduce some of the utility though? I know int is a simple primitive that gets the job done, but would it ever be valid or useful to do something more complex, like return ['entity' => User...] for example? Just don't want to force an int if it's possible the usefulness suffers.

I got confused, I thought the current patch was hinting array where it's actually mixed already. So addressing the feedback from @tstoeckler, we should return the simplest mixed thing we can, and that's just the user ID. Also fixing the docblock, there can only be one default value.

Also filed these:

#2975957: Convert the Workspace entity to use EntityOwnerTrait
#2975958: Convert EntityTest to use EntityOwnerTrait

If everyone is happy with the follow-ups I think this was the last bit of feedback.

It looks like #2347711: FieldItemlListInterface::processDefaultValue($default_value) is expected to massage polymorphic data ensured field values from callbacks were arrays (as expected by processDefaultValue) and then #2529034: Replace direct access to FieldConfigBase::default_value with methods removed it again. This impacts FieldConfigBase only and not BaseFieldDefinition, which explains why this only turns up when an over field is being translated: the field definition is saved as a BaseFieldOverride entity.

I've logged #2976244: The BaseFieldOverride entity fails to normalize default values into the "array keyed by delta" format in the same way BaseFieldDefinition does when a callback is specified for this bug, which will fix the fails in ##8.

+++ b/core/modules/user/src/EntityOwnerTrait.php
@@ -0,0 +1,90 @@
+        ->setDefaultValueCallback(static::class . '::getDefaultEntityOwner'),

It would be nice to get #2975503: allow FieldConfigInterface::setDefaultValueCallback() to accept a callback in service notation in first, so we don't have to add this pretty pointless wrapper method.

I'm loving the direction of that issue, but I'm also hesitant to add another blocker to this issue. Still waiting on #2976244: The BaseFieldOverride entity fails to normalize default values into the "array keyed by delta" format in the same way BaseFieldDefinition does when a callback is specified, so I suppose we can see how it progresses and take it from there.

Rerolling now the blocker is complete.

Context of fail in #2973791: Fix deprecation messages related to deprecated comment Action plugins.

Marking as @legacy as per: https://www.drupal.org/node/2985785

1. +++ b/core/modules/comment/tests/src/Functional/Hal/CommentHalJsonTestBase.php
   @@ -42,8 +42,8 @@
   -    'entity_id' => NULL,
   ...
   +    'entity_id' => NULL,
   
   +++ b/core/modules/comment/tests/src/Functional/Rest/CommentResourceTestBase.php
   @@ -31,9 +31,9 @@
   +    'uid' => "The 'administer comments' permission is required.",
   ...
   -    'uid' => "The 'administer comments' permission is required.",
   

   Unintentional change?

2. +++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php
   @@ -616,6 +616,16 @@ public function testRevisionDefaultState($entity_type_id) {
   +  /**
   +   * Tests the legacy method used as the default entity owner.
   +   *
   +   * @group legacy
   +   * @expectedDeprecation The ::getCurrentUserId method is deprecated in 8.6.x and will be removed before 9.0.0.
   +   */
   +  public function testGetCurrentUserId() {
   +    $this->assertEquals(['0'], ContentModerationState::getCurrentUserId());
   +  }
   +
   
   +++ b/core/modules/media/tests/src/Kernel/MediaTest.php
   @@ -34,4 +34,14 @@ public function testNameBaseField() {
   +  /**
   +   * Tests the legacy method used as the default entity owner.
   +   *
   +   * @group legacy
   +   * @expectedDeprecation The ::getCurrentUserId method is deprecated in 8.6.x and will be removed before 9.0.0.
   +   */
   +  public function testGetCurrentUserId() {
   +    $this->assertEquals(['1'], Media::getCurrentUserId());
   +  }
   +
   
   +++ b/core/modules/node/tests/src/Kernel/NodeOwnerTest.php
   @@ -75,4 +76,36 @@ public function testOwner() {
   +  /**
   +   * Tests the legacy method used as the default entity owner.
   +   *
   +   * @group legacy
   +   * @expectedDeprecation The ::getCurrentUserId method is deprecated in 8.6.x and will be removed before 9.0.0.
   +   */
   +  public function testGetCurrentUserId() {
   ...
   +  }
   

   Do we really need these tests?

3. +++ b/core/modules/file/src/FileStorageSchema.php
   @@ -26,6 +26,11 @@ protected function getSharedTableFieldSchema(FieldStorageDefinitionInterface $st
   +    // Entity keys automatically have not null assigned to TRUE, but for the
   +    // file entity, NULL is a valid value for uid.
   +    if ($field_name === 'uid') {
   +      $schema['fields']['uid']['not null'] = FALSE;
   +    }
   

   Don't we need a dedicated test for this, the update path and update path test?

1. Nope, this makes the test pass.
2. These methods aren't executed otherwise. Completely dead code seems risky enough to add a test for.
3. This doesn't have anything to do with update paths. It maintains the same schema before and after and was only changed because it was already covered by another test.

Ok then.

I wonder if we should be waiting for #2975503: allow FieldConfigInterface::setDefaultValueCallback() to accept a callback in service notation?

Unrelated failure.

RE #104: #2975503: allow FieldConfigInterface::setDefaultValueCallback() to accept a callback in service notation is a feature request and this is a task so I don't think we should be waiting for that issue.

Okay great. There is some utility in a method that is ready to override in a parent anyway, so I don't have any reservations about introducing this with ::getDefaultEntityOwner.

Adding review credits

We need a change record here, can't fault the patch

Draft CR added! Thanks for reviewing @larowlan!

Committed bea23d7 and pushed to 8.7.x. Thanks!

Published change record

Quick important followup spotted by @chr.fritsch - #2999306: Update numbering - quick follow-up to #2949964

Oops didn't mean to change the status.

FYI, the media update path is not working for me in a project: #3040746: Update functions that set owner entity key updates can fail on existing data with NULL values, reviews would be great to get this fixed asap.