Revision log message field missing from layout overrides entity form

Re-opening as I'm experiencing the same issue where there is no revision log field is present at all. Furthermore I opened up a Drupal Answers post and received confirmation of the bug from a user over there:

I did create a clean Drupal 8 instance, enabled Layout Builder for the basic page content type with enabled revisions.
I saw the same exact behaviour. Additionally, log messages were only saved with "edit" form and not with the "layout" form. So, it is definitely a core layout builder module bug and not related to the distribution you're using.

The distribution noted above is the Lightning Distro, but that doesn't appear to be the culprit. Marking Major as it completely breaks revisions related to Layout Builder.

I think I found the source of this bug.
In Drupal\layout_builder\Form\OverridesEntityForm class, the init() loads the entity $form_display using collectRenderDisplay method with the default_fallback set to FALSE, thus it removes the original components of the Node Display Form including the 'revision_log' component.

/**
   * {@inheritdoc}
   */
  protected function init(FormStateInterface $form_state) {
    parent::init($form_state);

    $form_display = EntityFormDisplay::collectRenderDisplay($this->entity, $this->getOperation(), FALSE);
    $form_display->setComponent(OverridesSectionStorage::FIELD_NAME, [
      'type' => 'layout_builder_widget',
      'weight' => -10,
      'settings' => [],
    ]);

    $this->setFormDisplay($form_display, $form_state);
  }

I think at this stage, the OverridesEntityForm should check if the entity implements \Drupal\Core\Entity\RevisionLogInterface and accordingly sets the component for 'revision_log'.

This patch I believe needs some work, but it is working for me on node entities, to add the log message when creating layout.

Oh I wish we'd had test coverage. This was definitely working before, but that change to collectRenderDisplay indeed broke it.

One note:

