Updated: Comment #0

Problem/Motivation

We need a consistent way to get/set the author of an entity. Some entities contain a uid field to represent the author but there is no consistency, eg the uid field on a user is not the author and obviously manipulating that value will cause issues.

Proposed resolution

Rather than checking for a hard-coded list of entity types that support a uid or guessing based on the presence or lack thereof of a uid property, we should add an EntityAuthor interface with getAuthor/setAuthor or getOwner/setOwner methods.

Remaining tasks

Decide on the method names - either getAuthor/setAuthor or getOwner/setOwner.

User interface changes

None

API changes

Just an additional api in the form of the interface

#1979260: Automatically populate the author default value with the current user See comment 10
#731724: Convert comment settings into a field to make them work with CMI and non-node entities/#1907960: Helper issue for "Comment field" (see comment 353 on the helper issue)
#2074191: [META] Add changed timestamp tracking to content entities

Files: 
CommentFileSizeAuthor
#113 Add_an_EntityOwnerInterface-2078387-113.patch70.32 KBInternetDevels
PASSED: [[SimpleTest]]: [MySQL] 63,688 pass(es).
[ View ]
#60 drupal_2078387_60.patch34.38 KBXano
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_2078387_60.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#51 interdiff.txt472 bytesXano
#51 drupal_2078387_51.patch34.43 KBXano
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_2078387_51.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#49 drupal_2078387_49.patch34.44 KBXano
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#49 interdiff.txt8.65 KBXano
#44 drupal_2078387_44.patch34.47 KBXano
PASSED: [[SimpleTest]]: [MySQL] 58,804 pass(es).
[ View ]
#39 drupal_2078387_39.patch34.94 KBXano
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_2078387_39.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#39 interdiff.txt455 bytesXano
#36 interdiff.txt2.36 KBandypost
#36 drupal8.entity-system.2078387-36.patch34.83 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 59,160 pass(es).
[ View ]
#35 interdiff.txt2.85 KBandypost
#35 drupal8.entity-system.2078387-35.patch34.13 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 59,267 pass(es).
[ View ]
#33 interdiff.txt791 bytesandypost
#33 drupal8.entity-system.2078387-33.patch33.23 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 58,754 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#32 interdiff.txt5.77 KBandypost
#32 drupal8.entity-system.2078387-32.patch33.49 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 55,430 pass(es), 378 fail(s), and 222 exception(s).
[ View ]
#28 entity-author-interface-2078387-28.patch28.05 KBsplatio
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entity-author-interface-2078387-28.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#28 interdiff.txt1.38 KBsplatio
#25 entity-author-interface-2078387-25.patch28.04 KBsplatio
FAILED: [[SimpleTest]]: [MySQL] 58,658 pass(es), 110 fail(s), and 25 exception(s).
[ View ]
#25 interdiff.txt2.14 KBsplatio
#22 2078387-22.patch26.09 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 59,148 pass(es).
[ View ]
#22 interdiff.txt1.03 KBamateescu
#8 author-interface-2078387-8.patch24.55 KBtwistor
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch author-interface-2078387-8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#8 interdiff.txt12.97 KBtwistor
#6 author-interface-2078387-6.patch11.58 KBtwistor
PASSED: [[SimpleTest]]: [MySQL] 58,081 pass(es).
[ View ]
#4 author-interface-2078387-4.patch2.95 KBtwistor
PASSED: [[SimpleTest]]: [MySQL] 58,079 pass(es).
[ View ]

Comments

I went with Author in the Node and File methods, because that's also what the Node UI uses.

So there should be 2 methods Author() and AuthorId()

Yes please. I've been dealing with this problem personally. The best that can be done at this point is guessing that the uid field is the author/owner field.

I would prefer Owner, since it's more generic, but don't really care.

There's even a security aspect to this. When performing entity operations on behalf of a user, during cron for example, there's not a decent way to find the owner.

Status:Active» Needs review
StatusFileSize
new2.95 KB
PASSED: [[SimpleTest]]: [MySQL] 58,079 pass(es).
[ View ]

Well, since I'm so gung-ho.

