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:

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: [PP-6] 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.

timmillwood’s picture

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