Introduce a EditorialContentEntityBase class for revisionable and publishable entity types

The status field is added to BlockContent in #2820848: Make BlockContent entities publishable

+++ b/core/modules/node/src/Entity/Node.php
@@ -466,31 +400,9 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+    $fields['revision_timestamp']->setQueryable(FALSE);

I have no idea what this is supposed to be. And why this field on node wouldn't be "queryable" while it would be on the trait/default definition. Either both shouldnt' be queryable or neither?

Should we also use this for some test entity types? One that does nothing but extend from this, to make sure that this works out of the box on its own?

I have no idea what this is supposed to be. And why this field on node wouldn't be "queryable" while it would be on the trait/default definition. Either both shouldnt' be queryable or neither?

Making them not queryable seems to be just a pure historical effect. If you are honest, there is a big value in a complex moderation workflow, in which people want to know which particular revisions they have touched for example. Enabling queryability, is IMHO not some sort of BC break.

If I understand right setting it as not queryable is just to not break BC, maybe we can look in a follow up about resolving this.

@timmillwood
Sure, but we could enable it for new entity types and then just override it in node ... even I still don't see how it should be a BC break :)

Now all the dependencies have landed I'm re-uploading the "do not test" patch from #4 to see what testbot thinks. I've tested locally and it applies to 8.4.x and seems to work fine.

I guess the only remaining item would be #6, or if we break that out into a follow up?

the REST fails are because that test code still hardcodes the order in which fields are defined/returned, and this patch is changing that.

IMHO, that is a bad test, and we should rewrite it so it verifies that all the fields deny access but that it doesn't matter in which order.

IMHO, that is a bad test, and we should rewrite it so it verifies that all the fields deny access but that it doesn't matter in which order.

+1

If field error order does matter for a particular entity type (it could if field A depends on field B for example), then they should have explicit test coverage for that.

I think this looks good, ready to go!

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -829,19 +829,15 @@ public function testPatch() {
-    // First send all fields (the "maximum normalization"). Assert the expected
-    // error message for the first PATCH-protected field. Remove that field from
-    // the normalization, send another request, assert the next PATCH-protected
-    // field error message. And so on.
-    $max_normalization = $this->getNormalizedPatchEntity() + $this->serializer->normalize($this->entity, static::$format);
-    for ($i = 0; $i < count(static::$patchProtectedFieldNames); $i++) {
-      $max_normalization = $this->removeFieldsFromNormalization($max_normalization, array_slice(static::$patchProtectedFieldNames, 0, $i));
-      $request_options[RequestOptions::BODY] = $this->serializer->serialize($max_normalization, static::$format);
+    foreach (static::$patchProtectedFieldNames as $field_name) {
+      $normalization = $this->getNormalizedPatchEntity() + [$field_name => [['value' => $this->randomString()]]];
+      $request_options[RequestOptions::BODY] = $this->serializer->serialize($normalization, static::$format);
       $response = $this->request('PATCH', $url, $request_options);
-      $this->assertResourceErrorResponse(403, "Access denied on updating field '" . static::$patchProtectedFieldNames[$i] . "'.", $response);
+      $this->assertResourceErrorResponse(403, "Access denied on updating field '$field_name'.", $response);
     }
 
     // 200 for well-formed request that sends the maximum number of fields.
+    $max_normalization = $this->getNormalizedPatchEntity() + $this->serializer->normalize($this->entity, static::$format);

Explicitly RTBC'ing this portion of the patch as rest.module maintainer. This still tests what we need to be tested, just in a way that is less brittle. Thanks!

Looks like it needed a reroll.
UPDATE: Sorry didn't refreshed the page. This was already rerolled :(
But let this set to NR to see the reroll passes all the test

Fixing the broken rest tests.

Helps if I upload the right patch.

oops, I guess that did more harm than good.

+++ b/core/lib/Drupal/Core/Entity/EditorialContentEntityBase.php
@@ -0,0 +1,31 @@
+ * Provides a base entity class with extended revision and publishing support.
+ *
+ * @ingroup entity_api
+ */
+abstract class EditorialContentEntityBase extends ContentEntityBase implements EntityChangedInterface, EntityPublishedInterface, RevisionLogInterface {

IMHO it would be great to reference this base class somewhere in some central documentation, otherwise people might not find it.

I guess Drupal/Core/Entity/entity.api.php would be a good place to document this?

I also wonder if it's worth adding a EditorialContentEntityInterface, just so we can do if ($entity_type interfaceof EditorialContentEntityInterface) rather than if (is_subclass_of($entity_type, 'EditorialContentEntityBase')).

I guess Drupal/Core/Entity/entity.api.php would be a good place to document this?

Yeah sounds like it.

if ($entity_type interfaceof EditorialContentEntityInterface) rather than if (is_subclass_of($entity_type, 'EditorialContentEntityBase')).

I think we should never explicitly test for that ... Aren't there all the other interfaces already we should use instead?

We could do if ($entity_type instanceof EntityPublishedInterface && $entity_type instanceof RevisionLogInterface) to test if an entity type is revisionable and publishable, I guess we can also test for "published" and "revision" entity keys too.

We could do if ($entity_type instanceof EntityPublishedInterface && $entity_type instanceof RevisionLogInterface) to test if an entity type is revisionable and publishable, I guess we can also test for "published" and "revision" entity keys too.

Well, I would have thought that code either wants to deal with the published or the revision log part of an entity, so it can check what it needs to at a given point in time.

Let's fix #32 and then it is good to go imo.

That works for me.

Why did patch #38 fail? I checked if it needs to reroll. But it is not the problem.
Why does just adding documentation make it fail tests?

I put #38 for a retest.

It seems a shame we are changing the order of base field definitions here - especially since as far as I can see revision_uid now comes before uid - does that even make sense? It's like EntityOwnerInterface needs a trait or something. Not sure.

+++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -271,6 +271,11 @@
+ *   If you are defining a content entity type, it is recommended to extend the
+ *   \Drupal\Core\Entity\EditorialContentEntityBase base class in order to get
+ *   out-of-the-box support for Entity API's revisioning and publishing
+ *   features, which will allow your entity type to be used with Drupal's
+ *   editorial workflow provided by the Content Moderation module.

This change deserves a change record.

@amateescu thanks for the CR - it is really useful for people implementing the new base class. I think you should have a section on updating existing entities - and point them to the CRs that coverage the additional metadata needed for the traits to work if you have non standard field names.

I've tried to compare all the node tables before and after this patch - whilst the base field definitions have changed order in the API this has not resulted in any change to the underlying data structures. I guess we should include something in the CR about the order of this changing but that this is not API. I'm unsure of setStorageWeight() it'd be great if an entity API maintainer could chime in.

Committed 7667c3e and pushed to 8.4.x. Thanks!

This caused a nasty-to-fix failure in a previously RTBC patch: #2768651. The root cause is explained at #2768651-113: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX, and is not the fault of this patch, it's the flaws in the Entity/Field API's validation system, i.e. either in the Symfony constraints system, or the way Drupal uses it.

See #2820364: Entity + Field + Property validation constraints are processed in the incorrect order.