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

CommentFileSizeAuthor
#113 Add_an_EntityOwnerInterface-2078387-113.patch70.32 KBInternetDevels
#110 drupal_2078387_110.patch72.08 KBXano
#105 drupal_2078387_105.patch72.1 KBXano
#105 interdiff.txt543 bytesXano
#101 drupal_2078387_101.patch72.09 KBXano
#99 interdiff.txt720 bytesXano
#99 drupal_2078387_99.patch863 bytesXano
#97 drupal_2078387_97.patch72.23 KBXano
#97 interdiff.txt2.31 KBXano
#94 drupal_2078387_94.patch71.86 KBXano
#80 drupal_2078387_80.patch36.38 KBXano
#78 entity-author-2078387-78-interdiff.txt534 bytesBerdir
#78 entity-author-2078387-78.patch35.96 KBBerdir
#71 entity-author-2078387..patch35.88 KBlarowlan
#71 interdiff.txt3.64 KBlarowlan
#64 drupal_2078387_64.patch34.12 KBvladan.me
#60 drupal_2078387_60.patch34.38 KBXano
#51 interdiff.txt472 bytesXano
#51 drupal_2078387_51.patch34.43 KBXano
#49 drupal_2078387_49.patch34.44 KBXano
#49 interdiff.txt8.65 KBXano
#44 drupal_2078387_44.patch34.47 KBXano
#39 drupal_2078387_39.patch34.94 KBXano
#39 interdiff.txt455 bytesXano
#36 interdiff.txt2.36 KBandypost
#36 drupal8.entity-system.2078387-36.patch34.83 KBandypost
#35 interdiff.txt2.85 KBandypost
#35 drupal8.entity-system.2078387-35.patch34.13 KBandypost
#33 interdiff.txt791 bytesandypost
#33 drupal8.entity-system.2078387-33.patch33.23 KBandypost
#32 interdiff.txt5.77 KBandypost
#32 drupal8.entity-system.2078387-32.patch33.49 KBandypost
#28 entity-author-interface-2078387-28.patch28.05 KBmsmithcti
#28 interdiff.txt1.38 KBmsmithcti
#25 entity-author-interface-2078387-25.patch28.04 KBmsmithcti
#25 interdiff.txt2.14 KBmsmithcti
#22 2078387-22.patch26.09 KBamateescu
#22 interdiff.txt1.03 KBamateescu
#8 author-interface-2078387-8.patch24.55 KBtwistor
#8 interdiff.txt12.97 KBtwistor
#6 author-interface-2078387-6.patch11.58 KBtwistor
#4 author-interface-2078387-4.patch2.95 KBtwistor
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

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

andypost’s picture

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

twistor’s picture

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.

twistor’s picture

Status: Active » Needs review
FileSize
2.95 KB

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.

twistor’s picture

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

twistor’s picture

Here it is with FileInterface converted.

Berdir’s picture

Issue tags: +API change

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

twistor’s picture

And with Comment converted.

twistor’s picture

hat-trick!

andypost’s picture

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();
}
jessebeach’s picture

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.

tim.plunkett’s picture

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.

twistor’s picture

@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:

$info = \Drupal::entityManager()->getDefinition($entity_type);

if (!empty($info['entity_keys']['author'])) {
  $author_id = $entity->get($info['entity_keys']['author'])->value;
}

jessebeach’s picture

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

twistor’s picture

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

jessebeach’s picture

Ok, thank you for explaining, twistor.

twistor’s picture

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

jibran’s picture

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.

Berdir’s picture

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.

Wim Leers’s picture

Hah, interesting use case :)

amateescu’s picture

Status: Needs work » Needs review
FileSize
1.03 KB
26.09 KB

Here's a reroll and that @todo fixed.

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

Berdir’s picture

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

Berdir’s picture

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.

msmithcti’s picture

Status: Needs work » Needs review
FileSize
2.14 KB
28.04 KB

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.

Berdir’s picture

+++ 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 :)

msmithcti’s picture

Status: Needs work » Needs review
FileSize
1.38 KB
28.05 KB

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.

Berdir’s picture

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
33.49 KB
5.77 KB

re-roll + fixed new places

andypost’s picture

Proper fix

Status: Needs review » Needs work

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

andypost’s picture

Status: Needs work » Needs review
FileSize
34.13 KB
2.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

andypost’s picture

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

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

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

fago’s picture

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

Xano’s picture

FileSize
455 bytes
34.94 KB
Berdir’s picture

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

Berdir’s picture

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.

Xano’s picture

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.

Xano’s picture

Status: Needs work » Needs review
FileSize
34.47 KB

Re-roll.

andypost’s picture

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

Xano’s picture

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

Berdir’s picture

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.

Xano’s picture

Assigned: Xano » Unassigned
Xano’s picture

FileSize
8.65 KB
34.44 KB

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.

Xano’s picture

Status: Needs work » Needs review
FileSize
34.43 KB
472 bytes
tstoeckler’s picture

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?

Berdir’s picture

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

Xano’s picture

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.

Berdir’s picture

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

Xano’s picture

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

Xano’s picture

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.

Xano’s picture

Assigned: Unassigned » Xano
Issue tags: +Needs reroll
Xano’s picture

Assigned: Xano » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
34.38 KB

Re-roll.

tstoeckler’s picture

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

Berdir’s picture

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.

vladan.me’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
34.12 KB

Re-roll and applied changes noted in #61

tstoeckler’s picture

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.

vladan.me’s picture

Status: Needs work » Needs review

64: drupal_2078387_64.patch queued for re-testing.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

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

fago’s picture

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?

fago’s picture

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.

larowlan’s picture

Status: Needs work » Needs review
FileSize
3.64 KB
35.88 KB

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.

longwave’s picture

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.

longwave’s picture

Status: Needs work » Needs review

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

Xano’s picture

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
35.96 KB
534 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.

Xano’s picture

FileSize
36.38 KB

Re-roll and fixed the syntax error.

andypost’s picture

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

fago’s picture

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.

xjm’s picture

80: drupal_2078387_80.patch queued for re-testing.

catch’s picture

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.

longwave’s picture

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.

Berdir’s picture

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.

YesCT’s picture

80: drupal_2078387_80.patch queued for re-testing.

YesCT’s picture

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

Xano’s picture

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

catch’s picture

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.

Xano’s picture

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.

Xano’s picture

Assigned: Xano » Unassigned
Status: Needs work » Needs review
FileSize
71.86 KB

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

Xano’s picture

Title: Add an EntityAuthorInterface » Add an EntityOwnerInterface
fago’s picture

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?

Xano’s picture

FileSize
2.31 KB
72.23 KB

Addressed the feedback from #96.

Status: Needs review » Needs work

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

Xano’s picture

Status: Needs work » Needs review
FileSize
863 bytes
720 bytes

80-character issue fixed.

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

Xano’s picture

FileSize
72.09 KB
Xano’s picture

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.

fago’s picture

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?

Xano’s picture

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.

Xano’s picture

FileSize
543 bytes
72.1 KB
tstoeckler’s picture

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.

sun’s picture

105: drupal_2078387_105.patch queued for re-testing.

catch’s picture

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.

Xano’s picture

Status: Needs work » Needs review
FileSize
72.08 KB

Re-roll because of node.module.

sun’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

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
InternetDevels’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
70.32 KB

re-rolled

sun’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Berdir’s picture

Title: Add an EntityOwnerInterface » Change 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.

xjm’s picture

Issue tags: -Needs change rec +Needs change record
Berdir’s picture

Title: Change record: Add an EntityOwnerInterface » Add 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.