What about entity keys? It's out of scope for this patch, but both this and the EntityChangedInterface could become much more useful if we defined entity keys to go along with the interface.

Also, FileInterface uses Owner. Once that's decided, we'll consolidate.

StatusFileSize
new11.58 KB
PASSED: [[SimpleTest]]: [MySQL] 58,081 pass(es).
[ View ]

Here it is with FileInterface converted.

Issue tags:+API change

Hm, so that fact that I used getOwner() does make this an API change...

StatusFileSize
new12.97 KB
new24.55 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch author-interface-2078387-8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

And with Comment converted.

hat-trick!

RTBC + 1
Waiting for approval of the API change

Awesome and very helpful for #731724: Convert comment settings into a field to make them work with CMI and non-node entities so comment field could work cleanly with all entities

// @todo Use $entity->getAuthorId() after https://drupal.org/node/2078387
if ($entity->getPropertyDefinition('uid')) {
  $last_comment_uid = $entity->get('uid')->value;
}

to
if ($entity instanceof EntityAuthorInterface) {
  $last_comment_uid = $entity->getAuthorId();
}

It seems to me that we've gone down a road of building a one-to-one interface-to-property architecture. At this point, an interface ceases to be useful because it no longer describes a common API for a client to build against. If we atomized our interfaces down to a property level, we undermine the usefulness of these structures altogether.

Can an entity exist without a creator? If this is an "author" in the sense of a post, then perhaps what we're describing here is a PostInterface with author, publish date, etc. But if every entity has an author, then author should be part of the Entity interface.

There really is no logical end to this kind of interface tacking on:

+use Drupal\Core\Entity\EntityAuthorInterface;
use Drupal\Core\Entity\EntityChangedInterface;
use Drupal\Core\Entity\ContentEntityInterface;

The pattern it suggests will lead to Interface mosaicism.

AFAIK, none of the ConfigEntities have any concept of Author/Owner.

Here are the current classes implementing ContentEntity:
Comment
CustomBlock
Feed
File
Item
MenuLink
Node
Term

I still like the idea of a separate interface, but perhaps ContentEntityInterface should extend it? Or even have ContentEntityBase (which should exist but doesn't) implement it.

@jessebeach I couldn't disagree more strongly :)

Can an entity exist without a creator?

Yes. Entities can be created purely programatically and may not be persisted at all.

If this is an "author" in the sense of a post, then perhaps what we're describing here is a PostInterface with author, publish date, etc.

That's not a terrible idea, but it doesn't get us any more than this does. Is a Comment a Post? Post has implied meaning that doesn't transfer very well. It's hard enough deciding on Author, Creator, Owner, Originator...

The problem is that it's getting progressively harder to work with entities generically, and that's the whole point. We can't dump everything into base interfaces or we end up with tons of empty functions. Then you have to check the return value rather than checking the interface. Does NULL mean the author is missing, or does FALSE? Do we add hasAuthor() to EntityInterface?

It seems to me that we've gone down a road of building a one-to-one interface-to-property architecture

Well, no. We have changed, and maybe author. Nobody is suggesting that policy. These are coming from in-the-field use cases, nothing hypothetical. Interfaces should be as granular as makes sense so that we don't pigeon-hole implementations or have to come up with ugly workarounds. From an API perspective, this is a cleanup for not having to guess if uid means author. At this point, the large majority of entities do not have any kind of author. 75-80% of the time, you're dealing with a particular interface, like NodeInterface, and you couldn't care less about these because you know what nodes provide. But, when dealing with entities generically, these distinctions become important.

@tim.plunkett,

AFAIK, none of the ConfigEntities have any concept of Author/Owner.

Hmmm... But they could, maybe. That's not an easy sell.

Do we want to go and add a uid field to every entity that doesn't have one? Seems like you want to push the crap up from EntityInterface to ContentEntityInterface just to keep Config entities clean, but don't care where it lands :)

AFAIK, only nodes and comments have a uid at this point.

ContentEntityBase, I saw that. Often times, base classes are a sign that your interfaces aren't composable enough. Just as long as nobody is checking for ContentEntityBase, I don't care.

