New pseudo-fields cannot be removed, InvalidArgumentException thrown

Exceptions that can be easily thrown by UI actions are major priority.

If you can attach an example module with hook_entity_extra_field_info() and hook_entity_view() it would be handy to test the issue.

I was able to reproduce following the steps in the description. I did a Standard site install. Then I set the default view mode of Article content type to display with Layout Builder.

Then I added the code below to the core telephone module for convenience, enabled telephone, and cleared cache. I went to the Article layout and tried to remove the Test Pseudofield. I saw the same error in the console relating to invalid UUID.

/**
 * Implements hook_entity_extra_field_info().
 */
function telephone_entity_extra_field_info() {
  $extra = array();

  $extra['node']['article']['display']['test_pseudo_field'] = array(
    'label' => t('Test Pseudofield'),
    'description' => t('This is a test pseudo-field'),
    'weight' => 100,
    'visible' => TRUE,
  );

  return $extra;
}

/**
 * Implements hook_ENTITY_TYPE_view().
 */
function telephone_node_view(array &$build, EntityInterface $entity, EntityViewDisplayInterface $display, $view_mode) {
  if ($display->getComponent('test_pseudo_field')) {
    $build['test_pseudo_field'] = [
      '#type' => 'markup',
      '#markup' => 'This is a test pseudo-field',
    ];
  }
}

Great, thanks for that. I was able to reproduce.
In the short-term, here's the very silly workaround:

Once you save the layout (including the extra field), you can then remove it.

It's only during the time that it is *not yet saved* that this can break.
Which is because it is being dynamically added in \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::setComponent(), with a fresh call to \Drupal::service('uuid')->generate()/code> on every page.
Which means the UUID is one thing when the UI is printed (with the link to remove or configure), and another UUID by the time the form is loaded to remove/configure it.

This is still a very annoying bug (and an embarrassing one as the person who wrote the code) but since it has a straightforward workaround I'm downgrading to major.

The next step is to write tests that expose the bug.

Here's a patch that adds a new test to LayoutBuilderUiTest (FunctionalJavascript) that covers this issue. The new test is called testNewExtraFieldRemoval().

This function includes a couple of commented lines that recreate the current workaround. Uncommenting these lines results in the test passing. Not sure if that will help us, but it's in there.

I *think* this patch does the trick. Fingers crossed on the tests. This patch also resolves #3069578: Layout builder doesn't show new "extra fields" I think, which I've added as a related issue. I cross posted the patch so it gets visibility there.

I'm not great at git so here's a copy/past interdiff!

diff --git a/core/modules/layout_builder/src/EventSubscriber/PrepareLayout.php b/core/modules/layout_builder/src/EventSubscriber/PrepareLayout.php
index b68092ce83..74cced9c2b 100644
--- a/core/modules/layout_builder/src/EventSubscriber/PrepareLayout.php
+++ b/core/modules/layout_builder/src/EventSubscriber/PrepareLayout.php
@@ -76,8 +76,8 @@ public function onPrepareLayout(PrepareLayoutEvent $event) {
       foreach ($sections as $section) {
         $section_storage->appendSection($section);
       }
-      $this->layoutTempstoreRepository->set($section_storage);
     }
+    $this->layoutTempstoreRepository->set($section_storage);
   }
 
 }

Yes! It passed! The "idea" of the patch is that it looks like we need to get the section storage into the tempstore as soon as possible.

I noticed that to work around the bug you don't even need to save the layout, which is the workaround described in the issue summary. It's sufficient to do any action on the form that results in the section storage getting added to tempstore, like moving any block or removing any block. After taking such an action you can do whatever you want with the buggy extra field block.

Looks good
If further code review is not needed, this can be moved to RTBC

I'm closing #3069578: Layout builder doesn't show new "extra fields" as a duplicate, because this issue is further along. It's still not 100% clear to me why this change fixes the bug from the other issue, but it does... And it is clear why it fixes this one.

