Problem/Motivation

As decided in #2745619: [policy, no patch] Which core entities get revisions?, comments should be converted to be revisionable.

Quoting @webchick from #2745619-9: [policy, no patch] Which core entities get revisions?:

Would allow some recourse for content administrators to go after a malicious user who filled their comment with a bunch of racist garbage to which other users reacted, and then later went back and edited their comment to something more innocent, to dodge being banned.

Proposed resolution

Do it.

Remaining tasks

Review.

User interface changes

None yet, revision support is only enabled at the API level.

API changes

Nope.

Data model changes

Comments are now revisionable.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

amateescu created an issue. See original summary.

amateescu’s picture

andypost’s picture

Please update IS about why comments "should" become revisionable by defalt

amateescu’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

It's in the issue summary of the parent issue, but ok, cross-posted here as well.

amateescu’s picture

Title: [PP-3] Convert comments to be revisionable » Convert comments to be revisionable
FileSize
11.77 KB

No more blockers! Here's a fresh patch for the testbot.

Status: Needs review » Needs work

The last submitted patch, 5: 2880154-5.patch, failed testing. View results

amateescu’s picture

Status: Needs work » Needs review
FileSize
11.77 KB
809 bytes

Let's fix it.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

Look good!

+++ b/core/modules/comment/src/Tests/Update/CommentUpdateTest.php
@@ -71,4 +73,53 @@ public function testPublishedEntityKey() {
+    $comment = $storage->loadRevision($comment->getRevisionId());

Yay, now we can load comment revisions!

tstoeckler’s picture

+++ b/core/modules/comment/comment.post_update.php
@@ -0,0 +1,35 @@
+ * Update custom menu links to be revisionable.

Nit: s/custom menu links/comments/

timmillwood’s picture

Oops, missed that.
Could be done on commit, but to keep things moving along, here's a patch.

Wim Leers’s picture

+++ b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php
@@ -123,6 +123,9 @@ protected function getExpectedNormalizedEntity() {
+      'revision_id' => [
+        ['value' => 1],
+      ],
       'uuid' => [

@@ -193,6 +196,11 @@ protected function getExpectedNormalizedEntity() {
+      'revision_created' => [
+        $this->formatExpectedTimestampItemValues((int) $this->entity->getRevisionCreationTime()),
+      ],
+      'revision_user' => [],
+      'revision_log_message' => [],

This is technically a BC break for API-First… I'll let core committers decide here.

timmillwood’s picture

[berdir] timmillwood: I don't think adding new fields violates BC?

Berdir’s picture

Content entities are by definition extensible, I don't see how adding more fields is a BC break? That would mean that I could introduce a BC break for Drupal by adding a field in the UI? :)

Then any improvement that adds more stuff to the serialized output would be a BC break too, e.g. exposting things like file url/image style urls, formatted text and so on.

To be clear: This is far from a trivial change, switching out the table storage could break a lot of stuff if people query database tables directly. Although making an entity type revisionable is far less problematic than making it translatable, because we don't really touch the existing tables that much, otherwise a lot of our custom threading queries and so on would be broken too. But those additional fields in the serializer seem like the least of the possible problems IMHO :)

Wim Leers’s picture

Issue tags: +Needs change record

Content entities are by definition extensible, I don't see how adding more fields is a BC break?

The difference is that this affects every Drupal site using this entity type. Adding more fields is usually a manual operation: if I add additional fields, I'll know I'm changing the data model, and I'll know I'm changing my REST/JSON API responses.

That's a very big difference.

Then any improvement that adds more stuff to the serialized output would be a BC break too, e.g. exposting things like file url/image style urls, formatted text and so on.

Yep! And this is why it's so painful to work on API-First: because every improved normalization requires a BC layer. This is why we have bc_entity_resource_permissions in REST and bc_primitives_as_strings +bc_timestamp_normalizer_unix in Serialization: all these improved normalizations are disabled by default, you have to opt in to them manually.

We'll need similar BC layers for every single thing you mentioned. Without those BC layers, they'd never have been committed.


I do agree that adding more fields is less of a BC break than changing the normalization of a field! But it's still a BC break.

That's why I didn't mark this NW. That's why I said technically a BC break and I'll let core committers decide: because I think this is a reasonable change, with a manageable amount of disruption. But it's still totally possible that this is going to break some decoupled applications. And at minimum, this will need a change record, to warn those sites.

(Also, if it lands, it ought to get an entry in the 8.4.0 release notes!)

timmillwood’s picture

Issue tags: -Needs change record

Added a provisional change record https://www.drupal.org/node/2897893

Berdir’s picture

Yep! And this is why it's so painful to work on API-First: because every improved normalization requires a BC layer. This is why we have bc_entity_resource_permissions in REST and bc_primitives_as_strings +bc_timestamp_normalizer_unix in Serialization: all these improved normalizations are disabled by default, you have to opt in to them manually.

Both of those two are different in that they *change* existing structure/behavior. That's not the same as adding propertes/fields that didn't exist before.

We're allowed to add methods to most classes, we're allowed to add the actual fields to the entities (and add and change database tables!), I don't see why we wouldn't be allowed to expose those fields in the serializer. That's how the serializer works by definition IMHO, it dynamically exposes all available/accessible data, as I've said elsewhere, if you require a 100% stable, identical output for your client, then you should write your API. If that is not as clear to you/others as it is to me, then we should document that explicitly on https://www.drupal.org/core/d8-bc-policy. #2716081: BlockContent should have revision_user and revision_created fields and implement RevisionLogInterface is an example that also added additional fields to block_content for example.

But yes, we definitely need a change record here, because as I said, making the entity type revisionable, adding the revision id and other fields is likely going to affect some sites. I'm definitely not arguing against that :)

Wim Leers’s picture

I'm strongly in favor of documenting the BC promises around REST responses and normalizations at https://www.drupal.org/core/d8-bc-policy! I think that demanding that clients allow additional keys (whether they be fields or properties) to show up in normalizations is a totally fair expectation. I'd love to see that documented there.

All I'm saying here is that this change has the potential to break API clients, and therefore it's technically a BC break, and we should pay close attention to it.

Wim Leers’s picture

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 2880154-10.patch, failed testing. View results

timmillwood’s picture

Status: Needs work » Needs review
FileSize
12.95 KB

reroll

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

Going back to RTBC because I RTBC'd @amateescu's patch in #8, since then I fixed a nitpick and performed a reroll.

@Wim Leers and @Berdir make some good comments regarding backwards compatibility but I'm not sure there is any way around this. The awesome CR updates from @Wim Leers really help, so let's get this in and start to make more things revisionable in 8.4.0

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

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

Wim Leers’s picture

#22++

timmillwood’s picture

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

As this is already RTBC it should be fine for 8.4.x

xjm’s picture

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

@timmillwood reached out to me about whether this issue is still eligible for 8.4.x. It's a distruptive change, so it should remain filed against 8.5.x generally. During the alpha phase, disruptive changes may be backported at committer discretion after commit. The fact that the issue was RTBC for the past couple weeks is taken into account but does not necessarily mean it will be backported.

In the case of this issue, though, it's a strategic priority, so I would indeed like to backport it if we commit it in time for the beta freeze deadline on Aug. 15, unless other committers raise concerns. Leaving the branch at 8.5.x though; we make the final decision after commit.

Thanks!

xjm’s picture

I talked to @larowlan about this issue and we're a little concerned about the risk and disruption. For example, the related taxonomy issue introduced regressions for the term overview page. It might be best to commit issues like these and #2820848: Make BlockContent entities publishable to 8.5.x early so we have the full development release to test them.

timmillwood’s picture

I can understand the concern for this issue but not for #2820848: Make BlockContent entities publishable. Making BlockContent entities will really help Content Moderation, because it will be possible to archive a block and it will not be visible to users. Currently when BlockContent Entity is archived it is still visible on the site.

timmillwood’s picture

I've been taking a look at possible risks of committing this to 8.4.0:

  • Anonymous users with the permission "Post comments" see a confusing "Revision log message" field.
  • With Content Moderation enabled comments are users with the right permission see the Content Moderation widget.
  • With Content Moderation enabled users with no Content Moderation permissions get comments created as "Draft" (unpublished), even if they have the "Skip comment approval" permission.
  • Bulk operations to "Approve" (publish) comments is blocked by Content Moderation. Approving comments now needs to be done by changing the moderation state to "Published", which doesn't have a bulk operation.

I hate to say this, but pushing to 8.5.x might be a wise choice.

(Although still sure publishing BlockContent entities will be fine for 8.4.0)

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Thanks @timmillwood. Some of those things sound like they might be blocking to me. The first one in particular is a UX regression for a pretty common usecase (anonymous comments). Let's file separate issues for each point and postpone this issue? Setting NR for now.