I really don't see having too many characters after "implements" in a class declaration as a valid argument.

I would actually prefer this to be a generic interface that has nothing to do with entities, but I'm not going to fight that.

All that said, we could scrap this and *just* add an entity key of the same meaning. We need to add one anyway for changed and if this gets in, then author. These extra interfaces are really just sugar for:

<?php
$info
= \Drupal::entityManager()->getDefinition($entity_type);
if (!empty(
$info['entity_keys']['author'])) {
 
$author_id = $entity->get($info['entity_keys']['author'])->value;
}
?>

All that said, we could scrap this and *just* add an entity key of the same meaning. We need to add one anyway for changed and if this gets in, then author.

twistor, the way you describe the interface, it makes me think that it might be better to treat author as a field of a predefined content type (through configuration), rather than a property of an entity definition. Certainly other will want to create content types with an author field and having it as field on custom content types and as a property on programmatically created entities, will be inconsistent. There's work going on to turn these properties into field-like things:

#1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title

#1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title is so that we can re-use widgets and formatters for base fields. Base fields being entity properties. It's about code re-use and consistency.

Every content type has an author property, it's intrinsic in the node. We can't remove that and make it use configurable because it's used for access control, and it's part of the NodeInterface.

This issue does not have any UI, or user-configurable consequences at all. It simply provides a consistent way for code to analyze whether the entity they are dealing with has an author. At this point, we don't have any concept of an Author filed. A user can create a user reference field, fine. But, an author has more meaning attached, usually access control, and is part of the entity definition, not its fields.

Ok, thank you for explaining, twistor.

No problem. This isn't easy at all, and it's always good to have your assumptions checked.

Issue tags:-API change

#8: author-interface-2078387-8.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+API change

The last submitted patch, author-interface-2078387-8.patch, failed testing.

Another use case for this:

\Drupal\entity_reference\Plugin\field\widget\AutocompleteWidgetBase::formElement():
// @todo: Use wrapper to get the user if exists or needed.
'#autocreate_uid' => isset($entity->uid) ? $entity->uid : $user->id(),

The code is obviously wrong ($entity->uid is now an object) and can check for this interface instead now.

Hah, interesting use case :)

Status:Needs work» Needs review
StatusFileSize
new1.03 KB
new26.09 KB
PASSED: [[SimpleTest]]: [MySQL] 59,148 pass(es).
[ View ]

Here's a reroll and that @todo fixed.

/me starts to whiste and walks in another direction if anyone mentions anything about tests.

Looks great IMHO, but we need an API change approval for File, which I think shouldn't be a problem as hardly anyone will already be using that.

But the entity_reference stuff is a bit weird now that I look closer, so we might want to ignore that for now. Looking at AutocompleteWidgetBase::createEntity(), there uid is hardcoded. Instead, we need to switch that to do an instanceof check afterwards and if existing, call setAuthorId(). And what's even more fun is that terms are the only core entity that currently support auto-create and they don't have a uid. So that code is currently dead :)

Status:Needs review» Needs work

Needs work for the createEntity() stuff. I'm fine with not adding tests for that because there's currently no target entity type that has an author and supports auto-create.

Let's add the interface to EntityTest too, it does have an author as well and could in a follow-up be used add test coverage for the auto-create-author functionality?

And last, issue summary needs a short update to explain the API change involved here.

Status:Needs work» Needs review
StatusFileSize
new2.14 KB
new28.04 KB
FAILED: [[SimpleTest]]: [MySQL] 58,658 pass(es), 110 fail(s), and 25 exception(s).
[ View ]

OK, so I think I have done what you described @Berdir.

Status:Needs review» Needs work

