Convert comments to be revisionable

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

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!

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

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

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

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

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

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

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

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

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.

Oh and yes, the same problem applies to #2716081: BlockContent should have revision_user and revision_created fields and implement RevisionLogInterface, but I never saw that one until now, and that entity type still doesn't have REST test coverage, so it was easy to miss. #2835845: EntityResource: Provide comprehensive test coverage for BlockContent entity is still in progress.

CR updated: https://www.drupal.org/node/2897893/revisions/view/10579311/10580834

reroll

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

#22++

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

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

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.

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.

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)

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.

I re-rolled this because I wanted to test the update path. Are the revision user and creation times meant to be pre-filled from the original entities, because they are empty after the update.

Oops, I did actually post a patch with the initialValueFromField fix but I posted it to the wrong issue! Unable to convert a non-translatable entity into a revisionable entity - That worked well for me.

Per #34.

#2915398: The moderation_state field is not computed during the creation of a new entity translation. fixed.

Using the same approach as #2880149: Convert taxonomy terms to be revisionable to use #2984782: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data

This is a 8.6 re-roll of #34.

This is a 8.7 re-roll of #34.

Deleted by user. Sorry, posted this to the wrong issue page. I was working on fixing a patch for ECK and had this issue open as well. Just put it in the wrong place.

Last patch had an issue which was breaking configuration synchronization when attempting to enable Content Moderation module. Added a condition to fix that and re-rolled the patch for 8.9.x.

Here an interdiff for the last patch.

I updated to D9, but the patches above do not apply. My current version is 9.2.10. Can some one guide me what need to be changed

@sreenivasparuchuri - The patch is just old and needs be rerolled against 9.3.x

it needs reroll against 9.4.x as 9.3 in release freeze

Re-rolled against 9.4.x. Please review

Fixed CS issues.

I can't apply any of these patches. At the moment I have to stick with Core 9.1.6, and I get the following error

Class 'Drupal\Core\Entity\Sql\SqlContentEntityStorageSchemaConverter' not found in comment_post_update_make_comment_revisionable() (line 24 of /var/  
  www/html/docroot/core/modules/comment/comment.post_update.php). 

and indeed it seems SqlContentEntityStorageSchemaConverter was deprecated in drupal:8.7.0 and is removed from drupal:9.0.0. Use \Drupal\Core\Entity\EntityDefinitionUpdateManagerInterface::updateFieldableEntityType() instead. (source).

Could you point me in the right direction please ?

@Kojo Unsui I'm getting the same. I'm going to look into creating another patch to test it with 9.3.6.

I am using 9.3.13, and i have comment enabled, and after using this patch, when i saved the Comment form display, I am seeing below error

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "hidden" plugin does not exist. Valid plugin IDs for Drupal\Core\Field\WidgetPluginManager are: address_default, address_zone_default, address_country_default, brightcove_inline_entity_form_complex, comment_default, moderation_state_default, cshs, datetime_datelist, datetime_default, daterange_datelist, daterange_default, entity_browser_entity_reference, entity_browser_file, entity_reference_revisions_autocomplete, erviews_options_buttons, erviews_options_select, file_generic, image_image, inline_entity_form_complex, inline_entity_form_simple, link_default, oembed_textfield, media_library_widget, menu_item_extras_view_mode_selector_select, metatag_firehose, path, pega_ecm_entity_reference_drill_down_select, pega_react_multi_select_widget, pegacc_core_industry, pegacc_core_product_area, file_bolt, pegacc_core_suggestion_textfield, poll_choice_default, redirect_source, datetime_timestamp_no_default, scheduler_moderation, select2_entity_reference, select2, text_textarea_with_summary, text_textfield, text_textarea, vote_up_down_widget_type, webform_entity_reference_autocomplete, webform_entity_reference_select, entity_reference_paragraphs, paragraphs, datetime_timestamp, email_default, options_select, string_textarea, uri, string_textfield, language_select, number, boolean_checkbox, entity_reference_autocomplete, options_buttons, entity_reference_autocomplete_tags in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 53 of /var/www/docroot/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php).

This is due to the code in core/modules/comment/src/Entity/Comment.php

$fields['revision_log_message']->setDisplayOptions('form', [
      'type' => 'hidden',
    ]);

As per the https://www.drupal.org/node/2801513 , 'type' => 'hidden', causes issue, I am adding new patch where i replaced the that code
Before

$fields['revision_log_message']->setDisplayOptions('form', [
      'type' => 'hidden',
    ]);

After

$fields['revision_log_message']->setDisplayOptions('form', [
      'type' => 'hidden',
    ]);

Reroll the patch #59 with Drupal 9.5.x

Comment #65 discards the changes from #63. I'm going to reroll against #63 and replace the usage of \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchemaConverter with \Drupal\Core\Entity\EntityDefinitionUpdateManagerInterface::updateFieldableEntityType().

I started a MR with the patch from #65, rebased and changed the following things:

• Rebased against 10.1.x
• Removed the usage of SqlContentEntityStorageSchemaConverter
• Set initial values for the revision_created and revision_user fields to fix an issue when displaying the revisions overview, as discussed in #2350939.

I also created Comment Revision UI, a module using the work from #2350939: Implement a generic revision UI to implement a revision UI for comments.

Left a note on MR.

Commented.

Addressed Drupal CS issues of MR.

Shouldn't we also add a new_revision field to comment types like node types have, and implement CommentType::shouldCreateNewRevision()?

Seems to be some failures in the MR2872

Also the update hook will need upgrade tests.

Probably should update comment_user_cancel() so that revision_user is re-assigned to zero (0) when the author's account is cancelled. I recently ran into an issue where the comment was re-assigned but not the revision. See node_user_cancel() as an example:

node_mass_update($vids, [
  'uid' => 0,
  'revision_uid' => 0,
], NULL, TRUE, TRUE);