1. +++ b/core/modules/layout_builder/src/EventSubscriber/PrepareLayout.php
   @@ -76,8 +76,8 @@ public function onPrepareLayout(PrepareLayoutEvent $event) {
   -      $this->layoutTempstoreRepository->set($section_storage);
        }
   +    $this->layoutTempstoreRepository->set($section_storage);
   

   Can't see from the patch context, but this means an extra set() even when it's already there.

   Currently we have an if/elseif, and no else. The if case does not need this set(), but the elseif and unwritten else case do.

   Let's switch to only having an if/else, make the current elseif a standalone if inside the else, and have the set() call at the end of that else. (hope that makes sense)
   Also add an inline comment explaining that we're adding it to tempstore regardless of what the storage is.

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -240,6 +240,31 @@ public function testAddHighlights() {
   +    drupal_flush_all_caches();
   

   Replace this with the actual cache needing clearing:
   \Drupal::service('entity_field.manager')->clearCachedFieldDefinitions();

3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -240,6 +240,31 @@ public function testAddHighlights() {
   +    // Uncomment next two lines to make test pass, workaround for #3144010.
   +    // $this->drupalGet(static::FIELD_UI_PREFIX . '/display/default/layout');
   +    // $page->pressButton('Save layout');
   +
   

   Let's remove this

4. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -240,6 +240,31 @@ public function testAddHighlights() {
   +    $this->clickContextualLink('.block-extra-field-blocknodebundle-with-section-fieldlayout-builder-extrafield-test', 'Remove block');
   +    $this->assertNotEmpty($assert_session->waitForElementVisible('css', '#drupal-off-canvas'));
   +    $assert_session->assertWaitOnAjaxRequest();
   +    $assert_session->pageTextContains('Are you sure you want to remove');
   

   Might as well finish removing it, just to prove the whole process works

Thanks @tim.plunkett. All those notes look good. I have some free time now so I can make those changes.

Regarding #3069578: Layout builder doesn't show new "extra fields", do you think we should include a test addressing that specific situation? It would be easy enough to add a second extra field that has visible => FALSE. It wouldn't even have to be a whole new function. It could just be a few lines added to the test we're already working on here.

Yes please!
And can you expand the issue summary here to include that?
Thanks

I have tried to check issue from https://www.drupal.org/project/drupal/issues/3069578 with patch #10, but have no success.

Extra field with visible => FALSE is not shown in LB.

Same problem, if you have visible => TRUE, but then disable layout_builder, move the extra_field to "Disabled" and then re-enable layout_builder.

While patch #21 from original issue solves the problem, so I'm not sure that it should be closed as duplicate.

You're right, @sorlov. I'll re-open that issue (#3069578: Layout builder doesn't show new "extra fields") and comment there.

Thanks for testing. Sorry for the confusion I caused!

Here's a patch incorporating all the feedback from comment #13.

And as @sorlov pointed out, I was mistaken that this also fixed #3069578: Layout builder doesn't show new "extra fields". So this patch does not increase the scope at all; my ambition in #14 was misguided!

Removing "Needs issue summary update" tag. That was added based on my confusion regarding the connection to #3069578: Layout builder doesn't show new "extra fields".

Updating the issue summary.

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
@@ -240,6 +240,30 @@ public function testAddHighlights() {
+   * Tests removing newly added extra field. See issue #3144010.

We usually don't reference issues like this, that info can be gotten from the git log/blame

Otherwise I think this is ready to go!

Here's a new patch with that small change to the test description.

Thanks!

Looks really good but some cspell issues - either needs the words splitting or added to the dictionary:

core/modules/layout_builder/tests/modules/layout_builder_extrafield_test/layout_builder_extrafield_test.module:14:25 - Unknown word (extrafield)
core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php:258:81 - Unknown word (fieldlayout)

Addressed comment #26, added extrafield and fieldlayout into dictionary.

If extrafield isn't already in the dictionary, there's no reason to add it for this. I should have put a space in that from the beginning. Here's a new patch that changes extrafield to extra_field. There's no way around adding fieldlayout to the dictionary since it comes from a programmatically created css class.

If reviewers prefer #27 though, that's fine.

#28 looks good to me, thanks!

Send retest as it failed

Splitting rather than adding to cspell makes sense to me as well.

Committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!

This seems to have caused regression #3207875: "Unsaved changes" message incorrectly appears on layout builder.