The last submitted patch, entity-author-interface-2078387-25.patch, failed testing.

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Entity/EntityTest.php
@@ -147,4 +148,26 @@ public static function baseFieldDefinitions($entity_type) {
+  public function getAuthorId() {
+    return $this->get('user_id')->value;
+  }

->target_id, not value.

  1. +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
    @@ -461,4 +461,26 @@ public function getChangedTime() {
    +  public function getAuthorId() {
    +    return $this->get('uid')->value;
    +  }

    Same here.

  2. +++ b/core/modules/file/lib/Drupal/file/Entity/File.php
    @@ -120,15 +120,23 @@ public function getChangedTime() {
    +  public function getAuthorId() {
    +    return $this->get('uid')->value;
    +  }

    And this :)

Status:Needs work» Needs review
StatusFileSize
new1.38 KB
new28.05 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entity-author-interface-2078387-28.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

The test fails look like hiccups to me so I have just replaced "value" with "target_id" :)

Status:Needs review» Needs work
Issue tags:-API change

The last submitted patch, entity-author-interface-2078387-28.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+API change

The last submitted patch, entity-author-interface-2078387-28.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new33.49 KB
FAILED: [[SimpleTest]]: [MySQL] 55,430 pass(es), 378 fail(s), and 222 exception(s).
[ View ]
new5.77 KB

re-roll + fixed new places

StatusFileSize
new33.23 KB
FAILED: [[SimpleTest]]: [MySQL] 58,754 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new791 bytes

Proper fix

Status:Needs review» Needs work

The last submitted patch, drupal8.entity-system.2078387-33.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new34.13 KB
PASSED: [[SimpleTest]]: [MySQL] 59,267 pass(es).
[ View ]
new2.85 KB

New {comment_entity_statistics} should always have fallback to current user also in case when no author is set.

PS: the issue help to find the bug

StatusFileSize
new34.83 KB
PASSED: [[SimpleTest]]: [MySQL] 59,160 pass(es).
[ View ]
new2.36 KB

The query should not executed if no fields exists, maybe better to file another issue

Status:Needs review» Reviewed & tested by the community

This looks ready to me, just need to get approval for the API change

+++ b/core/lib/Drupal/Core/Entity/EntityAuthorInterface.php
@@ -0,0 +1,42 @@
+interface EntityAuthorInterface {

Is there in any reason we do not have $entity->setAuthor($user); ?

StatusFileSize
new455 bytes
new34.94 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_2078387_39.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Seemed easy enough to do setAuthorId($user->id()), so I didn't bother adding another method that all entity types have to implement.

Issue tags:-API change

#39: drupal_2078387_39.patch queued for re-testing.

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

The last submitted patch, drupal_2078387_39.patch, failed testing.

Assigned:Unassigned» Xano

Working on re-roll.

Seemed easy enough to do setAuthorId($user->id()), so I didn't bother adding another method that all entity types have to implement.

I assume most entities will use entity reference fields, which make the addition of a setAuthor() method less problematic, as storage can simply be passed on to the field.

Status:Needs work» Needs review
StatusFileSize
new34.47 KB
PASSED: [[SimpleTest]]: [MySQL] 58,804 pass(es).
[ View ]

Re-roll.

#44: drupal_2078387_44.patch queued for re-testing.

Seemed easy enough to do setAuthorId($user->id()), so I didn't bother adding another method that all entity types have to implement.

Also, setAuthor() may make testing easier, since classes implementing EntityAuthorInterface can receive a user object directly instead of an ID (which would have required them to load the entity from storage, making unit tests harder or impossible).

Not sure how that would be easier. If you want to unit test a specific implementation of that interface, then this would just mean that you have to unit test both ;) And if you use it somewhere to test something else, then you have to mock getAuthor() anyway, content entities with all their internal objects are much more complicated to handle than just mock the method.

Assigned:Xano» Unassigned

StatusFileSize
new8.65 KB
new34.44 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

I moved the interface to the User module, since it depends on module-level code.

Status:Needs review» Needs work

The last submitted patch, drupal_2078387_49.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new34.43 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_2078387_51.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new472 bytes

Both comments and nodes can currently be created by anonymous users. drupal_anonymous_user(), however, returns \Drupal\Core\Session\AccountInterface and not \Drupal\user\UserInterface, so I'm not sure whether EntityAuthorInterface introduced here, should do so as well. NodeInterface currently already typehints \Drupal\user\UserInterface for getAuthor() which seems weird at least (if not a bug), so I'm not really sure. I'm also not aware of the current status of the "clean-up the weird anonymous user handling in Drupal" initiative, so...

Thoughts?

I think that's correct. Here, we very specifically talk about a user entity, only those can be an author IMHO.

Also, drupal_anonymous_user() should just die :)

