clean up use of interfaces and trait

Looks like the documentation you're referring to might be what is on this page https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21... - it's just a single line. So may or may not be worth linking to.

1. +++ b/content_entity_example/src/Entity/Contact.php
   @@ -153,13 +153,6 @@ class Contact extends ContentEntityBase implements ContactInterface {
   -  public function getChangedTime() {
   -    return $this->get('changed')->value;
   -  }
   -
   

   The removal of this method isn't exactly related to this issue. But it also appears that it's safe to do because it currently overrides the same method from \Drupal\Core\Entity\EntityChangedTrait, which has the exact same code.

2. +++ b/content_entity_example/src/Entity/Contact.php
   @@ -189,6 +182,69 @@ class Contact extends ContentEntityBase implements ContactInterface {
   +
   +  /**
   +   * {@inheritdoc}
   +
   +  /**
   +   * {@inheritdoc}
       *
       * Define the field properties here.
       *
   

   Seems like there might be an extra comment here?

Should this also update \Drupal\content_entity_example\Entity\Controller\ContactListBuilder::buildRow to use these new getter methods?

Interfaces define the public API of the service, as a promise to the code that uses the implementation.

Since that's the case, they also define the code that won't change. Other methods on the object might change. This implies that if you make an interface with getters and setters, you're locking yourself into maintaining those getters and setters. It also implies that the pieces of information that the object provides will always include whatever the getters give you. In our case this isn't such a big deal, but in this sense it's not a best practice to have getters and setters.

There's a whole debate about whether getters and setters are considered good. If the interface is meant to give you a specific piece of information (like getName() would), then you should add it. But if the purpose of the interface is to perform a task, then implementation details should be hidden.

So with that in mind, here's my review of the patch in #2:

+++ b/content_entity_example/src/ContactInterface.php
@@ -9,10 +9,84 @@ use Drupal\Core\Entity\EntityChangedInterface;
+  public function getName();

+++ b/content_entity_example/src/Entity/Contact.php
@@ -189,6 +182,69 @@ class Contact extends ContentEntityBase implements ContactInterface {
+  public function getName() {

This patch adds getters and setters to the interface and then implements them. But they are not used anywhere else in the module. So basically we're adding some methods and extra lines of code we don't need.

The only place we might use the interface is ContactListBuilder::buildRow():

  public function buildRow(EntityInterface $entity) {
    /* @var $entity \Drupal\content_entity_example\Entity\Contact */
    $row['id'] = $entity->id();
    $row['name'] = $entity->link();
    $row['first_name'] = $entity->first_name->value;
    $row['gender'] = $entity->gender->value;
    $row['role'] = $entity->role->value;
    return $row + parent::buildRow($entity);
  }

Here we see that we might only need getters for first name, gender, and role. So then we ask: Do we really need getters for these three things, and do we need them in the interface?

ContactInterface exists so that we can combine the other interfaces and make promises about Contact as belonging to the content entity API.

Contact subclasses ContentEntityBase, which defines how it works. If you look at ContentEntityBase::__construct() you see that entities don't want or need getters or setters, by design. That allows them to be highly arbitrary when it comes to existing fields. The entity is constructed out of a blob of data which is then mapped to the properties based on the entity definition.

So do we extend our entity implementation to have convenience methods? We do that for getCreatedTime(), which the patch rightly removes. :-) We also override EntityChangedTrait::getChangedTime() which we should probably also remove.

I don't think we need to add getters and setters here, particularly to the interface, because that's not the 'spirit' of the entity API. And also because it only adds more lines of code to need to change if we were to change our field definitions. We're planning on removing the gender field, which will be more effort if we add a gender getter to the interface: #2898380: Make content entity example more gender-inclusive

So: As cleanup, remove getCreatedTime() and getChangedTime() from Contact.

Thanks.

Made the changes from #5. Also this isn't a bug since everything works, we just have some questions about what our interface should be.

And we got it. Thanks!