+++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
@@ -88,6 +88,14 @@ protected function init(FormStateInterface $form_state) {
+    $section_storage = $form_state->getBuildInfo()['callback_object']->getSectionStorage();

$form_state->getBuildInfo()['callback_object'] is equivalent to $this here, so this can be $section_storage = $this->getSectionStorage();

Cool - as Ismail Cherri pointed out, we should check to see that the entity type is revisionable, so added that check in using the `isRevisionable()` method, which I *think* we can rely safely on here.

@twfahey If I may add, I don't think you need to get the Entity through the section storage at this point because $this->entity is already defined and it is used before your added code.

Good call - simplified in latest patch, and adding initial testing.

Cleaned up from last patch.

Remove unused modules and body.

Realized I wasn't asserting the block text had reverted back to the original text.

Setting this back to needs work for the nitpick, but I could only find that really small issue here, overall this is looking really good and it looks like it has sufficient (functional) coverage.

+++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
@@ -88,6 +88,14 @@ protected function init(FormStateInterface $form_state) {
+      $form_display->setComponent($revision_log_message_form_item, $log_field_revision_message_form_data);  ¶

Übernit: this adds unneeded whitespace at the end of the line, can we remove that?

Comments addressed in #15 & added an interdiff as well.

Removed the needs tests tag because it seems like there is sufficient coverage now. My only nitpick also has been resolved.

This patch works as expected. Applied cleanly against core 8.7.3.

1. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -88,6 +88,14 @@ protected function init(FormStateInterface $form_state) {
   +    if ($entity_type->isRevisionable()) {
   +      $revision_log_message_form_item = $entity_type->getRevisionMetadataKey('revision_log_message');
   

   The getRevisionMetadataKey method is only on \Drupal\Core\Entity\ContentEntityTypeInterface so in theory this needs an instanceof check...

   The isRevisionable check worries me. It shouldn't be necessary if there's a revision metadata key. Also, are we sure this is all that is needed? Are there more checks needed?

2. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -88,6 +88,14 @@ protected function init(FormStateInterface $form_state) {
   +      $log_field_revision_message_form_data = $log_field_definition->getDisplayOptions('form');
   +      $form_display->setComponent($revision_log_message_form_item, $log_field_revision_message_form_data);
   

   Will this respect the individual options as set up in the baseFieldDefinitions methods? For example, MenuLinkContent hides this

Looks like we need answers to #20.

The attached patch addresses the questions/concerns raised by @tim.plunkett in #20:

The getRevisionMetadataKey method is only on \Drupal\Core\Entity\ContentEntityTypeInterface so in theory this needs an instanceof check...

Added instanceof check.

The isRevisionable check worries me. It shouldn't be necessary if there's a revision metadata key. Also, are we sure this is all that is needed? Are there more checks needed?

Replaced isRevisionable() with showRevisionUi() which also checks the $show_revision_ui property on the EntityType plugin.

Will this respect the individual options as set up in the baseFieldDefinitions methods? For example, MenuLinkContent hides this

I believe using showRevisionUi() instead of isRevisionable() should prevent this from doing anything for EntityType plugins like MenuLinkContent that haven't explicitly enabled the $show_revision_ui property.

Added defaultTheme property and setup method return typehint to LayoutBuilderRevisionTest.

Patch from comment #30 works for me

+1 for #30

1. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -88,6 +89,14 @@ protected function init(FormStateInterface $form_state) {
   +    if ($entity_type instanceof ContentEntityTypeInterface && $entity_type->showRevisionUi()) {
   

   This form extends ContentEntityForm, so the instanceof check isn't needed and there's a $this->showRevisionUi() method already that can be used.

2. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -88,6 +89,14 @@ protected function init(FormStateInterface $form_state) {
   +      $revision_log_message_form_item = $entity_type->getRevisionMetadataKey('revision_log_message');
   +      $log_field_definition = $this->entity->getFieldDefinition($revision_log_message_form_item);
   +      $log_field_revision_message_form_data = $log_field_definition->getDisplayOptions('form');
   +      $form_display->setComponent($revision_log_message_form_item, $log_field_revision_message_form_data);
   

   I guess my real question is: why do we need this when I don't see any other part of core doing it?

   The only time I see revision_log_message related to form displays is for things like Term where it's being hidden. Which implies that the default is to have this shown. So is there some other place we're doing something wrong instead?

   In fact, there are only two places in core that $form_display->setComponent() is ever called: a form alter (which makes sense), and here. That makes me feel like we're missing something

3. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderRevisionTest.php
   @@ -0,0 +1,108 @@
   +  public static $modules = [
   

   protected not public

4. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderRevisionTest.php
   @@ -0,0 +1,108 @@
   +    $components = Node::load(1)->get('layout_builder__layout')->getSection(0)->getComponents();
   

   I bet this is copied from somewhere else, so no one's fault here. But this should use the OverridesSectionStorage:: constant instead of 'layout_builder__layout'

Addressed items 1, 3 & 4 from #36.

@tim.plunkett Re: item 2 in #36, I definitely don't know the answer to your question(s) but the only alternative to this implementation that I can think of would be to change the $default_fallback parameter to TRUE in the call to EntityFormDisplay::collectRenderDisplay() and I imagine that would cause other problems.

FWIW, it seems that this issue was introduced as a result of the changes committed for #3038442: Use hook_entity_form_display_alter() instead of Layout Builder's custom overrides display altering hook (based on earlier comments #4 and #7 in this issue).

Patch applied successfully for me on drupal 9.3.x-dev version. Thanks for the patch. RTBC +1

Created MR from #37 to make Tugboat QA site available.

Updated issue title and issue summary because this part of the original issue report is not reproducible in 9.3.x:

When saving a layout override for an entity, a new entity revision is created regardless of whether the "Create new revision" checkbox is checked or not.

I would still like to explore #36.2, might have to ping an Entity Field API expert.

Hiding the old patches now that we're using an MR.

My content supervisors are asking for this functionality, so I tested the patch on our dev site. It worked. The revision message field is now visible and usable on the Layout Builder page. I'll see if I can find time to do a proper patch review later.

I think that this can be safely moved to RTBC without addressing #36.2. The solution seems to work great and there is test coverage.

My rationality here is that imperfect under-the-hood implementation is preferable to broken functionality. In situations like this, IMO it is preferable to restore functionality and then open a task to address any concerns in the working functionality.

The MR is on 9.3.x. Let's get a patch for 9.5 and 10.

Added a patch for Drupal 9.5.x which is also getting applied on Drupal 10.1.x

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Moving to NW for an answer for 36.2

The patch from #48 seems to create a corrupted revision, when saving the layout form without any changes.

Steps to reproduce:

1. Get a fresh installed d10.1.5 with layout builder enabled and the patch applied.
2. Have a content type that has the full view enabled for layout builder.
3. Create a node of that content type.
4. This will create the first revision on /node/1/revisions.
5. Goto /node/1/layout and press save layout without changing anything.
6. No new revision will show up on /node/1/revisions.
7. Goto /node/1/layout, move a block an press save layout.
8. A new revision will show up on /node/1/revisions, but hast vid 3.

I've also found that in the DB in node_field_revision are three entries, so 5. created data, but this row has the revision_translation_affected field emtpy. That's why it's not showing up on /node/1/revisions.

Edit: Sorry, just realized that even without the patch, a corrupted revision get's saved. So is is probably another bug.

Patch #48 resolves the issue for us

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

This looks good to me. I merged in the latest from 11.x and fixed a couple minor deprecation issues in the test.

Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!