I'm inclined to agree with Berdir, although I have to admit that at first it seemed unusual to me too that we are returning a user entity rather than an account. However, we do reference a user and this is not for access control purposes (the major use case of AccountInterface). I do wonder if and how we can support anonymous users (an entity can have no author, an anonymous author or a non-anonymous author), but maybe I'm just not familiar enough with that part of core.

We do have an explicit record for uid 0 that you can load, so that shouldn't be a problem.

no author => NULL
anon => uid = 0
non-anon => uid > 0

Ah, so that still exists. Thanks. Somehow I was under the impression that was removed or refactored for D8.

Issue tags:-API change

#51: drupal_2078387_51.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+API change

The last submitted patch, drupal_2078387_51.patch, failed testing.

Assigned:Unassigned» Xano
Issue tags:+Needs reroll

Assigned:Xano» Unassigned
Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new34.38 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_2078387_60.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-roll.

+++ b/core/modules/comment/comment.module
@@ -1017,18 +1018,22 @@ function comment_entity_insert(EntityInterface $entity) {
+      // Get the user ID from the entity if it's set, or default to the
...
+        // Default to current user when entity does not implements
+        // EntityAuthorInterface or author is not set.

1. Our coding standards require "it is" over "it's"

2. does not *implement* (without a trailing s)

More importantly, though, I think the two comments are duplicated, no?!

Actual code looks great.

60: drupal_2078387_60.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 60: drupal_2078387_60.patch, failed testing.

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new34.12 KB
PASSED: [[SimpleTest]]: [MySQL] 59,979 pass(es).
[ View ]

Re-roll and applied changes noted in #61

Status:Needs review» Reviewed & tested by the community

Reviewed the entire patch again and couldn't find anything to complain.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 64: drupal_2078387_64.patch, failed testing.

Status:Needs work» Needs review

64: drupal_2078387_64.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Agreed with RTBC, this was just set back because testbot had issues.

hm, why do we have $entity->getAuthor() and not $entity->setAuthor($account) ?

As others, I'm not sure on whether AccountInterface or UserInterface should be used here. Aren't we interested in the account with which the entity was created, actually?

Status:Reviewed & tested by the community» Needs work

Thinking about it more, I think using UserInterface here is fine as this interface is tied to Drupal entities anyway.

However, imo the non-existence of $entity->setAuthor() is a DX problem and should be fixed.

Status:Needs work» Needs review
StatusFileSize
new3.64 KB
new35.88 KB
PASSED: [[SimpleTest]]: [MySQL] 59,039 pass(es).
[ View ]

Adds ::setAuthor as requested and re-rolls against HEAD

Status:Needs review» Needs work

The last submitted patch, 71: entity-author-2078387..patch, failed testing.

Status:Needs work» Needs review

71: entity-author-2078387..patch queued for re-testing.

The failed test passes locally for me.

Status:Needs review» Needs work

The last submitted patch, 71: entity-author-2078387..patch, failed testing.

The last submitted patch, 71: entity-author-2078387..patch, failed testing.

Status:Needs work» Needs review

71: entity-author-2078387..patch queued for re-testing.

Status:Needs review» Needs work

+++ b/core/modules/user/lib/Drupal/user/EntityAuthorInterface.php
@@ -21,6 +21,17 @@
+   * Returns the entity author's user entity.

Copy-paste error: Gets and not Returns.

The setters also need to be tested at least to make sure they return the entity.

Status:Needs work» Needs review
StatusFileSize
new35.96 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/comment/lib/Drupal/comment/CommentAccessController.php.
[ View ]
new534 bytes

Re-roll and fixed the typo.

Not a big fan of setAuthor(). Unlike getAuthor(), which is helpful, setAuthor() is trivial to emulate with setAuthorId($user->id()) and it requires each implementation to provide that method and test coverage for it, as we don't use them :)

The last submitted patch, 78: entity-author-2078387-78.patch, failed testing.

StatusFileSize
new36.38 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_2078387_80.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-roll and fixed the syntax error.

Status:Needs review» Reviewed & tested by the community

I also see no reason in setAuthor() method, but while there's no other nitpicks RTBC again

Patch, looks good to me as well now.

Not a big fan of setAuthor(). Unlike getAuthor(), which is helpful, setAuthor() is trivial to emulate with setAuthorId($user->id()) and it requires each implementation to provide that method and test coverage for it, as we don't use them :)

Well, it's not used now as it did not exist before, but imho it should be.

$comment->setAuthor($account); is better readable than
$comment->setAuthorId($account->id());, and better hides away details. Moreover, IDEs should be able to offer auto-complete suggestions for $account, while the won't for $account->id(). Lastly, I think its good practice and that what people used to oop would expect.

80: drupal_2078387_80.patch queued for re-testing.

I'm not completely sure about this one.

For nodes, many times the person who actually authored the content is not the person that posted it, and to get around this sites end up adding a separate 'author' entity reference field (which may not point to a user entity either). The confusion between node uid and node_revision uid is an example of this issue in core itself.

This feels like adding semantics to the uid field which match core's assumptions of what it's used for, but aren't always there. On the other hand the 'owner' of an entity is a pretty baked in core concept and this feels useful to represent that.

Leaving RTBC but would be good to discuss more.

I was watching this patch for use in Ubercart, and there is a similar dichotomy there - orders have the necessary concept of an "owner" uid, but that may not be the same as the user that actually authored (created) the order entity; the author is of secondary importance.

The UI does use Author too, but that can be altered/changed/hidden easily. The schema says "The {users}.uid that owns this node; initially, this is the user that created it." for nodes, But comments say "The {users}.uid who authored the comment. If set to 0, this comment was created by an anonymous user.". And the base field description of node.uid says "The user ID of the node author.".

So whatever we chose, it won't be a 100% match for all entities.

And one answer to that could be that we shouldn't do it at, but I think there are a number of valid use cases, like this patch shows.

Yes, maybe owner is a more generic concept for this than author. Not sure...

Also note that Node already uses get/setAuthor() in HEAD, this just introduces a separate interface for it.

80: drupal_2078387_80.patch queued for re-testing.

it was passing 2 weeks ago with 59,844 pass(es). retesting to see if it's still green.

calling it Owner makes sense to me.
Should we check with more people (who?) before we change it here in now (and change the title of the issue)?

we might be taking the author methods out of #2028025: Expand CommentInterface to provide methods as it conflicts with this issue

I would not be against renaming author to owner. I think it more clearly communicates what this interface is about.

Status:Reviewed & tested by the community» Needs work

I wasn't really suggesting 'Owner' when I posted, but I do think it's probably better - if we're going to change that, should do it here I think.

Assigned:Unassigned» Xano

Working on it.

The last submitted patch, 80: drupal_2078387_80.patch, failed testing.

The last submitted patch, 80: drupal_2078387_80.patch, failed testing.

Assigned:Xano» Unassigned
Status:Needs work» Needs review
StatusFileSize
new71.86 KB
PASSED: [[SimpleTest]]: [MySQL] 59,929 pass(es).
[ View ]

Re-roll, plus renamed author to owner and added a ton of type hinting comments to visually verify the methods exist.

Title:Add an EntityAuthorInterfaceAdd an EntityOwnerInterface

I actually like moving away from "author" as it is not always fitting - owner should better.

  1. +++ b/core/modules/comment/comment.module
    @@ -1249,7 +1257,7 @@ function comment_preview(CommentInterface $comment, array &$form_state) {
    +      $comment->setOwnerId($account->id());

    Minor: Should use $comment->setOwner($account ?

  2. +++ b/core/modules/comment/comment.tokens.inc
    @@ -142,13 +143,13 @@ function comment_tokens($type, $tokens, array $data = array(), array $options =
    +          if ($comment->getOwnerId() != 0) {

    Nothing new by this patch, but I think this should either not compare to 0 or be a type-safe comparison?

  3. +++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
    @@ -313,13 +314,14 @@ public function buildEntity(array $form, array &$form_state) {
    +      $comment->setOwnerId($account->id());

    Minor: Should use $comment->setOwner($account ?

  4. +++ b/core/modules/user/lib/Drupal/user/EntityOwnerInterface.php
    @@ -0,0 +1,53 @@
    + */

    I guess that could use some documentation on what an owner usually is?

StatusFileSize
new2.31 KB
new72.23 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Addressed the feedback from #96.

Status:Needs review» Needs work

The last submitted patch, 97: drupal_2078387_97.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new863 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_2078387_99.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new720 bytes

80-character issue fixed.

The last submitted patch, 99: drupal_2078387_99.patch, failed testing.

StatusFileSize
new72.09 KB
PASSED: [[SimpleTest]]: [MySQL] 59,891 pass(es).
[ View ]

Nothing new by this patch, but I think this should either not compare to 0 or be a type-safe comparison?

On second thought I don't think this is necessary after all. We want to check if the comment has any authenticated owner, so we can use its email address. No owner and an anonymous owner both mean there is no user entity with an email address available, so whether the owner ID is NULL or 0 does not make a difference here.

yeah, so my point is that then it should not try to compare with 0 (i.e not use != 0) but just go with (!$comment->uid->value) as it makes clearer that NULL is covered as well?

What is the use of this interface if we are going to access typed data properties anyway? It's bad practice and it defeats the purpose.

Next to that, we don't have to cover NULL. All we need to do is cover any owner ID that is not an integer that is greater than zero, i.e. any case where no user entity ID is stored. Whether that is because there is no owner, or the owner is anonymous, does not matter.

Lalalalalala, I need some coffee.

StatusFileSize
new543 bytes
new72.1 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_2078387_105.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Reviewed & tested by the community

+++ b/core/modules/comment/lib/Drupal/comment/CommentAccessController.php
@@ -22,13 +22,14 @@ class CommentAccessController extends EntityAccessController {
+    /** @var \Drupal\Core\Entity\EntityInterface|\Drupal\user\EntityOwnerInterface $entity */
+++ b/core/modules/comment/comment.tokens.inc
--- a/core/modules/comment/lib/Drupal/comment/CommentAccessController.php
+++ b/core/modules/comment/lib/Drupal/comment/CommentAccessController.php

This is strange, I think if we're going to type-hint we should typehint CommentInterface.

That's very minor though, and not technically incorrect, so setting RTBC. Couldn't find anything else to complain about.

105: drupal_2078387_105.patch queued for re-testing.

105: drupal_2078387_105.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 105: drupal_2078387_105.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new72.08 KB
PASSED: [[SimpleTest]]: [MySQL] 63,334 pass(es).
[ View ]

Re-roll because of node.module.

Status:Needs review» Reviewed & tested by the community

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

error: patch failed: core/modules/comment/comment.module:1342
error: core/modules/comment/comment.module: patch does not apply
error: patch failed: core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.php:246
error: core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.php: patch does not apply
error: patch failed: core/modules/rdf/lib/Drupal/rdf/Tests/CommentAttributesTest.php:226
error: core/modules/rdf/lib/Drupal/rdf/Tests/CommentAttributesTest.php: patch does not apply
error: patch failed: core/modules/system/lib/Drupal/system/Tests/File/FileTestBase.php:37
error: core/modules/system/lib/Drupal/system/Tests/File/FileTestBase.php: patch does not apply

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new70.32 KB
PASSED: [[SimpleTest]]: [MySQL] 63,688 pass(es).
[ View ]

re-rolled

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

Title:Add an EntityOwnerInterfaceChange record: Add an EntityOwnerInterface
Priority:Normal» Major
Status:Fixed» Active
Issue tags:+Needs change rec, +Missing change record

This needs a short change notice, will write something tonight if I find the time.

Issue tags:-Needs change rec+Needs change record

Title:Change record: Add an EntityOwnerInterfaceAdd an EntityOwnerInterface
Status:Active» Fixed
Issue tags:-Needs change record, -Missing change record

This should cover it: https://drupal.org/node/2188299

Status:Fixed» Closed (fixed)

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