Add generic status field to ContentEntityBase

Note that we also have #2009958: Introduce EntityStatusInterface to standardize how entities can be disabled which is trying to move the status concept to an interface.

Is there a reason this doesn't yet add a status() method or so?

Sorry, my comment in #15 haven't seen #13 and #14 yet. I agree this makes sense.

I think we should try to limit the amount of business logic we bake into ContentEntityBase to the very minimum, otherwise we are kind of limiting its purpose to very specific use cases, which is not what we want for a base class, I think. Or at very least we are adding code that won't be used in many cases.

It would make more sense to me to implement this as a trait that can be individually used by entity types implementing the EntityPublishedInterface.

In Node status is an entity key, and this patch makes status an entity key in Comment too. All base fields currently defined within ContentEntityBase are all entity keys, therefore I like the idea of carrying on putting entity key fields in ContentEntityBase.

Entity keys are meant to provide a way to map generic business logic to the fields required to implement it without "crippling" subclasses with reserved field names. So the fact that status is an entity key does not automatically imply it should be part of content entity base, it's just a pointer that it probably makes sense as being used by a base class. IMHO the node entity type is currently abusing the status entity key (and it's doing it wrong, btw: http://cgit.drupalcode.org/drupal/tree/core/modules/node/src/Entity/Node...).

I'd like to get to the point that the majority of entity types have a published/unpublished state and therefore use the status entity key and field.

What feels wrong to me here is the following bit: the majority of entity types. If we do this it must be something that we feel is an inherent and obvious part of the generic content entity business logic. However there are core entity types for which the concept of publishing simply makes no sense, at least according to their current implementation. In fact they may have a concept of status that does not map directly to the concept of publishing. If we add the status field to CEB, then the next natural step is making it implement EntityPublishedInterface, which may quickly lead to a very confusing API in this case. The User and File entity types are a clear example of this scenario.

These are a few where a publishing status would make sense to me mostly when thinking to the "full site revisioning/previewing" problem space:

• Term
• MenuLinkContent

And a few more where I'm not even sure about that:

• ContentModerationState
• Feed
• Item
• Message
• Shortcut

So it seems to me that the end goal of this issue could use a broader discussion, unless it already happened and I missed it.

I forgot: if a trait is deemed a suboptimal DX, it would make a lot more sense to me from an architectural POV to have a base class extending RevisionableContentEntityBase that all entity types for which it clearly makes sense the publishing concept can inherit from, e.g. EditorialContentEntityBase .

Either the trait or the additional base class in #23 makes sense to me. This looks like a 50/50 use case given the list of entities in #22.

Thanks for trying the trait approach!

1. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
   @@ -0,0 +1,34 @@
   +      ->setDescription(t('A boolean indicating the published state.'))
   

   This t() call is inconsistent with the previous line.

2. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
   @@ -0,0 +1,34 @@
   +    return ['status' => BaseFieldDefinition::create('boolean')
   

   We can use $entity_type->getKey('status') here to get the field name.

3. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
   @@ -0,0 +1,34 @@
   +    return (bool) $this->get('status')->value;
   ...
   +    $this->set('status', $published ? NODE_PUBLISHED : NODE_NOT_PUBLISHED);
   

   And here we can assume the existence of a ::getEntityKey() method. We may even declare it as abstract if we want to be super strict.

Removing the status key sounds like an API/BC break to me. Why can't we just keep it since it's not hard to do?

Thanks, almost there!

1. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
   @@ -0,0 +1,42 @@
   +trait EntityPublishedTrait {
   ...
   +  public static function publishedBaseFieldDefinitions(EntityTypeInterface $entity_type) {
   

   Missing PHP docs.

2. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
   @@ -0,0 +1,42 @@
   +  public function isPublished() {
   +    return $this->getEntityType()->hasKey('status') ?
   +      (bool) $this->getEntityKey('status') :
   +      (bool) $this->get('status')->value;
   +  }
   

   We could save a few method calls if we did something like this:

     $status = $this->getEntityKey('status');
     return (bool) (isset($status) ? $status : $this->get('status')->value);
   

   Since we have a default value, I guess ::getEntityKey() will return NULL only if the entity key is not defined.

   Same goes for the setter.

3. +++ b/core/modules/rest/src/Tests/UpdateTest.php
   @@ -292,10 +292,10 @@ public function testUpdateComment() {
        $read_only_fields = [
   +      'status',
          'name',
          'created',
          'changed',
   -      'status',
   
   @@ -311,6 +311,7 @@ public function testUpdateComment() {
   +      'status',
          'pid', // Extra compared to HAL+JSON.
          'entity_id',
          'uid',
   @@ -318,7 +319,6 @@ public function testUpdateComment() {
   
   @@ -318,7 +319,6 @@ public function testUpdateComment() {
          'homepage', // Extra compared to HAL+JSON.
          'created',
          'changed',
   -      'status',
   

   Are these changes required?

+++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
@@ -0,0 +1,42 @@
+      ->setLabel(new TranslatableMarkup('Publishing status'))
+      ->setDescription(t('A boolean indicating the published state.'))

Can we always use TranslatableMarkup here?

+++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
@@ -5,34 +5,53 @@
+    return (bool) ($status ?: $this->get('status')->value);

Won't this break if the entity type has a status key different from status and the field value is 0 or FALSE?Shouldn't we use isset() to ensure we have a NULL value?

Looks good, thanks

+++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
@@ -0,0 +1,61 @@
+    // similar when introduced. https://www.drupal.org/node/2811667

This references #2811667: Introduce EntityPublishedInterface for content entities, which has been closed as a duplicate of #2789315: Create EntityPublishedInterface and use for Node and Comment so we might want to update the reference to point to the active issue.

This looks really tidy to me. Committed/pushed to 8.3.x, thanks!

The update function for the comment entity type introduced here is not really updating the cached field storage definition. A followup for this - #2841614: comment_update_8300 is not updating correctly the field storage definition of the status field.