Problem/Motivation

As part of #2721129: Workflow Initiative, we want to improve revision metadata and publishing state capabilities for content entity types, in order to allow them to be used in the following contexts:

  • Content Moderation: more content entity types will be able to go through moderation workflows.
  • Trash: more content entity types will have archiving support.
  • Workspaces: see #2732071: WI: Phase G1: Workspace module.

Proposed resolution

Introduce a new base class for content entities which supports extended revision and publishing state capabilities by default, through RevisionLogInterface / RevisionLogEntityTrait and \Drupal\Core\Entity\EntityPublishedInterface / EntityPublishedTrait.

The list of core entity types which will implement the new base class is being discussed in #2745619: [policy, no patch] Which core entities get revisions?.

Remaining tasks

User interface changes

Nope.

API changes

API addition: a new content entity base class is available.

Data model changes

Nope.

Comments

amateescu created an issue. See original summary.

amateescu’s picture

Status: Needs review » Needs work

The last submitted patch, 2: 2825973-combined.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
5.26 KB
252.52 KB

Ok, BlockContent needs the status field first so it will require it's own issue, so let's try to convert only nodes here.

timmillwood’s picture

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

Berdir’s picture

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

dawehner’s picture

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.

timmillwood’s picture

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.

dawehner’s picture

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

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

timmillwood’s picture

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?

Status: Needs review » Needs work

The last submitted patch, 11: 2825973-11.patch, failed testing.

Berdir’s picture

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.

Wim Leers’s picture

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.

amateescu’s picture

Status: Needs work » Needs review
FileSize
7.8 KB
2.54 KB

Ok then, let's fix the bad test.

Status: Needs review » Needs work

The last submitted patch, 15: 2825973-15.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
7.56 KB
2 KB

All those tests are not initializing the entity tables properly, so let's just use a dummy field value for the purpose of that test.

As for the 'queryable' discussion above, I did quite a bit of digging and found out that it was introduced in #1696640-57: Implement API to unify entity properties and fields but never used, so I opened #2855886: Deprecate \Drupal\Core\Field\FieldStorageDefinitionInterface::isQueryable() because it's not used anywhere for it.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

I think this looks good, ready to go!

Wim Leers’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: 2825973-17.patch, failed testing.

amateescu’s picture

himanshu-dixit’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.49 KB

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

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

The testbot will kick it back to NW in case of a test fail ;)

The last submitted patch, 21: 2825973-21.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: 2825973-21.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 26: 2825973-26.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
9.26 KB

Helps if I upload the right patch.

Status: Needs review » Needs work

The last submitted patch, 28: 2825973-28.patch, failed testing.

timmillwood’s picture

oops, I guess that did more harm than good.

amateescu’s picture

Status: Needs work » Needs review
FileSize
9.47 KB
1.98 KB

Rerolled for the short array syntax conversion and fixed the failing tests.

Interdiff is to #21.

dawehner’s picture

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

timmillwood’s picture

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

dawehner’s picture

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?

timmillwood’s picture

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.

dawehner’s picture

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.

jibran’s picture

Status: Needs review » Needs work

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

amateescu’s picture

Status: Needs work » Needs review
FileSize
10.51 KB
1.04 KB

How about this?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

That works for me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 38: 2825973-38.patch, failed testing.

boaloysius’s picture

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.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community

It was just a random testbot fail :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 38: 2825973-38.patch, failed testing.

amateescu’s picture

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

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

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

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record

Struggled a bit to write this one without repeating myself too much: https://www.drupal.org/node/2870643

Hope it's useful though :)

amateescu’s picture

As for the base field ordering, it is indeed a shame that we don't have any good way to order them, but that should be easy enough to implement with something like setStorageWeight(). What do you think, should we open an issue for that?

alexpott’s picture

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

amateescu’s picture

@alexpott, ok, I added a paragraph for updating an existing entity type and pointed to https://www.drupal.org/node/2831499 and https://www.drupal.org/node/2830201 which I also had to publish because they were still in draft.

And I don't think we should go into much detail about the field ordering change in this CR, better keep it simple and to the point :)

alexpott’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed 8938ab2 on 8.4.x
    Issue #2825973 by amateescu, timmillwood, himanshu-dixit, dawehner,...
Wim Leers’s picture

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 validation constraints are processed in the incorrect order.

himanshu-dixit’s picture

Status: Fixed » Closed (fixed)

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