Reduce the number of field blocks created for entities (possibly to zero)

Would this behaviour mean I'd need to enable Layout Builder on the User entity to be able to add author (user) fields on a node, etc?

I think the performance impact here warrants this being major

Re #9/#10
Right now the entity references are not added as a context, so AFAIK the user fields added to a node display are not showing the author, but the user currently viewing the page.

I don't see how we can safely make this change.
Trying to introspect everywhere that block plugins are used is an exercise in futility.

And this would straight up remove the plugin definition, cause a "broken or missing block plugin" message with no clear recourse.

Unless we put in a secret config flag to opt into this? I'm really not sure.

Discussed with @tim.plunkett a bit and I think I see a potential way forward here.

I like the idea of making this behavior configurable...at least for now. There are two behaviors:

1. Expose as many fields as possible as blocks, regardless of whether or not they are used in Layout Builder. This is the current behavior.
2. Expose only the fields which belong to LB-ized entity bundles. This is the proposed behavior.

So what if we made a config switch which specifies which one of these paths should be taken, and deprecate the old (current) behavior?

This could be implemented as two deriver classes. Let's call them (we'll rename them later) FieldBlockDumbDeriver (which has the current behavior) and FieldBlockSmarterDeriver (which would have the new behavior). FieldBlockSmarterDeriver would be the "main" deriver (i.e., the one specified in FieldBlock's annotation), and it would check the config switch. If the config switch said to use the old, "dumb", deriver, it would simply throw a deprecation warning, then delegate to that one. Then, we can remove the dumb deriver in Drupal 9, and live happily ever after.

I think that's worth pursuing, tentative +1

This could be implemented as two deriver classes. Let's call them (we'll rename them later) FieldBlockDumbDeriver (which has the current behavior) and FieldBlockSmarterDeriver (which would have the new behavior). FieldBlockSmarterDeriver would be the "main" deriver (i.e., the one specified in FieldBlock's annotation), and it would check the config switch. If the config switch said to use the old, "dumb", deriver, it would simply throw a deprecation warning, then delegate to that one.

Hell, why over-engineer it? After tinkering around with this, I think we could easily do this in FieldBlockDeriver, with a config switch, and just throwing deprecation errors as necessary.

New patch which makes this configurable.

Also, I'm not even sure we need to deprecate this. It's possible that there are some sites which will want to expose every field as a block, performance be damned. That obviously should not be the default behavior, but if we need to add the config switch anyway, why not let people use it if they have crazy needs?

1. +++ b/core/lib/Drupal/Core/Block/BlockManager.php
   @@ -48,7 +48,7 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
   -    $this->setCacheBackend($cache_backend, 'block_plugins');
   +    $this->setCacheBackend($cache_backend, 'block_plugins', ['config:entity_view_display_list']);
   

   This reminds me of #3001284: Allow plugin derivers to specify cache tags for their definitions and is out of scope for this issue.

   Looking at \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::preSave(), could add a \Drupal::service('plugin.manager.block')->clearCachedDefinitions(); if if ($already_enabled !== $set_enabled) {

2. +++ b/core/modules/layout_builder/src/Plugin/Derivative/FieldBlockDeriver.php
   @@ -86,6 +110,16 @@ public static function create(ContainerInterface $container, $base_plugin_id) {
   +    $displays = $this->entityViewDisplayStorage->loadMultiple();
   ...
   +      $displays_by_bundle[$display->getTargetEntityTypeId()][$display->getTargetBundle()][] = $display;
   
   @@ -96,6 +130,20 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   +              if ($display->isLayoutBuilderEnabled()) {
   

   I think this foreach loop at the top could check and only add if it is enabled.

   Or, you could do this similar to LayoutBuilderOverridesPermissions like this:

   $displays = $this->entityTypeManager->getStorage('entity_view_display')->loadByProperties(['third_party_settings.layout_builder.enabled' => TRUE]);
   

Fixed the feedback in #21.

Moved getFieldMap() below getDerivativeDefinitions(), which will hopefully make the diff a lot easier to read. No real changes since the last patch, so not adding an interdiff.

1. +++ b/core/modules/layout_builder/config/install/layout_builder.settings.yml
   @@ -0,0 +1 @@
   +expose_all_field_blocks: false
   

   So if we set this to FALSE by default, we'll need a hook_update_N to set it to TRUE for existing sites, right?

2. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -158,6 +158,10 @@ public function preSave(EntityStorageInterface $storage) {
   +      // Invalidate the block cache in order to regenerate field block
   +      // definitions.
   +      \Drupal::service('plugin.manager.block')->clearCachedDefinitions();
   

   Theoretically this is only needed when the new config is FALSE, right?

This will need an update path and tests.

Oh, and it'll also need normal tests.

Let's say you enable LB for nodes and users. And you add a user field to the node display. (regardless if it's the author or currently viewing user).

Then you turn off LB for users

That user field block on your node will break

I kinda am thinking this is won't fix, and point to the Lullabot module and article in #5

Here's another idea that puts the "mad" in mad science:

1. In simple config, we maintain a list of which entity types and bundles should have field blocks exposed. This is used by the deriver. On existing sites, we default this to every entity type and bundle which has Layout Builder applied.
2. When you opt a display into Layout Builder, the target entity type and bundle is added to the list.
3. If you subsequently remove that display from Layout Builder, the target entity type and bundle are NOT removed from the list. Thus, any blocks from that entity type and bundle continue to work.
4. The only way to remove previously opted-in entity types/bundles from field blocks is to edit the config and clear the block cache. Site builders or developers can therefore tune the performance if they need to, but otherwise everything continues to work.

In other words...a somewhat stickier opt-in.

Expanding on that idea: we could add another configuration checkbox to entity view displays (under "Use the Layout Builder"), where we specifically allow people to expose the bundle's blocks as fields. It would be checked by default when you enable Layout Builder for that bundle. This way, the sticky opt-in is accessible to site builders, rather than hidden in config somewhere, and we can document its effects.

I wonder if we could utilize the third-party settings on the entity view display here.

We have a flag for 'allow layout builder' already, we could expand that with 'included fields' defaulting to all selected.

So that would provide BC - no behaviour change for those already using it. But for large-scale/real-world sites, such as the one Sam is profiling, there is capacity to turn off fields.

One super stupid approach would be to take the idea from #23 without actually fixing anything.

But this split would allow custom code to swap out the FieldBlockDeriver and override just the getFieldMap() method, without having to recreate the complexity of the main portion of the deriver.

My esteemed colleague @phenaproxima reminds me that by the time plugin alters run, derivers have already run, and there's no way to swap out that annotation. So, no go on that one.

I wonder if we could utilize the third-party settings on the entity view display here.

We have a flag for 'allow layout builder' already, we could expand that with 'included fields' defaulting to all selected.

I don't see how this could work; field block definitions are derived globally, not locally per entity view display.

This seems like mostly a performance issue if Webform is enabled and you have tons of forms, bundles.
Re #5

Also linking https://www.lullabot.com/articles/announcing-new-lullabotcom which cites https://www.drupal.org/project/block_blacklist as a reference for better performance as well.

This was before #2994550: Filtering block plugins by context is slow was committed. I am pretty sure in their case the performance would not be problem after that.

I have a site where 1600 block plugins are being created for webform submission bundles, which will never require field blocks.

Webform is pretty unique in how it uses bundles. In almost all other cases creating bundles is a site builder task and doesn't grow of over 1000 bundles(or even 100?)

Is this really a problem with any other module?

There is already hook_block_alter which would allow the webform module remove the field blocks easily.

I don't think we should be removing the field blocks for bundles not enabled for Layout Builder. Layout Builder would become much more useful if a contrib module(or core) added the context for any entity reference field to a fieldable entity on the entity using Layout Builder.

This would allow you to display individual field from the author via the layout builder or use any of the entity references as contextual filters for Views.

I don't think we should be making this any harder than it already is.

Another way to solve this without Webform having to do this and not having to have any new config or options is not create field blocks for bundles/entity types when Layout Builder cannot be enabled for the bundle.

Webform submission bundles don't have a manage display tab so we can't enable Layout Builder at all for them anyways either for defaults or overrides(side point we create overrides storage routes even if there are not default routes and overrides can't be turned on)

But if that is too difficult or relies us coupling to much to field UI then I think asking Webform to implement hook_block_alter would not be too much to ask.

#38 is worth exploring, IMO. But it will be tricky to do in a way that doesn't couple it to core implementations too closely

It's worth noting that I'm currently seeing over 30,000 block plugins created for Webforms on our large government site. It's completely crippling our performance. This is a big deal.

Just to follow up here, I'm now using the block_list_override module to avoid loading block plugins for field_block:webform_submission and extra_field_block:webform_submission and it's paid ENORMOUS dividends.

In my local benchmarking here’s the effect it has on an authenticated user loading a standard node without a warm cache:

Old: 1m6s and 312MB memory
New: 5.37s and 50.1 MB memory

This is not a subtle change. This problem should, at the very least, be much more highly publicized. If anyone is planning on using Layout Builder with Webform (a not-uncommon combo) or even on sites with large numbers of fieldable entity types and bundles, the effect will be considerable.

While we consider how to solve this in core, I strongly recommend people check out block_list_override.

I don't know why this would be creating plugins for Webforms. Maybe worth an additional issue in the Webform queue?

Also in the meantime, you can use https://www.drupal.org/project/block_list_override
Unlike LB Restrictions (which is UI only), this one will benefit performance.

@Tim.plunkett:

As you can see from the original issue description, the issue with webforms is what first caused this to be noticed. The problem is that every webform creates a new bundle for the webform_submission entity type, and FieldBlockDeriver creates blocks for every field on every bundle of every entity type..including these. If you have a lot of webforms then you have a lot of webform_submission bundles.

As I posted in my follow-up I am, in fact, using block_list_override to great effect!

Just a follow-up here. block_list_override, while being a great help, isn't a total panacea. It's not able to prevent LB from doing a lot of work in \Drupal\layout_builder\Plugin\Derivative\FieldBlockDeriver::getDerivativeDefinitions for every single field.

I'm hitting max execution time limits on a client site during cache rebuilds with this - taking over 20 seconds to complete. No webform but need to investigate further what it's running into.

I've rebuilt this patch for clean application to 9.4.x.

Just confirming this patch dropped a function call to getDerivativeDefinitions from 53.3MB memory usage to 1.57MB memory usage with no other changes; and 440ms down to 13ms processing time.

The profiling pointed out that we likely need a similar solution for `ExtraFieldBlockDeriver` though.

I'll work on this.

A new version including the ExtraFieldBlockDeriver.

On to fixing the tests (and not putting in review because if tests fail, it won't be reviewed).

+++ b/core/modules/layout_builder/config/schema/layout_builder.schema.yml
@@ -82,3 +82,11 @@ layout_plugin.settings.layout_twocol_section:
+layout_builder.settings:

+++ b/core/modules/layout_builder/src/Plugin/Derivative/ExtraFieldBlockDeriver.php
@@ -88,14 +100,26 @@ public static function create(ContainerInterface $container, $base_plugin_id) {
+    $expose_all_fields = $this->configFactory->get('layout_builder.settings')->get('expose_all_field_blocks');

• This means we are adding a new config object for layout_plugin.settings do we really want to do that?
• Should we enable it per entity type?
• Having a config object for layout_plugin would mean it will need a config page as well.
• Also adding a new config object means we need to add a file in config/default for default value and a post-update hook to update the active config storage.

This means we are adding a new config object for layout_plugin.settings do we really want to do that?

Why does it matter? Also do you mean layout_builder?

Should we enable it per entity type?

IMO - yes, even if an entity type has layout_builder enabled, it would be nice to be able to still disable field blocks being generated.

I agree with both comments here. The config option I think is the right one... to disable by default and only enable when the person knows what they are doing. I would expect 99% of sites will never want everything.

Yes, if there is a config object, we should probably have a config page. That said, I know of other config options that are purposefully hidden from the UI so that only people who know the effects can use them (via code).

I also agree that you should probably select the fields you want to expose, in much the same way as you can select the standard blocks you want to expose. But that could be an extension of this.... rather than a requirement of it. This 'bug' currently has massive effects on larger sites and I think the improvement warrants it going out sooner than later.

Caveat, If we did implement field selection at the LB config form element on content type, we could delete the config setting... it would no longer be required.

@acbramley

Why does it matter?

@nterbogt

If we did implement field selection at the LB config form element on content type, we could delete the config setting... it would no longer be required.

Yeah, that's what I meant. If we make it per entity type then we don't need config setting.
@acbramley

Also do you mean layout_builder?

Yeah bad copy paste :D
@nterbogt

you should probably select the fields you want to expose, in much the same way as you can select the standard blocks you want to expose.

I think this can be done in contrib. We can add a checkbox on the field settings page and both Derivers can call a hook, fire an event or add an EFQ query tag to exclude blocks.
@nterbogt

This 'bug' currently has massive effects on larger sites and I think the improvement warrants it going out sooner than later.

As you said it is not needed for 99% and no UI that is why I think having a hook/event/EFQ query tag would be easier to implement and commit. JSON:API kind of doing the same thing with resources.

FWIW, no UI, and code-only changes are not good for SaaS platforms as we have more than 300 sites on GovCMS in Australia.

A new version to set up the settings with an update_N. Wasn't sure of the numbering rules for 9.x so took a guess; easy to change.

Update hook looks good. We are missing core/modules/layout_builder/config/install/layout_builder.settings.yml file.

Hi Sam,

We talked about this internally a bit and understand the concerns about BC. It was discussed and decided that 99% of users would never want all fields, and due to the performance impacts, we think we should disable by default.

Users with enough know how can easily turn it back on... but honestly, our preference would be to never include all fields (with no option) and leave it up to contrib to expose all fields as an extension, if we were starting from scratch.

Here is a new patch with install file added back in, my bad.

I don't think these changes effect the global context.

The deriver is exposing blocks that display content from the node context of the current page. It can't show derived data from an entity relation.
i.e. the block that is created only displays something when that something has that field and that block is put on the layout for that something.

And these types of things are exposed by default with the patch.

TBH, I'm not actually sure how you can use the blocks that were being exposed on content types that don't have LB displayed. You can't select them through the UI anywhere, and even if you managed to, the blocks aren't going to show any data unless used in the context of an entity that has the field.

Thanks, I'll investigate further.

We talked about this internally a bit and understand the concerns about BC. It was discussed and decided that 99% of users would never want all fields, and due to the performance impacts, we think we should disable by default.

The point is that you need to keep the current behaviour (i.e BC), and instead inform users via CRs that the new option is available and why they should use it. It's not really up to us to decide that we should suddenly turn off fields that were previously there for any project out there that may be using it.

I've updated the patch so that the update hook is renamed to layout_builder_manual_N(). I've also updated the code within that to make the default TRUE to be backwards compatible.

The reason that I renamed the update hook is that people using the patch can call it from another custom module and it won't interfere with any releases made by core while this is being reviewed. It will need to be changed before it is merged.

Fixing a small bug in the previous iteration. This makes some of the code more consistent between the field deriver and extra field deriver, so we should pull that out into a helper function of some sort.

Any thoughts on where the function from ExtraFieldBlockDeriver::bundleIdsWithLayoutBuilderDisplays() should live to be reused across both derivers and available for other devs to use externally to look up what entity types and bundles have LB enabled?

+++ b/core/modules/layout_builder/layout_builder.install
@@ -82,3 +82,13 @@ function layout_builder_schema() {
+
+/**
+ * Configure the default settings.
+ */
+function layout_builder_manual_9401() {
+  $config_factory = \Drupal::configFactory();
+  $layout_builder_settings = $config_factory->getEditable('layout_builder.settings');
+  $layout_builder_settings->set('expose_all_field_blocks', TRUE)
+    ->save(TRUE);
+}

fwiw I think the default config defaulting to FALSE, but setting this to TRUE in the update is the right approach. We should detail this in the change record so that sites can manually set it to FALSE themselves.

We should however open a follow-up to consider other approaches - for example if we do per-field configuration, we could deprecate and drop this setting later on.

Per @catch:

[This issue] indeed should have been a stable blocker, it's a serious performance issue just having layout_builder enabled on any medium-large site.

Based on that, it maybe should be classified as a bug? And added to the scope of Standard blockers we want to complete for EOOTB.

1. +++ b/core/modules/layout_builder/src/Plugin/Derivative/ExtraFieldBlockDeriver.php
   @@ -88,14 +100,26 @@ public static function create(ContainerInterface $container, $base_plugin_id) {
   +        if (!$expose_all_fields && !in_array($bundle_id, $enabled_bundle_ids[$entity_type_id])) {
   

   should we use the third argument to in_array here because we're comparing string?

2. +++ b/core/modules/layout_builder/src/Plugin/Derivative/ExtraFieldBlockDeriver.php
   @@ -123,4 +147,17 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   +  protected function bundleIdsWithLayoutBuilderDisplays() {
   

   missing a doc block here, and a return type of array

3. +++ b/core/modules/layout_builder/src/Plugin/Derivative/FieldBlockDeriver.php
   @@ -135,4 +159,50 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   +    // Process $field_map, removing any bundles which are not using Layout
   

   this is actually removing entity-types, not bundles, the top level keys are entity-type IDs

Any thoughts on where the function from ExtraFieldBlockDeriver::bundleIdsWithLayoutBuilderDisplays() should live to be reused across both derivers and available for other devs to use externally to look up what entity types and bundles have LB enabled?

I think the two of them are different enough to keep them separate. One uses the fieldmap, the other uses the raw display plugins.

Updating remaining tasks in the issue summary

1. Get tests passing
2. Add tests for the new functionality
3. Add update path test
4. Add UI to allow someone to turn the setting off?

Please leave the above change requests with me, I'll look at them today. I might need some help on the remaining tasks though.

As a group, can we make a decision on the UI so that I can include that too (if wanted)?

Add UI to allow someone to turn the setting off?

I think not having a UI is going to be a major issue for non-developer site builders.

@nterbogt I'm happy to lend a hand.

This patch contains all the change requests from @larowlan, a functional fix to the FieldBlockDeriver (even further optimised) and all the code for setup form (permission, menu, routes, form, etc). I think the functional side of the issue is complete, just onto the tests.

Sorry I stole your comment number for my patch @jibran... accident.

Fixing tags.

What's with layout_builder_manual_9401()? That won't run on its own, have we added code like this before?

Also while this is a perfectly reasonable thing to do, I wonder how much would be mitigated by something like #3186116: Optimize \Drupal\Core\Plugin\Context\ContextHandler::checkRequirements()

It needs to be renamed to hook_update_N before merge... I changed it so that I could run this patch in production without the risk of missing actual core updates in the meantime.

I could change it back, and then patch a patch on my end, but that didn't seem very reusable or responsible for other devs... who may run the patch and then miss core update hooks.

Hey @nterbogt can you please make sure to add interdiffs to updated patches to make the review process easier?

The update here should be a post update since it's just updating configuration.

Since post update get a meaningful string name, this also means no issues with number conflicts from updates added elsewhere.

Thanks catch.

Updated patch to move to a post update.

The update here should be a post update since it's just updating configuration.

+++ b/core/modules/layout_builder/config/schema/layout_builder.schema.yml
@@ -82,3 +82,11 @@ layout_plugin.settings.layout_twocol_section:
+layout_builder.settings:

@catch Nope, it is not just updating config we are adding new schema as well. I posted this comment #3039568-46: Add a read-only mode to JSON:API back in the day. I think @alexpott mentioned it somewhere that if config schema is getting updated then we need update_N hook. Sorry, I was unable to find that conversation now.

I would add this in a hook_update_N. Until this update is applied the derived field blocks will not be as expected. I think as far as possible the system should be correct when running post updates. Until the update has run field blocks that were there before the update will not be there... and if this is a post update then they suddenly will appear again. Because you can't add dependencies on post updates if there was another post update depending on one of the field blocks being derived it would be fragile.

Lets move it back before the merge then. I don't really want an update_N in the patch because it limits our ability to use it while the issue is completed.

@nterbogt we can always upload two patches. :)

I just repeat here what I posted on slack:

I’m currently debugging a site that run on D9.5.0-rc1 and has performance issues. I found one major problem. The site uses the core layout builder and lot of webforms.
On the first page load, core tries to read and then to write cid “block_plugins” in the chained fast backend “cache_discovery”. That cache entry consists of more than 6,000 single entries, most of them start with “field_block:webform_submission:*“.
Basically any single entity field in the system has an entry here, which is already a lot. But webforms seems to dynamically add fields per form.
The problem now is that this uses a significant amount of memory and the serialization of the cache entry takes a significant amount of time. But the critical part is that it seems that the entry is too big for APCu or gets deleted quickly. I now switched to Redis as backend for “cache_discovery” which improved the page loading times. But due to the nature of that site which will add multiple webforms a week, this is just a short time solution.

For that site the issue is critical, not major. Just enabling the layout builder and starting using it for some parts of the site will potentially kill that site soon.

@mkalkbrenner in the meantime you can do something like this to filter out block plugin definitions that you don't want:

  function my_module_block_alter(array &$definitions): void {
    $removeStartsWith = [
      'field_block:webform_submission:',
    ];
    // Filter out block definitions that we don't want/need.
    $definitions = array_filter($definitions, function ($key) use ($removeStartsWith): bool {
      foreach ($removeStartsWith as $remove) {
        if (str_starts_with($key, $remove)) {
          return FALSE;
        }
      }
      return TRUE;
    }, ARRAY_FILTER_USE_KEY);
  }

You can then add more to $removeStartsWith, we actually get rid of almost every entity type's field blocks so it looks something like this:

    $removeStartsWith = [
      'field_block:block_content:',
      'field_block:microcontent:',
      'field_block:linky:',
      'field_block:media:',
      'field_block:menu_link_content:',
      'field_block:monitoring_sensor_result:',
      'field_block:paragraph:',
      'field_block:redirect:',
      'field_block:scheduled_transition:',
      'field_block:taxonomy_term:',
      'field_block:webform_submission:',
      'extra_field_block:',
    ];

@acbramley Thank you! I'll try that tomorrow. Maybe the other way around using a positve list which ones to keep. Otherwise new modules might increase the number of items again.

Ideally I think we would take a different approach to this overall problem, although even if we do that we would probably want something similar to the existing patch too. I don't know exactly how to do that in terms of one or two issues and whether there's any dependencies between them, so just trying to write it down for now.

I think (and I think @alexpott has said this somewhere) that instead of using derivers at all, we could have a single 'entity field' block plugin, where the field is part of the block configuration. Then you create instances with the fields you want but there is no pre-defined list.

However, it might be hard to write an upgrade path for config to switch from the derivative blocks to new instances, especially if sites have block-specific markup or anything like that.

This would mean we end up with the new single block on top of the derived blocks. So in that case, we would probably want new sites to start with all the derived blocks suppressed, and for existing sites, allow them to removed derived blocks as they (manually) make the switch.

Then we could deprecate the derived blocks altogether, and issue deprecation messages, maybe add phpstan checks if they're found in config, and remove them entirely in a major version, either directly or to a contrib module.

Long process, but then we'd have one block, at least to start with, instead of hundreds.

I think moving to a configurable option is a good idea *for site builders*.

However, when using Layout Builder in overrides mode, this would place that feature in the hands of a content editor.

At present the existing field block is already too overwhelming for content editors (we're asking them to chose and configure a formatter) and this would make that scenario worse.

I think what we should instead be considering is something like layout builder browser module, but extending it for field blocks.

I would see this working whereby a site builder could create a pre-configured field block using a config entity. The config entity would store the entity type, field name, bundle and pre-configured formatter. We could have one block for each of those. Then the site builder might configure e.g. a 'Short published date' and a 'Full published data' field for the content editor to place in overrides mode.

So to summarize:

* I think configurable instead of derivatives are good for site builders working on the default layout, and agree with your approach above, and that the upgrade path will be hard
* I think we can't do that without also adding something that allows site builders to pre-configure field blocks for users that includes formatter configuration

I added #3365551: Add the notion of a 'configured layout builder block' to solve a number of content-editor and performance pain points for #95 and #96 but it also solves a lot of open issues in the LB queue. @DanielVeza and I have discussed it with product managers and the subsystem maintainer and have support for the idea.

I would like to propose we push ahead with the existing solution here in #85 as even with the new field block idea there and from #95 it will still help sites running in the 'legacy mode'

Re-rolling patch for 10.1.x. Haven't checked if this is working for 10.0.

Rolling an MR, will take a look at the failures and tests next

The last 2 failures are a bit mysterious:

testBlockFilter fails for me locally on HEAD so it's hard to figure out what the missing block is that's causing the count to be off by 1.

The failure in testLayoutBuilderUi can be reproduced via manual steps. Both the layout page and the node page show a bunch of "This block is broken or missing." messages immediately after enabling layout builder on a bundle. This goes away after a drush cr so we just need to figure out which cache isn't being cleared. I confirmed via debugging that the block plugin cache clear is being hit so it's something else.

@mstrelan helped figure out the testBlockFilter failure - the test is filtering on the text "adm" which was matching a user derived field block "Preferred admin language code". Enabling the config setting fixes it.

I've debugged the final fail, and can see what's going wrong

I confirmed via debugging that the block plugin cache clear is being hit so it's something else

This is correct, but the order of operations messes things up, it goes:

1. Block plugin cache is cleared in presave
2. layout_builder_entity_presave kicks in and calls InlineBlockEntityOperations::handlePresave
3. This calls getInlineBlockComponents which gathers some block plugins
4. Since the block plugin cache was cleared in step 1, the derivatives need to be regenerated
5. This gets back into the FieldBlockDeriver, and since we're still in presave, the loadByProperties in getFieldMap doesn't find the display that we're saving!
6. We cache a list of block plugins that doesn't include the newly enabled display's bundle's fields

Needs update path tests was added way back in #25 before we had an upgrade path at all. The upgrade path is just setting a config value. IMO this does not warrant a test. Please correct me if I'm wrong.

Added specific kernel test coverage for how the setting and layout builder displays interact with the plugin derivers.

Left some small feedback.

All feedback resolved.

I assume we need a change record if we are changing existing behaviour.

Do agree with the CR since it's adding a new form and behavior change.

CR added https://www.drupal.org/node/3416592

Setup a standard install locally with layout builder installed
Applied the MR without issue
Ran the database update, which ran fine.

Went to /admin/config/content/layout-builder and verify the checkbox was checked
Went into my Article content type which is using layout builder.
All the user fields (for example) appear to be there just as they were before. I don't have layout builder enabled for users

Unchecking the setting at /admin/config/content/layout-builder
Went back into my Article content type
No user fields appeared now (Yay!)

Changes look good to me +1 RTBC for me. Per new approach for #needs-review-queue-initiative going to leave in review for a few days for additional reviews.

Gave it a few days and as a Major don't want to wait too long.

I'm triaging RTBC issues. I read the IS, the comments, the MR and the CR.

I tend to think in lists, so here is what I found.

• In #33 it is stated "by the time plugin alters run, derivers have already run". There is an issue to alter plugins early for migrate. I am adding that as a related issue.
• In #73 there is an ask for a followup. I am not 100% sure it is still needed. But I am adding the tag so that others working on this issue can decide.
• #74 says this should have been a stable blocker. I will consult with the other release managers on this.
• In #83 a reminder was given to provide interdiffs. Thank you! Without the interdiffs it requires too much time to confirm that the requested changes were made so I did not. Fortunately, there are experienced contributors here and there is now an MR,
• #95 suggests a different approach. Does this need discussion or a followup?
• This is changing the UI, adding tag.
• I read the MR (not a code review) and left comments that need to be addressed. This is changing schema and the remaining tasks asks for an update test. I do not see that test.
• I read the CR and it does not mention the UI nor performance. It should tell the reader how to use the new settings, where to find it, what is the URL, etc. Usually with a UI change including a screenshot is helpful, though not a requirement here. If it is true that this effects performance then that should be explained in the change record.
• I applied the MR and eventually figured out how to alter the setting. I found the description confusing. For me, the first two sentences are saying the same thing, that the field is always exposed. And is the 'impact' to reduce performance or increase it? I think that this needs input from Usability folks.

Forgot a tag.

See #98 for the alternative/better approach/follow up.

I will update the MR/CR tomorrow.

IMO we should not need update tests for a simple configuration set.

See #104 re update path tests. Can add one if needed but feels like overkill.

Discussed with catch and changing to Critical because this can make sites unusable.

Updated the CR, I think the only thing remaining from #113 is the field description.

Updated field description and associated screenshot.

The MR is hitting https://www.drupal.org/project/drupal/issues/3422537 and needs a clean rebase with 11.x

Believe the description contains enough detail. Wouldn't want to add much more personally.

needs a clean rebase with 11.x

Dumb question but is that different from a regular rebase?

@smustgrave some people use the word interchangeably with "merge"

I don't know how I messed up the merge so badly yesterday, MR was showing 220+ commits. Rebased and looking good now.

it may need upgrade test

Working on an upgrade path test... bit of a waste of time if you ask me but would rather just do it to get this across the line.

Believe this one is ready again,

Can see the test coverage here https://git.drupalcode.org/issue/drupal-3043330/-/jobs/875234

Ran the same tests from #111

Setup a standard install locally with layout builder installed
Applied the MR without issue
Ran the database update, which ran fine.

Went to /admin/config/content/layout-builder and verify the checkbox was checked
Went into my Article content type which is using layout builder.
All the user fields (for example) appear to be there just as they were before. I don't have layout builder enabled for users

Unchecking the setting at /admin/config/content/layout-builder
Went back into my Article content type
No user fields appeared now (Yay!)

Added some review comments. We need a config subscriber to clear the block manager cache. It is tempting to add this to \Drupal\layout_builder\Cache\ExtraFieldBlockCacheTagInvalidator and rename it to something about cache management.

Back to NR, just need a decision on the br tags or not.

Feedback has been addressed

Believe with the
it definitely makes it easier to read the description.

Just realised we've missing test coverage of the config form being added. Given there are plans to add more to this form and config object I think it'd be nice to make sure this form is working as we think it should.

Another thing I thought was too trivial to warrant a test 😅

Will get it sorted soon.

Test coverage has been added for the form. Looks good to me

Committed and pushed 1561edc2b8 to 11.x and 9f4b1f4e1c to 10.3.x. Thanks!

I think we should consider adding this to the release notes so users who don't use this functionality are alerted that they can turn it off.

Thanks very much, this one was near and dear to me 😅

I can have a crack at the release note, I'll have a look round for something as a guide.

Congrats on landing this! And yay for using #config_target! 😄

I created the follow-up #3426429: Mark layout_builder.settings fully validatable to ensure the brand new layout_builder.settings is fully validatable from the first release it ships with.
(Note: if #3422641: Add a "Validatable config" tests job to GitLab CI to help core evolve towards 100% validatability had already landed, then that CI job would've automatically detected that we're getting a lower ratio of validatable config.)

P.S.: AFAICT this means we can now close #3240819: Drupal Quickstart command runs into php memory limits, especially with demo_umami profile? 🤞

I was following along this issue loosely, but never realized that there were front facing changes - always thought the changes would only be under the hood. After it got committed I just noticed that the MR also added a new configuration page. Taking a look at the microcopy I think that there might be room for improvement - in regards of clarity and comprehension. Personally I consider it challenging to completely understand the consequences of that configuration based on the checkbox label and the description. Therefore I agree with @quiteone in #113 that it might be a good idea to discuss the microcopy in a usability meeting. I've already set it on the agenda #3424764-2: Drupal Usability Meeting 2024-03-08. In today's meeting we haven't had enough time left to discuss the issue, but we can hopefully get to it next Friday. Since this issue is already committed, in case we come up with a suggestion for a potential improvement, shall we open up a follow up issue about the proposed changes?

Yes please to follow up

As a novice Layout Builder user I did not understand the change record: it seemed safe to always disable that checkbox, which made me wonder why we even have it.

So I grepped this issue for "BC" and found a counterexample by @tim.plunkett 👍 Can we update the change record to clarify this? 🙏 A few examples of why you can't just uncheck that checkbox, and what it would take for an existing site to eventually uncheck it.

@Wim Leers thanks for pointing that out. I've added an example to the CR.

We reviewed this issue at #3426532: Drupal Usability Meeting 2024-03-15. That issue will have a link to a recording of the meeting.

For the record, the attendees at the usability meeting were @AaronMcHale, @blackbamboo, @rkoller, @simohell, @SKAUGHT, and @worldlinemine.

Per @larowlan's suggestion in #142 I've created a followup issue summarizing the outcomes of our discussion: #3431164: [meta] Improve the "Expose all fields as blocks to Layout Builder" feature

Should this be marked fixed?

@smustgrave we need to address #136 before we can close.

Would the release note go here or on this ticket which removes most of this change https://www.drupal.org/project/drupal/issues/3432874

#3432874: Replace "Expose all fields as blocks to Layout Builder" configuration with a feature flag module doesn't remove the important part of this change - it only changes how it is configured.

Added a release note, it's written as per the current config but we can change it in #3432874: Replace "Expose all fields as blocks to Layout Builder" configuration with a feature flag module

Release note seems fine to me.

Thanks.