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

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 ✨ Add the notion of a 'configured layout builder block' to solve a number of content-editor and performance pain points Active 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 🐛 Increment or remove GIT_DEPTH limit for GitlabCi Active 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.

acbramley → closed merge request !6132

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 📌 Mark layout_builder.settings fully validatable Needs review to ensure the brand new layout_builder.settings is fully validatable from the first release it ships with. (Note: if ✨ Add a "Validatable config" tests job to GitLab CI to help core evolve towards 100% validatability Fixed had 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 🐛 Drupal Quickstart command runs into php memory limits, especially with demo_umami profile Postponed: needs info ? 🤞

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 🐛 Reduce the number of field blocks created for entities (possibly to zero) Needs review 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 📌 Drupal Usability Meeting 2024-03-15 Active . 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 🐛 Reduce the number of field blocks created for entities (possibly to zero) Needs review I've created a followup issue summarizing the outcomes of our discussion: 📌 [meta] Improve the "Expose all fields as blocks to Layout Builder" feature Active

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 📌 Replace "Expose all fields as blocks to Layout Builder" configuration with feature flag Active

📌 Replace "Expose all fields as blocks to Layout Builder" configuration with feature flag Active 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 📌 Replace "Expose all fields as blocks to Layout Builder" configuration with feature flag Active

Release note seems fine to me.

Thanks.

Automatically closed - issue fixed for 2 weeks with no activity.