Layout Builder's Field Blocks do not work with Quick Edit

Looks great!

1. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +89,52 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   + * Implements hook_quickedit_render_field().
   

   Yay, this (weird!) API comes to the rescue again :)

2. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +89,52 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   +  list(, $delta, $component_uuid) = explode('-', $view_mode_id, 3);
   ...
   +            $component['content']['#view_mode'] = implode('-', ['layout_builder', $delta, $component_uuid]);
   

   The first array part is the module name. That's why it can be omitted when destructuring.

3. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -163,15 +163,9 @@ public function buildMultiple(array $entities) {
   -  protected function getRuntimeSections(FieldableEntityInterface $entity) {
   +  public function getRuntimeSections(FieldableEntityInterface $entity) {
   
   +++ b/core/modules/layout_builder/src/Entity/LayoutEntityDisplayInterface.php
   @@ -33,4 +34,15 @@ public function isOverridable();
   +  public function getRuntimeSections(FieldableEntityInterface $entity);
   

   API change!

4. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
   @@ -0,0 +1,123 @@
   +    $editor = Editor::create([
   ...
   +    $editor->save();
   

   Nit: no need for $editor, just do Editor::create([…])->save()?

5. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
   @@ -0,0 +1,123 @@
   +      'format' => 'full_html',
   

   Supernit: I think it's better to use something other than 'full_html'. Even some_format is better IMHO.

6. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
   @@ -0,0 +1,123 @@
   +      'create article content',
   

   I don't think this permission is necessary?

7. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
   @@ -0,0 +1,123 @@
   +    // Get the UUID of the default body field block.
   

   Nit: s/UUID/component UUID/

8. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
   @@ -0,0 +1,123 @@
   +    // To prevent 403s on save, we re-set our request (cookie) state.
   +    $this->prepareRequest();
   

   😱

9. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
   @@ -0,0 +1,123 @@
   +    $assert_session->pageTextContains('Hello world');
   +    $assert_session->elementExists('css', $field_selector);
   

   Hm, this check is not very strict. It could theoretically still allow the text to be rendered but the surrounding markup to be lost, couldn't it?

I did some manual testing with and without Layout Builder using this patch and the behaviours are the same. Nice work!

Nice work. I have tested this a bit and it looks good. I have also played a bit around with it and encountered few issues.

I have tested everything on default Drupal article.

Case 1:
I used following display layout: two one column sections. The top section contains only body field block and the section below is a default layout for the article (what we get after enabling layout builder).

When I use quick edit on any of two body fields, I get following error:

Uncaught Error: FieldModel cannot be destroyed if it is not inactive state.
    at n.destroy (FieldModel.js?v=8.6.0-dev:47)
    at renderField (AppView.js?v=8.6.0-dev:259)
    at n.renderUpdatedField (AppView.js?v=8.6.0-dev:277)
    ...

Changes are saved.
Edited body field block is re-rendered, but another body field block is not.

=> When I have single body field block in layout, it works properly. So I guess it's some issue with multiple uses of same field block in one layout.

Case 2:
It looks like, that quick edit works only for single column sections. If I have two column (or some other), then the Quick edit boxes are not enabled for field blocks inside it.

Case 3:
It's a bit edge case and tricky to reproduce. If I have enabled "Allow each content item to have its layout customized.", it's possible to reproduce this problem in following steps.

1. Go to view page of an article (fe. /node/2)
2. After that click "Layout" in the tabs to edit layout for that article
3. Remove all sections
4. Add new one column section -> add body field block into that section
5. Save Layout (you will be redirected to view page for that article)
6. Click "Quick edit" for that article

Uncaught TypeError: Drupal.quickedit.editors[editorName] is not a constructor
    at n.setupEditor (AppView.js?v=8.6.0-dev:141)
    at AppView.js?v=8.6.0-dev:44
    at Function.m.each.m.forEach (underscore.js:153)
    at n.each (backbone.js:87)
    at n.appStateChange (AppView.js?v=8.6.0-dev:43)
   ...

I'm not sure what is the problem. Looks like some temp store or some caching. Since it works after reload of the page. :(

Good feedback @mtodor!
Removing from sprint tag since @samuel.mortenson isn't actively working on this. Please feel free to retag if someone picks this up!

#8.1 Not sure whet ever this would work if you have a duplicated field rendered without using Layout Builder? If that's true, perhaps that could be a follow-up?

#8.2 This patch addresses this issue.

#8.3 I've created a new issue for this: https://www.drupal.org/project/drupal/issues/2966136

Setting "Needs Review" for tests

Looks good to me!

1. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +89,58 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   +  if ($entity instanceof FieldableEntityInterface) {
   

   I'm not sure we need this check; I don't think Quick Edit works with config entities at all, and I can't imagine that this hook would even be invoked for any situation except rendering fields on a content entity. So the very fact that we're implementing a Quick Edit hook implies that $entity is fieldable.

2. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +89,58 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   +      $contexts['layout_builder.entity'] = new Context(new ContextDefinition("entity:{$entity->getEntityTypeId()}", new TranslatableMarkup('@entity being viewed', ['@entity' => $entity->getEntityType()->getLabel()])), $entity);
   

   We should add a @todo to use EntityContext when #2932462: Add EntityContextDefinition for the 80% use case is fixed.

3. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +89,58 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   +            if (isset($component['content']) && isset($component['content']['#theme']) && $component['content']['#theme'] === 'field') {
   

   This line seems a little fragile, and I can't quite articulate the reasons why. Is there some other way we could do this? Could we get in front of Quick Edit's relevant theme/preprocess functions, or wrap around them? I dunno. Maybe we should open a follow-up to revisit this?

4. +++ b/core/modules/layout_builder/src/Entity/LayoutEntityDisplayInterface.php
   @@ -33,4 +34,15 @@ public function isOverridable();
   +  /**
   +   * Gets the runtime sections for a given entity.
   

   It'd be nice to expand this doc comment to explain what, exactly, a "runtime section" is.

5. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
   @@ -0,0 +1,121 @@
   +  public static $modules = [
   

   Should be protected.

6. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
   @@ -0,0 +1,121 @@
   +    $section = reset($sections);
   +    $component_uuid = array_keys($section->getComponents())[0];
   

   These lines can be collapsed into one: $component_uuid = key(reset($sections)->getComponents());

7. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
   @@ -0,0 +1,121 @@
   +    // To prevent 403s on save, we re-set our request (cookie) state.
   +    $this->prepareRequest();
   

   Can the comment be expanded to explain why 403s would happen if we *didn't* reset the request state?

NW for #12

re #12
1. I think we should leave the check in. I am not sure in which ways you can extend QuickEdit also hook_quickedit_render_field could have been made with it's first argument as FieldableEntityInterface but it was made with EntityInterface as the first argument so we should assume it could actually be called with just that. And out next line has a call to EntityViewDisplay::collectRenderDisplay() which expects a FieldableEntityInterface which would throw an exception if it didn't get one.
2. added @todo
3. I have changed to only take effect if $component['#base_plugin_id'] === 'field_block'. Since we really only care about this plugin.
4. Expanded to my understanding.
5. Looking at other tests this is always public.
6. Fixed
7. I removed this and the tested passed locally. Lets see if it does on DrupalCI.

I also change the test to extend WebDriverTestBase

The tag has been added to make the issue visible on the Kanban sprint board.

Need re-roll

1. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +89,60 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   +function layout_builder_quickedit_render_field(EntityInterface $entity, $field_name, $view_mode_id, $langcode) {
   

   Is actually being called? I am going to upload a patch without this running and the tests still pass.

2. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +89,60 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   +function layout_builder_entity_view_alter(array &$build, EntityInterface $entity, EntityViewDisplayInterface $display) {
   

   Since this patch another commit add layout_builder_entity_view_alter

   So to separate the logic of the 2 purposes for this hook now I have created _layout_builder_quick_edit_view_alter().

   The existing logic for extra fields have moved in the new static method \Drupal\layout_builder\Plugin\Block\ExtraFieldBlock::entityViewAlter() we were already calling a static method which I changed to private static method.

3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
   @@ -0,0 +1,117 @@
   +    $component_uuid = key(reset($sections)->getComponents());
   

   This only worked because body was the first field. Now that extra fields are working this is not true.

4. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
   @@ -0,0 +1,117 @@
   +    $this->click('.contextual-toolbar-tab button');
   +    $this->click($entity_selector . ' [data-contextual-id] > button');
   +    $this->click($entity_selector . ' [data-contextual-id] .quickedit > a');
   

   Adding waits before clicking these elements because I was getting fails locally

5. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
   @@ -0,0 +1,117 @@
   +    $assert_session->elementExists('css', 'p:contains("Hello world")');
   

   Also checking if exists after waits.

Rerolling #18

+++ b/core/modules/layout_builder/layout_builder.module
@@ -234,3 +215,79 @@ function layout_builder_layout_builder_section_storage_alter(array &$definitions
+      $section_list =$section_manager->findByContext('view', ['entity' => EntityContext::fromEntity($entity)]);
+      if (count($section_list) === 0) {
+        // @todo Do I have to do this extra call to findByContext()?
+        // It seem weird that the *manager* couldn't figure out by the entity has not overrides
+        // so the default storage should take over.
+        $section_list =$section_manager->findByContext('view', ['display' => EntityContext::fromEntity($view_display)]);
+      }

Is there a way around this? Or does this just make sense?

This should work:

$section_list = $section_manager->findByContext('view', [
  'entity' => EntityContext::fromEntity($entity),
  'display' => EntityContext::fromEntity($view_display),
]);

If it doesn't, that's due to a bug in OverridesSectionStorage that we're wrestling with currently in #2976148: Layout-based entity rendering should delegate to the correct section storage instead of hardcoding to either defaults or overrides.

Opened #3020677: Even when empty, Overrides returned instead of Defaults when using SectionStorageManagerInterface::findByContext() specifically to address this

Here is reroll and a fix using #23(slightly updated)

#25 failed because before the reroll on move all of layout_builder_entity_view_alter into \Drupal\layout_builder\Plugin\Block\ExtraFieldBlock::entityViewAlter() to separate it out.

but since #3007973: Layout builder prevents the rendering of extra fields (like Links) on pages not using Layout Builder changed layout_builder_entity_view_alter so it needed to be updated.

1. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -92,39 +98,10 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   -  // Only replace extra fields when Layout Builder has been used to alter the
   -  // build. See \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::buildMultiple().
   -  if (isset($build['_layout_builder']) && !Element::isEmpty($build['_layout_builder'])) {
   -    /** @var \Drupal\Core\Entity\EntityFieldManagerInterface $field_manager */
   -    $field_manager = \Drupal::service('entity_field.manager');
   -    $extra_fields = $field_manager->getExtraFields($entity->getEntityTypeId(), $entity->bundle());
   -    if (!empty($extra_fields['display'])) {
   -      foreach ($extra_fields['display'] as $field_name => $extra_field) {
   -        // If the extra field is not set replace with an empty array to avoid
   -        // the placeholder text from being rendered.
   -        $replacement = isset($build[$field_name]) ? $build[$field_name] : [];
   -        ExtraFieldBlock::replaceFieldPlaceholder($build, $replacement, $field_name);
   -        // After the rendered field in $build has been copied over to the
   -        // ExtraFieldBlock block we must remove it from its original location or
   -        // else it will be rendered twice.
   -        unset($build[$field_name]);
   -      }
   -    }
   -  }
   ...
   +  ExtraFieldBlock::entityViewAlter($build, $entity);
   

   This makes sense and is a good refactor, but I don't see this as being in scope

2. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -268,3 +245,83 @@ function layout_builder_plugin_filter_layout__layout_builder_alter(array &$defin
   +  foreach (Element::children($build['_layout_builder']) as $delta => &$section) {
   +    $section = &$build['_layout_builder'][$delta];
   

   this is very confusing, and I think incorrect?

   shouldn't it end with as $delta since the switch to Element::children, and then the next line makes more sense.

3. +++ b/core/modules/layout_builder/src/Plugin/Block/ExtraFieldBlock.php
   @@ -158,7 +159,7 @@ public function getPreviewFallbackString() {
   -  public static function replaceFieldPlaceholder(array &$build, array $built_field, $field_name) {
   +  private static function replaceFieldPlaceholder(array &$build, array $built_field, $field_name) {
   

   I mean, I'm all for it. Wish we'd done it the first time. But this was in 8.6.0, so idk that we can break it :(

4. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
   @@ -0,0 +1,141 @@
   +    // @todo Test with an override.
   

   is this a todo to be resolved before commit? if not, needs an issue

5. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
   @@ -0,0 +1,141 @@
   +    foreach (reset($sections)->getComponents() as $component) {
   +      if ($component->getPlugin()->getPluginId() === 'field_block:node:article:body') {
   +        break;
   

   I feel like this is copied from somewhere, maybe deserves a helper?

6. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
   @@ -0,0 +1,141 @@
   +    };
   

   extraneous ;

@tim.plunkett thanks for the review.

1. Removed. Chatted with @tim.plunkett about this more. I wanted to move the extra field logic to its own method because layout_builder_entity_view_alter use to be only this logic and now it is two separate things. But I agree this is out of scope.

   So instead I made new QuickEditIntegration class that will have 2 methods for the 2 hooks that need logic for quick edit. I think this is cleaner.

2. Fixed. It was only working because the deltas are numeric values so the keys and values where the same, [0=>0, 1=> 1]
3. removed
4. Yes. we should do it in this issue. Should be able to use all the same code just different setup.
5. unaddressed here.
6. fixed

Also because I can see the chrome browser for the javascript test I can see that quick is actually getting an error but the test is still passing. Something is wrong with the test.
To confirm this this patch throws an exception in \Drupal\layout_builder\QuickEditIntegration::quickEditRenderField() and I think the test will still pass.

I think we should actually extend \Drupal\Tests\quickedit\FunctionalJavascript\QuickEditJavascriptTestBase because there are a lot of helper methods in there. But don't have time right now.

I debug a little and the problem is in \Drupal\quickedit\QuickEditController::entitySave

// Take the entity from PrivateTempStore and save in entity storage.
    // fieldForm() ensures that the PrivateTempStore copy exists ahead.
    $tempstore = $this->tempStoreFactory->get('quickedit');
    $tempstore->get($entity->uuid())->save();

$tempstore->get($entity->uuid()) returns a NULL for some reason. I have test manually though and I can get quick edit to work with this patch. So I think it is a testing issue and hopefully using QuickEditJavascriptTestBase will allow us to fix this.

Here I have make \Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderQuickEditTest extend \Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest

This makes sure that the test coverage for nodes and custom blocks that QuickEdit module provides will also pass when the displays are using the Layout builder. This required 2 small changes to in QuickEdit test code.

1. Create and use QuickEditJavascriptTestBase::getQuickEditFieldId() because the attribute used for Quick edit fields depends on the view_mode which is different using Layout Builder.
2. Move the create of the custom block content type out of testCustomBlock() and into the setup.

Thanks for the patch. I tested #31 and it's not working for me so maybe I'm doing something wrong. Here's what I did:

1. Enabled layout modules
2. Configured page content type for layout
3. Created a page node and added a section and blocks
4. Clicked on Quick Edit mode
5. Title is shown as editable but not the rest of the content
6. Reverted to defaults in layout builder and then title and body were editable with quick edit

Putting back to "Needs work" based on this.

Quick edit only works for title (with layout):

Quick edit only works for title and body (without layout):

@Kristen Pol thanks for testing the screenshots. I think the problem wasn't that you have override vs defaults. I think if you added same fields to the default it would have the same bug.

The problem I think stems from \Drupal\quickedit\MetadataGenerator::generateFieldMetadata()

$formatter_id = EntityViewDisplay::collectRenderDisplay($entity, $view_mode)->getRenderer($field_name)->getPluginId();

This gets an error because ->getRenderer($field_name) will return a null because \Drupal\Core\Entity\Entity\EntityViewDisplay::getRenderer has

if (($configuration = $this->getComponent($field_name)) && isset($configuration['type']) && ($definition = $this->getFieldDefinition($field_name))) {

But if we look at

public function getComponent($name) {
    return isset($this->content[$name]) ? $this->content[$name] : NULL;
  }

This still just getting looking at content which is not Layout builder specific.
So if you field was not being displayed before you turned on Layout Builder then it will not be returned. Also some fields were not displayable at all without layout Builder.

In the quick edit case because of the other changes in the patch that create a unique view mode with both the section delta and the component uuid we can actually figure out the component field settings.

I am not sure what to do in other cases and not sure why this doesn't cause other problems. We could always figure out the field formatter settings for the field block if the field is display but then also the field could be displayed twice.

Only testing layout_builder in this patch.

change to drupalci.yml

removed drupalci changes

The fail in \Drupal\Tests\config\Functional\ConfigImportAllTest::testInstallUninstall() does seem related.

Changing to use parent::getFieldDefinitions() unless the display is for quickedit.

1. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -465,4 +465,68 @@ private function sectionStorageManager() {
   +    $component = parent::getComponent($name);
   +    if (empty($component)) {
   +      return $this->getQuickEditFormatterSettings();
   +    }
   

   We should only be calling getQuickEditFormatterSettings() if $this->isLayoutBuilderEnabled()

   And then if we can't tell the component via quickedit we should just loop through all sections and components and find the field block. Otherwise will be getting the settings from $this->contents() which should only be backup if the field is not in a component.

2. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -465,4 +465,68 @@ private function sectionStorageManager() {
   +    if (empty($this->getQuickEditFormatterSettings())) {
   

   we should base this logic on $this->isLayoutBuilderEnabled() also

Some refactoring and adding testing of a layout override using quickedit.

.... and the patch

I figured out what is causing the test failure in \Drupal\Tests\config\Functional\ConfigImportAllTest::testInstallUninstall(). I'll provide a patch tomorrow with that fix+ the additional tests requested in the Todo. Mentioning it now on the off chance someone was planning to tackle this in the next 12 hrs.

Since this needs work, here are a few more things that could be updated at the same time. Thanks.

1. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -269,3 +274,14 @@ function layout_builder_plugin_filter_layout__layout_builder_alter(array &$defin
   +
   +
   

   Nitpick: Extra empty line.

2. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -465,4 +467,86 @@ private function sectionStorageManager() {
   +   *
   +   * @return array |null
   

   Nitpick: Extra space before pipe.

3. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -465,4 +467,86 @@ private function sectionStorageManager() {
   +
   +  private function getSectionComponentForFieldName($name) {
   

   Missing doc block.

4. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
   @@ -0,0 +1,158 @@
   +   * that modules identify themselves with a view mode in the format module
   +   * name-id.
   

   "module name-id" isn't clear to me. Is it supposed to mean:

   [module-name]-[id]?

5. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
   @@ -0,0 +1,184 @@
   +    // @todo Test publised, title or other field that does appear in manage dispaly.
   

   Typos: publised and dispaly. And, needs an Oxford comma.

6. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
   @@ -0,0 +1,184 @@
   +  public function provideTestArticleNode() {
   

   Needs doc block.

7. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
   @@ -0,0 +1,184 @@
   +  }
   +
   +
   +  /**
   

   Nitpick: Extra empty space.

ok I have done some work on this that I meant to post earlier.

@Kristen Pol thanks for review but I haven't had time to address it yet. I will though

1. Added patch from #3026434-59: Ensure that Layout Builder Inline Blocks doesn't assume section storage internals because this includes some helper functions that will make work here easier. I will include a patch without these changes.
2. The previous patch didn't not take into account overrides. The LayoutViewDisplay can only know about the defaults.
   

   +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
   @@ -0,0 +1,158 @@
   +              $component['content']['#view_mode'] = implode('-', [
   +                'layout_builder',
   +                $delta,
   +                $component_uuid,
   +              ]);
   

   I changed this to also pass the entity_id and revision_id(if any) because I am not sure how else we are going to get the information about overrides which is stored as a field value.

3. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
   @@ -0,0 +1,158 @@
   + if (isset($component['content']) && isset($component['#base_plugin_id']) && $component['#base_plugin_id'] === 'field_block') {
   

   Made new private method here \Drupal\layout_builder\QuickEditIntegration::supportQuickEditOnComponent() that determines if we should support a component.

   The extra check I added in the method was not allow quickedit on non-view configurable fields.

4. Added a possible fix for \Drupal\Tests\config\Functional\ConfigImportAllTest::testInstallUninstall() fail.
   Which was a change to layout_builder_install to check <code>!empty($field_layout['settings'])

   This came from @bnjmnm. If remember correctly this is what he was talking in #45

Other things remaining

1. Another thing I am wonder about is if we will need some changes directly to the QuickEdit module. It look like QuickEdit js it caches quickedit field information on the client localstorage. It probably assumes it can cache field information until there is a change to the EntityViewDisplay but now with overrides the fields and formatters could change on the displayed entity level.

Let me start with: great progress here, and nice job overall! Now let's figure out the remaining tricky bits :) 💪

Another thing I am wonder about is if we will need some changes directly to the QuickEdit module. It look like QuickEdit js it caches quickedit field information on the client localstorage. It probably assumes it can cache field information until there is a change to the EntityViewDisplay but now with overrides the fields and formatters could change on the displayed entity level.

Thanks for thinking about that! 🙂 🙏

It's been years since I last looked at this, so I had to dig in to the code to answer this too. Remarks:

1. Most importantly: the metadata is cached per entity, so per-entity overrides will be respected just fine. (IIRC it did this because entity and field access access may be different for each individual entity of the same type and bundle.)
2. Quick Edit uses sessionStorage, not localStorage. So: the mere act of closing a tab actually already clears this cache, and the cache is per-tab.
3. Quick Edit assumes it can cache this forever on the client; only permissions changes trigger an explicit invalidation. This was considered a reasonable compromise: it is guaranteed to be safe, but it may be stale for non-security-sensitive aspects. The staleness was considered a lesser concern since sessionStorage is used (persistent for the life of a browser tab) instead of localStorage (persistent across browser tabs, browser windows and sometimes even browser sessions).
4. A consequence is that if the layout changes for a given entity, that Quick Edit will continue to use the same client-side cached metadata. Unless of course the "view mode ID" and hence the data-quickedit-field-id attribute in the HTML change when the layout changes, since that would cause a cache miss. If this is not the case, I'm happy to look into how Quick Edit can accommodate this.
5. @johndevman created an issue that relates to Quick Edit's client-side caching of metadata in #10: #2966136: Quick Edit assumes that all fields meta data are stored in cache or none.

#47.2: I think this makes perfect sense. I'd love to see test coverage for this: 1) Quick Edit triggered without Layout Builder enabled for a node type, 2) enable it for a node type, verify that Quick Edit still works, 3) override the layout in a forward revision and verify that once again Quick Edit works. I think that would cover all edge cases, and would also prove that the aforementioned client-side caching isn't getting in the way.

Patch review below. Some things weren't clear to me, a comment (and perhaps patch) addressing some of my questions would allow me to review some of the deeper bits that I couldn't get to due to the things that I didn't understand.

1. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -140,6 +141,10 @@ function layout_builder_entity_view_alter(array &$build, EntityInterface $entity
   +  $quickEditIntegration = \Drupal::classResolver(QuickEditIntegration::class);
   
   @@ -311,3 +316,12 @@ function layout_builder_system_breadcrumb_alter(Breadcrumb &$breadcrumb, RouteMa
   +  $quickEditIntegration = \Drupal::classResolver(QuickEditIntegration::class);
   

   I think this should adopt the pattern used by content_moderation.module, where an actual service is retrieved.

   Übernit: in .module files we don't use camelCase.

2. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
   @@ -0,0 +1,186 @@
   + * Helper methods for QuickEdit module integration.
   ...
   +class QuickEditIntegration implements ContainerInjectionInterface {
   ...
   +  public function __construct(SectionStorageManagerInterface $section_storage_manager, ContextRepositoryInterface $context_repository) {
   +    $this->sectionStorageManager = $section_storage_manager;
   +    $this->contextRepository = $context_repository;
   +  }
   ...
   +  public static function create(ContainerInterface $container) {
   +    return new static(
   +      $container->get('plugin.manager.layout_builder.section_storage'),
   +      $container->get('context.repository')
   +    );
   +  }
   

   This is not just a set of helper methods anymore, because it relies on services getting injected. Let's turn this into a proper service? (Also see point 1.)

3. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
   @@ -0,0 +1,186 @@
   +              if ($entity instanceof RevisionableInterface) {
   +                $component['content']['#view_mode'] .= ':' . $entity->getRevisionId();
   +              }
   

   I've done this too, but … RevisionableInterface is a lie.

   All content entities implement \Drupal\Core\Entity\ContentEntityInterface, and that implements RevisionableInterface. You either need to $entity->getEntityType()->isRevisionable(), or, in this case, you could get away with !$entity->isDefaultRevision().

4. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
   @@ -0,0 +1,186 @@
   +  public function quickEditRenderField(EntityInterface $entity, $field_name, $view_mode_id, $langcode) {
   ...
   +    list(, $delta, $component_uuid) = explode('-', $view_mode_id, 3);
   ...
   +          'view_mode' => new Context(new ContextDefinition('string'), $view_mode_id),
   

   I doubt this works correctly, because it's a still a "Quick Edit view mode ID", not a "Entity API view mode ID".

   (Yes, this API is frustratingly weird, I apologize.)

5. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
   @@ -0,0 +1,186 @@
   +      $cacheableMetaData = new CacheableMetadata();
   

   Another 🐫 spotted 🕵️‍♂️

6. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
   @@ -0,0 +1,186 @@
   +      $section_list = $this->sectionStorageManager->findByContext(
   +        [
   ...
   +      $contexts['layout_builder.entity'] = new Context(new ContextDefinition("entity:{$entity->getEntityTypeId()}", new TranslatableMarkup('@entity being viewed', [
   +        '@entity' => $entity->getEntityType()
   +          ->getLabel(),
   +      ])), $entity);
   

   🔍🐛 Some weird indentation here, surprisingly it doesn't trigger PHPCS.

7. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
   @@ -0,0 +1,186 @@
   +   * Determines whether a component should have QuickEdit support.
   ...
   +   * Only field_blocks components for view configurable fields should be
   ...
   +   *   Whether QuickEdit should be supported on the component.
   

   s/should be/are/
   s/field_blocks/field_block/
   s/QuickEdit/Quick Edit/

8. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
   @@ -0,0 +1,186 @@
   +  private function supportQuickEditOnComponent($component, FieldableEntityInterface $entity) {
   

   Could use an @see \Drupal\layout_builder\Plugin\Block\FieldBlock.

9. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
   @@ -0,0 +1,185 @@
   +    $user = $this->loggedInUser;
   ...
   +    $this->drupalLogin($user);
   

   Nit: this is the only place it's used, it's IMHO unnecessarily confusing to alias it to a local variable.

10. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
    @@ -0,0 +1,185 @@
    +    // @todo Test publised, title or other field that does appear in manage dispaly.
    

    This is a "todo of intent", should be completed still, but sounds good!

11. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
    @@ -0,0 +1,185 @@
    +      list($entity_type, $entity_id, $field_name, , $view_mode) = explode('/', $field_key);
    

    , , $view_mode, this is confusing. What's the rationale? Can we centralize the logic?

12. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
    @@ -0,0 +1,185 @@
    +   * @param string $field_name
    +   *   The field name.
    ...
    +  protected function getLayoutBuilderViewMode($entity_type, $entity_id, $field_name, $view_mode) {
    

    The fourth parameter is not documented.

13. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
    @@ -0,0 +1,185 @@
    +    if (in_array($field_name, ['title', 'uid', 'created'])) {
    +      return $view_mode;
    +    }
    

    Why are we special-casing these three fields?

14. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
    @@ -0,0 +1,185 @@
    +    // Find the body field component.
    +    foreach (reset($sections)->getComponents() as $component) {
    +      if ($component->getPlugin()->getPluginId() === "field_block:$entity_type:{$entity->bundle()}:$field_name") {
    +        // Hard code entity id and revision id.
    +        return 'layout_builder:0:' . $component->getUuid() . ":1:1";
    +      }
    +    }
    

    I don't understand what this is doing. The comment mentions body, but the code does not. And why is it okay to hardcode entity and revision ID?

15. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
    @@ -26,8 +26,12 @@ class LayoutBuilderTest extends WebDriverTestBase {
    +  protected $profile = 'standard';
    

    Why do we need to install standard? I suspect this is just a temporary thing, to make it easier to get the tests to a passing, complete state? I think that'd be a great strategy, but we should have a @todo to remove this prior to commit.

@Wim Leers Thanks for the review!

Re test coverage that sounds like good idea!

1. I am not sure here if you were referring \Drupal::classResolver() vs \Drupal::service('class_resolver')

   or actually use a service like content_moderation.module does in a couple cases Drupal::service('content_moderation.moderation_information').

   since #2949965: \Drupal::classResolver() could be more helpful was done in the last year I thought that is what I should be using.(which does use the service internally.

   Fixing the camel case.

2. ok. I see know. I am going to punt on this for now. Not opposed to doing it just not sure when we should be using service versus just a class that uses ContainerInjectionInterface
   Especially since \Drupal\Core\DependencyInjection\ClassResolver::getInstanceFromDefinition explicitly handles this interface. Is not use a service a further message that this class is not for reuses(+ it being @internal)?

   this is similar to \Drupal\content_moderation\EntityTypeInfo which also is retrieved via classResolver and injects services from the container.

3. Changing to $entity->getEntityType()->isRevisionable(). Not sure how !$entity->isDefaultRevision() would work. Couldn't you be editing the default revision?
4. Hmm. Right now we pass the view_mode as a context but it is ignored for finding it. We just have it there so that in the future we could use it as selection process(or in contrib).
   Do we need to add another context 'quick_edit_view_mode'? Technically I think you could find the correct entity view mode from this info because you have the uuid id of the component which should be unique if we did allow overrides of other view modes.

   No worries about the API weirdness, it happens.

5. 🐫 fixed.
6. I cleaned this indentation up.
7. fixed with slightly different wording
8. Added @see
9. This is to save the user that was logged in.
   added comment // Save the current user to re-login after Layout Builder changes.
   added this in the other case in the file where this is happening.
10. Actually now that \Drupal\layout_builder\QuickEditIntegration::supportQuickEditOnComponent() is excluding these fields I don't think we need the todo. Removed.
    The title field when displayed outside of the layout will still be tested because we are extending \Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest
11. Added a comment

          // Extract from $field_key all of the information we need to call
          // getQuickEditFieldId(). The fourth part of $field_key, language code, is
          // not needed so it can be ignored.
    

    I don't think there is anything we can centralize here. replaceLayoutBuilderFieldIdKeys() is already called by both assertEntityInstanceFieldStates() and replaceLayoutBuilderFieldIdKeys()

12. added @param
13. Added explanation comment.

        // If the field is one that is not rendered by Layout Builder do not change
        // $view_mode.
    

14. Actually it is NOT safe to hardcode this. Fixes 1 fail for overrides. Though test still failing.

    Fixed the comment

        // Find the component with the plugin ID in the field_block format that
        // matches the entity type, bundle, and field name.
    

15. Sorry all the changes to this file were trying help me debug. Removing all changes

The test are still failing

Here is screen from when the test fail

at

// Wait for the form to load.
    $this->assertJsCondition('document.querySelector(\'.quickedit-form-container > .quickedit-form[role="dialog"] > .placeholder\') === null');

it seems like placeholder it is not being removed and there is an ajax response.selector that is not valid.

Investigating

1. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
   @@ -0,0 +1,189 @@
   +              $component['content']['#view_mode'] = implode(':', [
   +                'layout_builder',
   +                $delta,
   +                $component_uuid,
   +                $entity->id(),
   +              ]);
   +              if ($entity->getEntityType()->isRevisionable()) {
   +                $component['content']['#view_mode'] .= ':' . $entity->getRevisionId();
   +              }
   +            }
   

   I think part of the problem is using the ":" instead of "-" and that not working with QuickEdit js.

   Change this fixes the test for me. I did this because the UUID has dashes in it so we couldn't add entity id after this. So just changing the UUID to use "_" in the view_mode string which will then be changed back on extraction.

2. In #50 I forgot to mention the patch include the patch from #3026434: Ensure that Layout Builder Inline Blocks doesn't assume section storage internals. I am including an updated version in this patch which should fix test fail in ResolvedLibraryDefinitionsFilesMatchTest

   will include a do not test patch this time. also the interdiff doesn't include changes in #3026434

3. I haven't looked at fail in LayoutBuilderFieldLayoutCompatibilityTest yet.

1. Ok I realized we don't actually need the changes in #3026434: Ensure that Layout Builder Inline Blocks doesn't assume section storage internals. Earlier in this patch it might have needed the updates LayoutEntityHelperTrait but not anymore. Removing

2. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
   @@ -0,0 +1,194 @@
   +              if ($entity->getEntityType()->isRevisionable()) {
   +                $component['content']['#view_mode'] .= '-' . $entity->getRevisionId();
   +              }
   

   I realized from @Wim Leers' comments that we can't actually put the revision id in the view_mode. Because the field info is cached for the entity and doesn't take into account revisions.

   I manually test this and quick edit doesn't work on the same node again after you save it.

   So I removed this we can still load the correct revision because QuickEdit doesn't support pending revisions.
   #2903524: Don't add quickedit to displays where entities have a forward revision.

3. I changed the node test to simply run the quick testing 2x on the same which now should work. This needed some more refactoring in \Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest. I am not sure if we should just be copying this test but it seems good they are extending it so are sure what ever is tested with quickedit is test with Layout Builder and quickedit.
4. From @Wim Leers in #49

   I'd love to see test coverage for this: 1) Quick Edit triggered without Layout Builder enabled for a node type, 2) enable it for a node type, verify that Quick Edit still works, 3) override the layout in a forward revision and verify that once again Quick Edit works.

   This sadly will not work at least from step 1 to step 2 right now. My understand the field information which contains the (quickedit)view_mode will be cached on the client so when you enable layout builder it will not trigger the client cache to be cleared.

   I am not exactly sure yet how to fix this. I know quickedit uses metadata but I am sure if this is used vary the client cache like it is Context Link client storage.
   In the Contextual module you add information to the contextual metadata it will keep a separate version of the links.

   Is something like this possible with Quickedit?

Looking at quickedit_entity_view_alter()

it sets
$build['#attributes']['data-quickedit-entity-id'] = $entity->getEntityTypeId() . '/' . $entity->id();

I am wondering if we were to use our own hook_entity_view_alter to change this when layout builder is enable for a bundle would that allow clearing the fields on the client storage.

On caveat to that is we would have to get layout_builder_entity_view_alter to run after quickedit_entity_view_alter to alter it.

We are actually already implementing layout_builder_module_implements_alter to make our implementation runs last. But I don't think is actually having any effect.

I could be wrong but .....

This comes from \Drupal\Core\Entity\EntityViewBuilder::buildMultiple():

// Allow modules to modify the render array.
$this->moduleHandler()->alter([$view_hook, 'entity_view'], $build_list[$key], $entity, $display);

where $view_hook in "[ENTITY_TYPE}_view".

Reading through the docs for hook_module_implements_alter

* Note that hooks invoked using \Drupal::moduleHandler->alter() can have
 * multiple variations(such as hook_form_alter() and hook_form_FORM_ID_alter()).
 * \Drupal::moduleHandler->alter() will call all such variants defined by a
 * single module in turn. For the purposes of hook_module_implements_alter(),
 * these variants are treated as a single hook. Thus, to ensure that your
 * implementation of hook_form_FORM_ID_alter() is called at the right time,
 * you will have to change the order of hook_form_alter() implementation in
 * hook_module_implements_alter().

So actually changing the order of 'entity_view_alter' actually has no effect.

because QuickEdit doesn't support pending revisions.

This reminds me of #2910005: JavaScript errors thrown when viewing non-latest default revision of entity — indeed, Quick Edit doesn't support this at all! And for good reason. Thanks for also linking #2903524: Don't add quickedit to displays where entities have a forward revision..

Will review #50–#57 next, but @tedbow just told me he found a solution, so waiting for that to land first :)

1. The code will need clean up but I hope the solution concept will be acceptable
2. The main problem is that the quickedit metadata that is stored on client local storage will be invalid more than QuickEdit currently excepts it to be.
3. For instance when layout builder is enabled for bundle, when the defaults for the default blocks change(add new, delete or change settings) or when an override is created or changed this could all affect the QuickEdit metadata for fields.
4. So basically when even of that happens we need to clear the QuickEdit metadata stored on the clients browser for that entity.(hopefully not for all entities)

I will attach the patch now and then do a self-review to explain the fix.

1. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -291,9 +291,22 @@ protected function buildSections(FieldableEntityInterface $entity) {
   +        if (\Drupal::currentUser()->hasPermission('access in-place editing')) {
   +          // If the current user can access QuickEdit then set a hash of the
   +          // sections to clear QuickEdit metadata on the client when it changes.
   +          $sections_hash = hash('sha256', serialize($sections));
   +          $build['#attached']['drupalSettings']['layout_builder']['section_hashes'][$entity->getEntityTypeId() . ':' . $entity->id() . ':' . $this->mode] = [
   +            'hash' => $sections_hash,
   +            'quickedit_storage_prefix' => $entity->getEntityTypeId() . '/' . $entity->id(),
   +          ];
   +          $build['#attached']['library'][] = 'layout_builder/drupal.layout_builder_quickedit';
   +        }
   

   Here we attach a hash of the current sections for any entity that is rendered by this module.

   This will be used in Javascript to compare it to the previous section hash if any and remove any metadata for the entity that quickedit has stored on the client if the hash is not the same or isn't present.

   We attach our new drupal.layout_builder_quickedit. The previous JS library is only need on the admin site where as this will be needed when the entities are being viewed.

   We only do any of this if the user has access to quickedit.

2. +++ b/core/modules/layout_builder/js/layout-builder-quickedit.es6.js
   @@ -0,0 +1,39 @@
   +  Object.keys(sectionHashes).forEach(sectionHashKey => {
   +    const sectionHash = sectionHashes[sectionHashKey];
   +    const hashStorageKey = _prefixSectionHashKey(sectionHashKey);
   +    const storedSectionHash = storage.getItem(hashStorageKey);
   +    if (!storedSectionHash || storedSectionHash !== sectionHash.hash) {
   +      // The stored hash does not match. Clear QuickEdit metadata.
   +      Object.keys(storage).forEach(storageKey => {
   +        if (
   +          storageKey.indexOf(
   +            `Drupal.quickedit.metadata.${sectionHash.quickedit_storage_prefix}`,
   +          ) === 0
   +        ) {
   +          storage.removeItem(storageKey);
   +        }
   +      });
   +      // Store the update sections hash.
   +      storage.setItem(hashStorageKey, sectionHash.hash);
   

   Here will loop through any section hashes that where set on the server.

   If section hash for this entity/view_mode either does not exist or is different that than the previous one stored we delete any metadata for this particular entity that quickedit has stored.

   This will force quick edit to refetch info whenever layout builder is enabled for an entity(the hash wasn't there at all) or the sections have been somehow changed(the hash is different).

   UPDATE: I just realized that this logic will only run when layout builder has rendered the entity so it does not cover the case where layout builder was enabled previously for an entity but then has disabled.

   To solve this in \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::buildSections()

   when
   $storage = $this->sectionStorageManager()->findByContext($contexts, $cacheability);
   is empty we probably need to set a JS var that there is not hash so we can still clear metadata. This should still only clear the metadata once when the layout builder is disabled not everytime after this.

   Will add this logic and test coverage.

3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
   @@ -58,18 +54,48 @@ protected function setUp() {
   +    $this->enableOverridesAtAdminPath('admin/structure/types/manage/article/display/default');
   +    $this->usingLayoutBuilder = TRUE;
   +    // Test article with Layout Builder enabled.
   +    $this->doTestArticle($node);
   +    $this->resetEditToolbar();
   +    $this->createLayoutOverride('node/' . $node->id() . '/layout');
   +    // Test article with Layout Builder override.
        $this->doTestArticle($node);
   

   Here is the new test coverage that uses quickedit on the same article in this order

   1. With layout builder disabled
   2. with it enabled but no override
   3. with an override

   We need to add another case at the end where layout builder is again disabled for the bundle( we will have to revert the override first)

4. +++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditJavascriptTestBase.php
   @@ -163,7 +163,9 @@ protected function assertQuickEditEntityToolbar($expected_entity_label, $expecte
   -    $this->assertSame($expected_entity_label, $this->getSession()->evaluateScript("return window.jQuery('#quickedit-entity-toolbar .quickedit-toolbar-label').clone().children().remove().end().text();"));
   +    // @todo this Line is comment out in current patch because of quickedit label
   +    //   bug https://www.drupal.org/node/2914826
   +    //$this->assertSame($expected_entity_label, $this->getSession()->evaluateScript("return window.jQuery('#quickedit-entity-toolbar .quickedit-toolbar-label').clone().children().remove().end().text();"));
   

   I commented out this line because of the bug here #2914826: Drupal.quickedit.metadata stores entity label in window.sessionStorage, does not update after entity label changed. The current quick edit test coverage does not hit this because it does NOT edit the same entity again after the updating the label.

   Until we fix this we might need some kind of flag to skip this known bug on subsequent edits of the same entity.

5. +++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditIntegrationTest.php
   @@ -124,6 +125,8 @@ protected function setUp() {
      public function testArticleNode() {
        $node = $this->createNodeWithTerm();
        $this->doTestArticle($node);
   +    $this->resetEditToolbar();
   +    $this->doTestArticle($node);
   

   I have updated QuickEdit's own test again to have run the same test case on the entity after it is update once.

   If I hadn't commented the line related to #2914826: Drupal.quickedit.metadata stores entity label in window.sessionStorage, does not update after entity label changed this would fail also.

6. +++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditIntegrationTest.php
   @@ -366,4 +371,28 @@ protected function createNodeWithTerm() {
   +  protected function resetEditToolbar() {
   +    $this->click('#toolbar-bar .contextual-toolbar-tab button');
   +    $contextual_links_trigger_selector = '[data-contextual-id] > .trigger';
   +    $this->waitForNoElement("$contextual_links_trigger_selector:not(.visually-hidden)");
   +  }
   

   The quickedit tests expect contextual "Edit" mode not be to enabled when the test starts off so I made this method to reset it so we can test the same entity multiple times.

7. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTestOld.php
   @@ -13,7 +13,7 @@
   -class LayoutBuilderQuickEditTest extends QuickEditIntegrationTest {
   +class LayoutBuilderQuickEditTestOld extends QuickEditIntegrationTest {
   

   I kept the previous test coverage in a temporary old file because I want to look at again and make sure we are still covering everything we were before the tests were refactored. At that point I will delete this from the patch.

Adding related issue #2914826: Drupal.quickedit.metadata stores entity label in window.sessionStorage, does not update after entity label changed.

This issue was already related #2966136: Quick Edit assumes that all fields meta data are stored in cache or none but may be solved at least for Layout builder by this patch.

In that issue @johndevman mentions as possible solutions

1. Implement some API to let us purge the meta data cache
2. Or make sure that meta data items length match with the fields rendered

I think 1) here would make this patch not have to assume how quickedit stores metadata on the client.

For instance if we had a new Javascript method
Drupal.quickEdit.clearCacheForEntity(entity_type, entity_id);
Then the Javascript in this patch could be simplified.

1. This patch adds test coverage for the case when the layout builder is enabled for a bundle, quickedit is used on entity and then layout builder is disabled.
2. This will fail now because the quickedit metadata storage is only cleared when layout builder is enabled and then when the sections are update. NOT when layout builder is disabled for a bundle.
3. Working the fix now but wanted to first demonstrate the failure.

fix for test coverage in #63

Having trouble debugging the one failure, but reverting the change to layout_builder_install for now to fix the other test.

Posting a bit more work here, but not making much progress on the QE bugs. mostly focused on the other fails.

@tim.plunkett thanks for the fixes for the other tests.

This should fix LayoutBuilderQuickEditTestOld.

Adding layout_builder_test_css_transitions module to tests. Because they are passing locally for me. This may fix it.

I think #72 is error to do with timing. This passes on my local.

Trying a 50x test see if it randomly passes on Drpualci. If so I will try to figure out what the timing issue is.

I have banging my head against this I don't think we can extend \Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest()

That test only edits the same node once. I have tried to alter it to see if it can handle editing the same node twice without layout builder involved and it fails randomly. see #3037436: [random test failure] Make QuickEditIntegrationTest more robust and fail proof

I am not sure why but the test doesn't fail all the time.

Is there a simpler way to test this?

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTestOld.php
@@ -0,0 +1,203 @@
+class LayoutBuilderQuickEditTestOld extends QuickEditIntegrationTest {

This patch gets rid of this class bring the test method into the new class.

I still think this the more comprehensive test will fail on DrupalCI because it attempts to edit the same node more than once

I talked with @xjm and @tim.plunkett about the random test failures in Quick Edit re #3037436: [random test failure] Make QuickEditIntegrationTest more robust and fail proof

We agreed that good test coverage for Layout Builder/quickedit integration until this gets would be"

1. Create a test that uses Layout Builder + QuickEdit with and without overrides. This would only test the same node 1x. This test could test actually using and saving via quickedit
2. Create a test that tests Layout Builder + QuickEdit in the workflow on a single node:
   1. not using layout builder on bundle
   2. enable layout builder
   3. enable override
   4. revert override
   5. disable layout builder on bundle

   This test because of random failures will not work on completely but we should able to confirm that quick edit at least initializes correctly.

Test coverage as described by #80

Here is test-only version to prove it doesn't work with fix.

#85 was an expected test only fail.

Tests look good enough considering, thanks for the followup!

1. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -264,8 +269,19 @@ public function buildMultiple(array $entities) {
   +      else {
   +        // Set variable indicate we are not using the Layout Builder. If the
   +        // Layout Builder was previously enable for this entity the QuickEdit
   +        // metadata will need to be clear on the client.
   +        $build['#attached']['drupalSettings']['layout_builder']['section_hashes'][$entity->getEntityTypeId() . ':' . $entity->id() . ':' . $this->mode] = [
   +          'hash' => 'no_sections',
   +          'quickedit_storage_prefix' => $entity->getEntityTypeId() . '/' . $entity->id(),
   +        ];
   +      }
   +      $build_list[$id]['#attached']['library'][] = 'layout_builder/drupal.layout_builder_quickedit';
   

   This should be able to live in layout_builder_entity_view_alter()

2. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -291,9 +307,21 @@ protected function buildSections(FieldableEntityInterface $entity) {
   +        if (\Drupal::currentUser()->hasPermission('access in-place editing')) {
   +          // If the current user can access QuickEdit then set a hash of the
   +          // sections to clear QuickEdit metadata on the client when it changes.
   +          $sections_hash = hash('sha256', serialize($sections));
   +          $build['#attached']['drupalSettings']['layout_builder']['section_hashes'][$entity->getEntityTypeId() . ':' . $entity->id() . ':' . $this->mode] = [
   +            'hash' => $sections_hash,
   +            'quickedit_storage_prefix' => $entity->getEntityTypeId() . '/' . $entity->id(),
   +          ];
   +        }
   

   Possibly this too? Not sure, but ideally

3. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -470,4 +498,71 @@ private function sectionStorageManager() {
   +   * @return array |null
   

   Extra space

4. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -470,4 +498,71 @@ private function sectionStorageManager() {
   +   *   The QuickEdit formatter settings.
   

   Is there somewhere this can point to?

5. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -470,4 +498,71 @@ private function sectionStorageManager() {
   +   * @see \Drupal\layout_builder\QuickEditIntegration::entityViewAlter
   

   Missing ()

6. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -470,4 +498,71 @@ private function sectionStorageManager() {
   +  private function getQuickEditSectionComponent() {
   

   This whole method could use some inline comments

7. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -470,4 +498,71 @@ private function sectionStorageManager() {
   +            $plugin = $component->getPlugin();
   +            if ($plugin instanceof FieldBlock) {
   ...
   +          $plugin = $component->getPlugin();
   +          if ($plugin instanceof FieldBlock) {
   

   I think this should check the base plugin ID instead

8. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -470,4 +498,71 @@ private function sectionStorageManager() {
   +  private function getSectionComponentForFieldName($name) {
   

   Missing docblock, also the rest should have inline comments

9. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
   @@ -0,0 +1,196 @@
   +   * Alters the entity for Quick Edit compatibility.
   

   Alters the entity view build? or something? It doesn't alter the entity

10. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
    @@ -0,0 +1,196 @@
    +      // @todo Change to use EntityContextDefinition in
    +      // https://www.drupal.org/project/drupal/issues/2932462.
    +      $contexts['layout_builder.entity'] = new Context(
    +        new ContextDefinition(
    

    That was committed, this can switch

@tim.plunkett thanks for the review!

1. I move this and 2) into \Drupal\layout_builder\QuickEditIntegration::entityViewAlter()
2. see 1). also I realized I wasn't checking for the quick edit permission here. Not is 1 block surround by the permission check.
3. fixed
4. added a @see to \Drupal\quickedit\MetadataGenerator::generateFieldMetadata()
5. fixed
6. Added a bunch of comments
7. Added a check for $plugin->getBaseId() === 'field_block'.
8. Added docblock and inline comments. Also changed to check $plugin->getBaseId() === 'field_block' instead for plugin class.
9. Changed to "entity view build".
10. changed

+++ b/core/modules/layout_builder/src/QuickEditIntegration.php
@@ -0,0 +1,196 @@
+    if (!$entity instanceof FieldableEntityInterface || !isset($build['_layout_builder'])) {
+      return;
+    }

Realized I should checking the QuickEdit permission here because altering is not needed if the current user doesn't have the permission. Removes the need for the check later I mentioned above.

Trying to only respond to things that have not yet been made irrelevant.

#50:

1. See point 2.
2. The class resolver is about resolving a class, not about retrieving a service from the container. You're right that \Drupal\Core\DependencyInjection\ClassResolver::getInstanceFromDefinition() explicitly supports ContainerInjectionInterface. But a consequence of using the class resolver is that each invocation will construct a new instance. So every layout_builder_entity_view_alter() invocation during a request (e.g. once for every node in a view listing nodes) will cause a new object to be constructed, and fetch the same dependencies from the container again.

13. 👍🤔 Ahhh! But why are these 3 special? I can understand it for title on the canonical page of an entity. I know that for Node, uid and created are also special. But not for other entity types, right? This is testing Node, so this makes perfect sense.
14. 🥳

#53.1: Right, \Drupal\quickedit\QuickEditController::renderField() always splits on -. quickedit.api.php documents this, but not very clearly 😞 Sorry.

#55:

2. You're right — I forgot that Quick Edit only allows editing the default revision. So indeed something Layout Builder also doesn't need to worry about.
3. Improving or even fixing Quick Edit tests is definitely out-of-scope here.

4. I am not exactly sure yet how to fix this. I know quickedit uses metadata but I am sure if this is used vary the client cache like it is Context Link client storage.
   In the Contextual module you add information to the contextual metadata it will keep a separate version of the links.

   Is something like this possible with Quickedit?

   It's cached by entity translation and used view mode (or for custom render pipelines: "Quick Edit view mode"). You have the power to pretend every layout change is a different "Quick Edit view mode". The data-quickedit-field-id is what is being looked up in sessionStorage. This may or may not be enough.

#56: this is going in the direction that I wrote in response to #55.4 too 👍 But just the data-quickedit-entity-id won't be enough, since that doesn't include view mode (because the intent is to ensure multiple occurrences of the same entity on the same page are all updated automatically if you edit one of them, this is the way we match them up IIRC).

#59 + #60: I like where this is going! But are you sure that data-quickedit-field-id alone isn't enough? Because this first checks if all metadata for the data-quickedit-field-ids for a given entity in the DOM of the current page exists:

    // Checks if the metadata for all given field IDs exists.
    function allMetadataExists(fieldIDs) {
      return fieldIDs.length === metadata.intersection(fieldIDs).length;
    }

and if not, if fetches that metadata using fetchMissingMetadata. I think that should work, and it'd remove the need for the custom JS, as well as the drupalSettings stuff, making Layout Builder far less coupled to Quick Edit's implementation details.

#61 + #80: there be dragons. That's where it really hurt us that Quick Edit was written in a time where JS testing in Drupal was impossible, JS tests were only added years later :(

#89:

1. (Intentionally not yet reviewing the JS per my #59/#60 response.)

2. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -291,9 +297,12 @@ protected function buildSections(FieldableEntityInterface $entity) {
   -      foreach ($storage->getSections() as $delta => $section) {
   -        $build[$delta] = $section->toRenderArray($contexts);
   +      if ($sections = $storage->getSections()) {
   +        foreach ($sections as $delta => $section) {
   +          $build[$delta] = $section->toRenderArray($contexts);
   +        }
          }
   

   🤔 Is this fixing a bug in HEAD? Do we need separate test coverage for it?

3. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -470,4 +479,94 @@ private function sectionStorageManager() {
   +  public function getComponent($name) {
   +    if ($this->isLayoutBuilderEnabled()) {
   +      if ($section_component = $this->getSectionComponentForFieldName($name)) {
   ...
   +  private function getSectionComponentForFieldName($field_name) {
   +    // First check if there is section component that was set up to work with
   +    // QuickEdit. This does not require $field_name because it requires this
   +    // display to be setup to handle a single field for QuickEdit.
   +    $section_component = $this->getQuickEditSectionComponent();
   

   🤔 The caller of getSectionComponentForFieldName() doesn't seem to be specific to Quick Edit at all, yet that method does things very specific to Quick Edit. I can't figure out why that is the case.

4. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -470,4 +479,94 @@ private function sectionStorageManager() {
   +   * Returns the QuickEdit formatter settings.
   ...
   +  private function getQuickEditSectionComponent() {
   

   🤔 "formatter settings" versus "component"?

   Also, there are no "Quick Edit section components" AFAICT? Does that mean this is actually getSectionComponentForUseInQuickEdit() or something like that?

5. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -470,4 +479,94 @@ private function sectionStorageManager() {
   +    // To determine the view_mode used by QuickEdit we need an originalMode set.
   +    if (isset($this->originalMode)) {
   

   🤔 Shouldn't this then do:

   if (!isset($this->originalMode)) {
     throw new \LogicException(…);
   }
   

   ? Isn't this currently allowing silent failures?

6. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -470,4 +479,94 @@ private function sectionStorageManager() {
   +        // Layout Builder component. The created view_mode starts
   

   Nit: s/starts/starts with/

@Wim Leers thanks for the review.
re #90
#50

2. Changed to service

#53.1 oh just found this. Nice

#55

3. Ok to avoid making any changes to quickedit test going to copy what we need over to layout_builder tests.

   Actually want to upload this patch with the current tests so I can make sure it is not just passing because somehow I messed up the test.

   We do this after the this patch.

4. . The data-quickedit-field-id is what is being looked up in sessionStorage. This may or may not be enough.

   This was not working for me before and that is why I added the Javascript to clear the metadata. I think I found the problem though.
   I think wasn't clearing the metadata even when it had new data-quickedit-field-id because of #2966136: Quick Edit assumes that all fields meta data are stored in cache or none
   Basically if the fields not controller by layout builder, title, uid and created had the same data-quickedit-field-id as when layout builder was not used then the new metadata was not fetched.
   So when a node was viewed and the layout was enabled for the node when the node was viewed again quickedit did not work.

   So the solutions for this for this is for Layout Builder alter the data-quickedit-field-id for all fields on the entity even if they are not controlled by Layout Builder.
   This allows removing the javascript from this patch!

   This last point from #90 I am going address in this patch will address the rest after this patch passes.

Some other things I changed while figuring this out.

1. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
   @@ -0,0 +1,219 @@
   +    if ($non_empty_sections) {
   +      $sections_hash = hash('sha256', serialize($non_empty_sections));
   +    }
   +    else {
   +      // Set the section hash to indicate we are not using the Layout Builder.
   +      // If the Layout Builder was previously enabled for this entity the
   +      // QuickEdit metadata will need to be cleared on the client.
   +      $sections_hash = 'no_sections';
   +
   +    }
   +    $build['#attached']['drupalSettings']['layout_builder']['section_hashes'][$entity->getEntityTypeId() . ':' . $entity->id() . ':' . $display->getMode()] = [
   +      'hash' => $sections_hash,
   +      'quickedit_storage_prefix' => $entity->getEntityTypeId() . '/' . $entity->id(),
   +    ];
   

   Instead of using drupalSettings with hash of the sections the hash is now being concatenated onto the end of the quickedit view mode.

   This means the data-quickedit-field-id will change everytime the sections change,which could mean field change in the layout.

   Also when I moved the logic into this function I switched to hash that actual built sections rather than the section storage sections. This will not work because it will change when the actual field values change.

2. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
   @@ -0,0 +1,219 @@
   +      $view_display = EntityViewDisplay::collectRenderDisplay($entity, $view_mode_id);
   +      $cacheable_metadata = new CacheableMetadata();
   +      $section_list = $this->sectionStorageManager->findByContext(
   +        [
   +          'display' => EntityContext::fromEntity($view_display),
   +          'entity' => EntityContext::fromEntity($entity),
   +          'view_mode' => new Context(new ContextDefinition('string'), $view_mode_id),
   +        ],
   +        $cacheable_metadata
   +      );
   +
   +      $component = $section_list->getSection($delta)
   +        ->getComponent($component_uuid);
   +      $contexts = $this->contextRepository->getAvailableContexts();
   +      $contexts['layout_builder.entity'] = EntityContext::fromEntity($entity);
   

   It makes more sense here to actually just render the entity and then extract the rendered component.

   This will also work for field not rendered by the layout builder like title which we have to handle because we altering the data-quickedit-field-id for these fields too.

+++ b/core/modules/layout_builder/layout_builder.module
@@ -150,7 +150,7 @@ function layout_builder_entity_view_alter(array &$build, EntityInterface $entity
-  $quick_edit_integration = \Drupal::classResolver(QuickEditIntegration::class);
+  $quick_edit_integration = \Drupal::service('layout_builder.quickedit_integration');

@@ -324,6 +324,6 @@ function layout_builder_system_breadcrumb_alter(Breadcrumb &$breadcrumb, RouteMa
-  $quick_edit_integration = \Drupal::classResolver(QuickEditIntegration::class);
+  $quick_edit_integration = \Drupal::service('layout_builder.quickedit_integration');

This goes against a standard pattern used in multiple core modules.
If this is a good change to be made, a follow-up should be opened to make the same change elsewhere.

#92: great step forward!

• RE: #55.3 I was not trying to say this patch is not allowed to make any changes to Quick Edit's tests. Given their less-than-ideal state I think this is acceptable. I was only trying to say that it's not this issue's responsibility to improve all the Quick Edit tests :)
• RE: #55.4: So the solutions for this for this is for Layout Builder alter the data-quickedit-field-id for all fields on the entity even if they are not controlled by Layout Builder. This allows removing the javascript from this patch! Hurray!

  I just dug into #2966136: Quick Edit assumes that all fields meta data are stored in cache or none and was unable to reproduce it: #2966136-6: Quick Edit assumes that all fields meta data are stored in cache or none

• #95: Okay, sorry, I did not realize that. Let's revert this change then. Sorry for requesting it.

Patch review:

1. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
   @@ -0,0 +1,213 @@
   +    // @todo IN THIS ISSUE determine if this a bug in QuickEdit or just how
   +    //   client metadata needs to be cleared.
   +    //   @see https://www.drupal.org/project/drupal/issues/2966136
   

   Reviewed that issue too.

2. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
   @@ -0,0 +1,213 @@
   +    foreach (Element::children($build) as $field_name) {
   +      if ($field_name !== '_layout_builder') {
   +        $field_build = &$build[$field_name];
   +        if (isset($field_build['#view_mode'])) {
   +          $field_build['#view_mode'] = "layout_builder-{$display->getMode()}-non_component-$sections_hash";
   +        }
   +      }
   +    }
   

   👍 LGTM! The hash is updated whenever the layout changes.

I didn't make any substantive changes to the code here. For the tests, it was mostly reverting things that seemed like debugging changes.

1. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -147,6 +148,10 @@ function layout_builder_entity_view_alter(array &$build, EntityInterface $entity
   +  /** @var \Drupal\layout_builder\QuickEditIntegration $quick_edit_integration */
   +  $quick_edit_integration = \Drupal::classResolver(QuickEditIntegration::class);
   +  $quick_edit_integration->entityViewAlter($build, $entity, $display);
   

   Should we wrap this in a moduleExists('quickedit') check?

2. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -45,6 +49,7 @@ public function __construct(array $values, $entity_type) {
   +    $this->sectionStorageManager = $this->sectionStorageManager();
   

   Nit: This seems unnecessary. It's not a big deal at all; just a little jarring at first glance, unless you know LayoutEntityHelperTrait.

3. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -470,4 +475,95 @@ private function sectionStorageManager() {
   +      $plugin = $section_component->getPlugin();
   +      if ($plugin instanceof ConfigurableInterface) {
   +        $configuration = $plugin->getConfiguration();
   +        if (isset($configuration['formatter'])) {
   +          return $configuration['formatter'];
   +        }
   +      }
   

   Why does this code check for ConfigurableInterface? If I'm understanding this correctly, the only block plugin which will work for this method is FieldBlock. Why not simply codify that, and check for an instance of FieldBlock? (In which case we can probably lose the isset($configuration['formatter']) check.)

4. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -470,4 +475,95 @@ private function sectionStorageManager() {
   +    if (isset($this->originalMode)) {
   

   Classes should call their own methods; can this be if ($original_mode = $this->getOriginalMode()) instead?

5. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -470,4 +475,95 @@ private function sectionStorageManager() {
   +      // 'layout_builder' to distinguish if from other view_modes. If the third
   

   Typo: Should be "...to distinguish it..."

6. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -470,4 +475,95 @@ private function sectionStorageManager() {
   +      // part of the view_mode is component then this is field that is rendered
   

   "component" should be quoted so that we know that it's a magic string.

7. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -470,4 +475,95 @@ private function sectionStorageManager() {
   +      if (count($parts) === 4 && $parts[0] === 'layout_builder' && $parts[2] === 'component') {
   

   It might be useful to add a @see here referencing the relevant code in QuickEditIntegration, and also provide an example of what the original mode string might look like.

8. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -470,4 +475,95 @@ private function sectionStorageManager() {
   +        $entity = $this->entityTypeManager()->getStorage($this->targetEntityType)->load($entity_id);
   

   Can we use $this->getTargetEntityTypeId()?

9. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -470,4 +475,95 @@ private function sectionStorageManager() {
   +  private function getSectionComponentForFieldName($field_name) {
   

   So, this will return the first instance of the relevant field block. Discussed with Tim, and he convinced me that this is actually okay behavior. However, I'm wondering if we should maybe add test coverage, assuming we don't already have any, of what happens if the same field is in the layout more than once. Even if it works in manual testing, we might want to have a test of it just to be sure it doesn't regress.

10. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
    @@ -470,4 +475,95 @@ private function sectionStorageManager() {
    +    $section_component = $this->getQuickEditSectionComponent();
    

    It a bit seems strange to me for this method to call getQuickEditSectionComponent(). Ideally, getComponent() would do something like $section_component = $this->getQuickEditSectionComponent() ?: $this->getSectionComponentForFieldName($name). I think that might make the overall logic a little less magical, and it would also remove a level of nesting from this method.

11. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
    @@ -470,4 +475,95 @@ private function sectionStorageManager() {
    +            list(, , , $field_block_field_name) = explode(':', $plugin->getPluginId());
    

    Maybe explode() should be called with PluginBase::DERIVATIVE_SEPARATOR?

12. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
    @@ -0,0 +1,229 @@
    +   * that modules identify themselves with a view mode in the format module
    +   * name-id.
    

    "format module name-id" is how this reads. Can it be "...in the format 'module_name-id'"?

13. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
    @@ -0,0 +1,229 @@
    +    $sections_hash = hash('sha256', serialize($section_list->getSections()));
    

    It's not clear why this hash is generated. Could probably use a comment...

14. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
    @@ -0,0 +1,229 @@
    +    // @todo IN THIS ISSUE determine if this a bug in QuickEdit or just how
    +    //   client metadata needs to be cleared.
    +    //   @see https://www.drupal.org/project/drupal/issues/2966136
    

    "IN THIS ISSUE"? Is this dealt with?

15. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
    @@ -0,0 +1,229 @@
    +    if ($entity instanceof FieldableEntityInterface) {
    

    I think it would be preferable to return early if $entity is not an instance of FieldableEntityInterface.

16. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
    @@ -0,0 +1,229 @@
    +          $entity_build = call_user_func($callable, $entity_build);
    

    Pro-tip: I think that callables can be invoked like $callable($entity_build). No need for call_user_func().

17. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
    @@ -0,0 +1,229 @@
    +      else {
    +        if (isset($entity_build[$field_name])) {
    +          return $entity_build[$field_name];
    +        }
    +      }
    

    I think this could be elseif to remove a level of nesting.

18. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
    @@ -0,0 +1,229 @@
    +    if (isset($component['content']['#field_name']) && isset($component['#base_plugin_id']) && $component['#base_plugin_id'] === 'field_block') {
    

    Pro-tip: isset() is variadic, so you can call isset($component['content']['#field_name'], $component['#base_plugin_id']) to save a bit of typing.

19. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
    @@ -0,0 +1,229 @@
    +      if ($entity->getFieldDefinition($component['content']['#field_name'])->isDisplayConfigurable('view')) {
    +        return TRUE;
    +      }
    

    This can just be return $entity->getFieldDefinition(...)->isDisplayConfigurable('view'), to remove a level of nesting.

20. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
    @@ -0,0 +1,351 @@
    +  public static $modules = [
    

    Should be protected.

21. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
    @@ -0,0 +1,351 @@
    +  /**
    +   * Whether the test is currently using Layout Builder on the entity.
    +   *
    +   * @var bool
    +   */
    +  protected $usingLayoutBuilder = FALSE;
    

    Well, this is pretty interesting. Can the comment expand on why we need this flag? Maybe a couple of @see references?

22. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
    @@ -0,0 +1,351 @@
    +  protected function replaceLayoutBuilderFieldIdKeys(array $array) {
    +
    +    $layout_builder_expected_states = [];
    

    Empty line. Also, can $array get a better name?

23. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
    --- a/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditIntegrationTest.php
    +++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditIntegrationTest.php
    

    Some of the changes to this class don't seem, on their face at least, to be in scope. Why are we modifying Quick Edit's tests?

#98
1)
Sure, although this was already protected by a permission check inside entityViewAlter. But can't hurt here.
The other QuickEditIntegration usage is in a quickedit-provided hook, so no need there.

2)
Removing this, because it's _not_ necessary, and it itself uses service location anyway (because it's an Entity class)

3)
It checks for ConfigurableInterface because SectionComponent::getPlugin() doesn't guarantee anything more.
I'd rather leave this than unnecessarily couple it to FieldBlock (which could always be swapped out)

4)
Sure :)

5)
Fixed

6)
Fixed

7)
There is a whole comment directly above this line that explains it and points to the method directly, and there are corresponding @see's on this method itself

8)
Fixed

9)
@todo for testing

10)
Sure, that's fine. Doesn't really matter either way

11)
Yes! Thanks for catching this.

12)
Fixed

13)
Wrote a comment

14)
After investigating more, I think this can be removed.

15)
Fixed

16)
I rewrote this whole part and opened up #3041635: Provide a reliable way to execute #pre_render while still retaining an array

17)
Fixed

18)
True, not a huge win, but changed anyway

19)
Changed

20)
Cannot be changed here because QuickEditJavascriptTestBase and QuickEditIntegrationTest (parent classes) wrongly use public

21)
Expanded

22)
Changed

23)
There are no actual changes in this class, but certain portions of it are divided into methods that can be overridden in the subclass.
For example, testArticleNode() is split into createNodeWithTerm() and doTestArticle()
Wim clarified in #96 that upstream changes were okay

Assigning to myself to work on #98.9

It checks for ConfigurableInterface because SectionComponent::getPlugin() doesn't guarantee anything more.
I'd rather leave this than unnecessarily couple it to FieldBlock (which could always be swapped out)

By checking for $configuration['formatter'], it is already coupled to FieldBlock, though not quite as tightly. As it currently is, the code assumes that any configurable plugin with a "formatter" element in its configuration (even if it's not an array!) is the plugin we want -- and it might or might not be a FieldBlock, which could potentially lead to side effects or errors. So IMHO, since this is already coupled, we can simplify the code around it. One possible option would be to couple it to the plugin ID (using DerivativeInspectionInterface), rather than the concrete implementation. However, if you feel very strongly about this point, I won't push back.

Otherwise, interdiff looks full of nice changes. Thanks, Tim!

re #100

I agree checking 'formatter' here we are assuming no other configurable plugin will use this key for other non field formatter configuration which doesn't seem safe.

checking the check to be $plugin instanceof DerivativeInspectionInterface && $plugin->getBaseId() === 'field_block'

because that is what are using in getQuickEditSectionComponent()

After discussion with @dead_arm and then with @tedbow and @phenaproxima, we agreed that the best resolution for #98.9 will be to prevent quickedit when a field is placed multiple times.
Fixing it in any other way would mean a significant refactor of quickedit itself.
I've opened #3041850: Allow Quick Edit to work when a field is placed more than once within the same entity as a follow-up and linked it where relevant.

Removing the CR tag as it was added in #5 when there were API changes being made. It is no longer relevant.

The additional change for multiple instances of the same field looks good to me!

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
@@ -347,9 +383,26 @@ protected function getQuickEditFieldId($original_field_id) {
+    try {
+      $has_attribute = $element->hasAttribute('data-quickedit-field-id');
+    }
+    catch (\Exception $e) {
+      $has_attribute = FALSE;
+    }

Under what circumstances would hasAttribute() throw an exception?

re #106

Under what circumstances would hasAttribute() throw an exception?

🤔 when I leave debug code in and the debug code is actually very bad because it doesn't actually do what I thought it did?

Removing and adding a couple comments in that method.

#108 was a random fail.

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
@@ -380,19 +380,14 @@ private function assertQuickEditInit(NodeInterface $node) {
+    // Remove the last part of the field id which will contain the QuickEdit
...
+    // layout sections and will may be different each time the layout changes.

Talked to @phenaproxima and pointed out this "will may be" typo.

Also split the comment into 2 sentences because the current sentence has 2 "which..." clauses.

@tim.plunkett and @tedbow were kind enough to get on a Zoom with me and walk me through this patch, nearly line by line.

I think we are all in agreement that this patch is a Slytherin: it's doing all sorts of weird, scary, and questionable magic to achieve its aims. We are also in agreement that this is mostly due to the underlying architectural assumptions upon which Quick Edit is built. It was never designed to work with something like Layout Builder; it assumes that the standard Field API is responsible for all entity rendering, and this assumption is baked into it at every level. Layout Builder explodes (pardon the pun once you read the patch and see the number of calls to explode()) a bunch of those assumptions, and therefore it needs to get into areas where, in an ideal world, it wouldn't have to go. Follow-ups are open to improve all of this, but for now, this patch is very much steeped in a (understandable) "do what you gotta do" mentality.

The extension of QuickEditIntegrationTest is confusing, but it's also clever. The rationale is to be sure that everything Quick Edit tests, is also tested with Layout Builder enabled. So the idea makes sense, but it necessitated moving a bunch of code around in QuickEditIntegrationTest, which is why that test has such extensive changes in this patch.

Given the facts of how Quick Edit currently works and its limitations, and how important this patch is to Layout Builder's stabilization, I agree with the "do what you must" approach, and I'm RTBCing it.

This is from the interdiff from #107

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
@@ -380,19 +380,14 @@ private function assertQuickEditInit(NodeInterface $node) {
-    try {
-      $has_attribute = $element->hasAttribute('data-quickedit-field-id');
-    }
-    catch (\Exception $e) {
-      $has_attribute = FALSE;
-    }
-    $this->assertTrue($has_attribute, $field_key_without_view_mode);
...
+    return $element->getAttribute('data-quickedit-field-id');

This is where the fail is happening. I had that assert in there and idk why it helped but it did.

the best resolution for #98.9 will be to prevent quickedit when a field is placed multiple times.

I think this is a reasonable compromise for what really is an edge case. Thanks for filing #3041850: Allow Quick Edit to work when a field is placed more than once within the same entity!

1. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -470,4 +475,89 @@ private function sectionStorageManager() {
   +   * Returns the QuickEdit formatter settings.
   
   +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
   @@ -0,0 +1,303 @@
   + * Helper methods for QuickEdit module integration.
   

   ✅ s/QuickEdit/Quick Edit/. Fixed.

2. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -470,4 +475,89 @@ private function sectionStorageManager() {
   +      $parts = explode('-', $original_mode);
   ...
   +      if (count($parts) === 4 && $parts[0] === 'layout_builder' && $parts[2] === 'component') {
   +        list($delta, $component_uuid, $entity_id) = explode('-', $parts[3]);
   

   🤔 How is it possible that we can explode('-', …), get 4 parts, and then again explode('-', …) on one of the parts?

3. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -470,4 +475,89 @@ private function sectionStorageManager() {
   +      // The QuickEdit view_mode is created by
   +      // \Drupal\layout_builder\QuickEditIntegration::entityViewAlter()
   +      // by concatenating together the information we need to retrieve the
   +      // Layout Builder component. The created view_mode starts with
   +      // 'layout_builder' to distinguish it from other view_modes. If the third
   +      // part of the is 'component' then this is a field that is rendered by
   +      // Layout Builder.
   

   👎 ✅ This is restating much of what \Drupal\layout_builder\QuickEditIntegration::entityViewAlter() is already stating and what hook_quickedit_render_field()'s documentation is prescribing. Fixed.

4. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -470,4 +475,89 @@ private function sectionStorageManager() {
   +          // We only care about FieldBlock because these are only components
   +          // that provide QuickEdit integration.
   

   ✅ This statement is not entirely accurate. Fixed.

5. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
   @@ -0,0 +1,303 @@
   +   * When rendering fields outside of normal view modes, Quick Edit requires
   +   * that modules identify themselves with a view mode in the format
   +   * [module_name]-[id].
   ...
   +    // Create a hash of the sections and use it in the unique Quick Edit ID.
   ...
   +   *   The unique view mode ID.
   ...
   +   * Deconstructs the view mode ID into its constituent parts.
   ...
   +   *   The view mode ID.
   

   ✅ That's a lot of different descriptions for the same thing. Understandable, because it's one of the weirdest things in Quick Edit.

   Let's call this Quick Edit view mode ID consistently. That'll make it easier to understand, now, and in the future.

   (Again: yes, this API is weird, I said that 364 days ago in #5 and more recently in #49, grep for "weird".)

   Done.

6. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
   @@ -0,0 +1,303 @@
   +   * Generate a unique view mode ID.
   
   +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
   @@ -0,0 +1,404 @@
   +   * Disable Layout Builder.
   

   ✅ Übernit: third person singular. Fixed.

7. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
   @@ -0,0 +1,303 @@
   +    $this->getLogger('layout_builder')->warning('The field "%field" failed to render.', ['%field' => $field_name]);
   

   👏 Very clever! This will most certainly help in future support requests :)

8. +++ b/core/modules/layout_builder/src/QuickEditIntegration.php
   @@ -0,0 +1,303 @@
   +   * Only field_block components for view configurable fields should be
   

   🤔 What does "view configurable fields" mean?

9. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
   @@ -0,0 +1,404 @@
   +  protected function getLayoutBuilderViewMode($entity_type, $entity_id, $field_name, $view_mode) {
   +    // If the field is one that is not rendered by Layout Builder do not change
   +    // $view_mode.
   +    if (in_array($field_name, ['title', 'uid', 'created'])) {
   +      return $view_mode;
   +    }
   +    /** @var \Drupal\Core\Entity\ContentEntityInterface $entity */
   +    $entity = $this->container->get('entity_type.manager')->getStorage($entity_type)->load($entity_id);
   +    $view_display = EntityViewDisplay::collectRenderDisplay($entity, 'default');
   +    $sections = $view_display->getSections();
   +    // Find the component with the plugin ID in the field_block format that
   +    // matches the entity type, bundle, and field name.
   +    foreach (reset($sections)->getComponents() as $component) {
   +      $component_in_view_mode = str_replace('-', '_', $component->getUuid());
   +      if ($component->getPlugin()->getPluginId() === "field_block:$entity_type:{$entity->bundle()}:$field_name") {
   +        return 'layout_builder-0-' . $component_in_view_mode . '-' . $entity->id();
   +      }
   +    }
   +    $this->fail("Component not found for: $entity_type, $entity_id, $field_name");
   +  }
   

   AFAICT this is now dead code. Deleted. 🥳

Only points 2 and 8 need a response, the rest I addressed myself.

Manually testing #115

Installed Layout Builder, Basic page, Override. Created a basic page.

When trying to Quick Edit the Page body, it goes up to the title, see attached image:

Trying to add a new block does not work:

TypeError: Argument 1 passed to Drupal\layout_builder\QuickEditIntegration::entityViewAlter() must be of the type array, null given, called in /Users/john/code/drupal/core/modules/layout_builder/layout_builder.module on line 246 in /Users/john/code/drupal/core/modules/layout_builder/src/QuickEditIntegration.php on line 98 #0 /Users/john/code/drupal/core/modules/layout_builder/layout_builder.module(246): Drupal\layout_builder\QuickEditIntegration->entityViewAlter(NULL, NULL, NULL)
#1 /Users/john/code/drupal/core/lib/Drupal/Core/Extension/ModuleHandler.php(539): layout_builder_plugin_filter_block_alter(Array, Array, 'layout_builder')
#2 /Users/john/code/drupal/core/lib/Drupal/Core/Plugin/FilteredPluginManagerTrait.php(31): Drupal\Core\Extension\ModuleHandler->alter('plugin_filter_b...', Array, Array, 'layout_builder')

So only #114.2 is a relatively important question, #114.8 is just a nit. Even #114.2 is not very important as long as the test show it works correctly.

Wearing my Quick Edit maintainer hat, this is RTBC:

1. It uses the intended API: provides a custom Quick Edit view mode ID + hook_quickedit_render_field(), to use Layout Builder's render pipeline
2. It comes with extensive tests.

But unfortunately the latest patch is failing, and if @tim.plunkett is right in #113, then reverting #107's simplification fixes it, but we don't understand yet why that's the case. The concrete failure is:

 Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderQuickEditTest::testArticleNode with data set "use override" (true)
WebDriver\Exception\CurlExec: Curl error thrown for http POST to http://chromedriver-jenkins-drupal-patches-89793:9515/session/e1949bb1cbb0132c5faea1e56349f040/execute with params: {"script":"return arguments[0].getAttribute(\"data-quickedit-field-id\")","args":[{"ELEMENT":"0.07455372260686577-6"}]}

This is triggered by \Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderQuickEditTest::getQuickEditFieldId() reading the value of an attribute of the current page. This exception is thrown by \Drupal\FunctionalJavascriptTests\WebDriverCurlService::execute():

throw WebDriverException::factory(WebDriverException::CURL_EXEC, sprintf("Curl error thrown for http %s to %s%s\n\n%s", $requestMethod, $url, $parameters && is_array($parameters) ? ' with params: ' . json_encode($parameters) : '', $error));

The code prior to #107 was catching the generic \Exception and was therefore gracefully handling this error. Note that this is a curl error, not a HTTP error response. If that were the case, then \WebDriver\AbstractWebDriver::curl() would've been able to handle it! No, this is a truly a curl error. It seems we're
hitting a bug in the PHP curl webdriver code … and I don't think that should hold this up.

If my analysis is correct, then reverting #107's interdiff but catching a more specific exception would be a fair way to move forward here, to avoid getting blocked on an upstream bug. I don't get why this does work and the code above does not, but us hitting an obscure edge case bug upstream would explain it.

This is just an attempt, if somebody sees a better path forward, I'm all for it!