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

Created on 26 March 2019, over 5 years ago
Updated 8 April 2024, 7 months ago

Problem/Motivation

Since having lots of block derivatives has various implications for performance, it would be great to only create field blocks for entities that are customised with layout builder.

I have a site where 1600 block plugins are being created for webform submission bundles, which will never require field blocks. This number will grow for every webform added to the site.

Proposed resolution

Use a new configuration setting, expose_all_field_blocks to ensure FieldBlockDeriver and ExtraFieldBlockDeriver only creates blocks it needs.

Remaining tasks

  1. - Not needed unless confirmed otherwise

User interface changes

New configuration

API changes

Data model changes

Release notes snippet

Layout builder now only exposes field blocks for bundles that have layout builder enabled. To expose field blocks for all bundles, enable the layout_builder.expose_all_field_blocks setting.

πŸ› Bug report
Status

Fixed

Version

10.3 ✨

Component
Layout builderΒ  β†’

Last updated about 3 hours ago

Created by

πŸ‡¦πŸ‡ΊAustralia Sam152

Live updates comments and jobs are added and updated live.
  • Blocks-Layouts

    Blocks and Layouts Initiative. See the #2811175 Add layouts to Drupal issue.

  • Performance

    It affects performance. It is often combined with the Needs profiling tag.

  • Usability

    Makes Drupal easier to use. Preferred over UX, D7UX, etc.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡¬πŸ‡§United Kingdom catch

    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.

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    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

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    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'

  • πŸ‡¦πŸ‡ΊAustralia nterbogt

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

  • last update about 1 year ago
    Patch Failed to Apply
  • last update about 1 year ago
    30,409 pass, 7 fail
  • last update about 1 year ago
    29,666 pass, 7 fail
  • πŸ‡¦πŸ‡ΊAustralia acbramley

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

  • Pipeline finished with Failed
    10 months ago
    Total: 586s
    #75953
  • Pipeline finished with Failed
    10 months ago
    Total: 741s
    #75955
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    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.

  • Pipeline finished with Failed
    10 months ago
    Total: 618s
    #77109
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    @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
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    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.

  • Pipeline finished with Canceled
    10 months ago
    Total: 82s
    #77132
  • Pipeline finished with Success
    10 months ago
    Total: 497s
    #77133
  • Status changed to Needs review 10 months ago
  • πŸ‡¦πŸ‡ΊAustralia acbramley

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

  • Pipeline finished with Success
    10 months ago
    Total: 676s
    #77148
  • Status changed to Needs work 10 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Left some small feedback.

  • Status changed to Needs review 10 months ago
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    All feedback resolved.

  • Pipeline finished with Success
    10 months ago
    Total: 656s
    #79597
  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

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

  • Status changed to Needs work 10 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

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

  • Status changed to Needs review 10 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    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.

  • Status changed to RTBC 10 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

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

  • Status changed to Needs work 9 months ago
  • πŸ‡³πŸ‡ΏNew Zealand quietone

    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.
  • πŸ‡³πŸ‡ΏNew Zealand quietone

    Forgot a tag.

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    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.

  • Pipeline finished with Success
    9 months ago
    Total: 573s
    #99092
  • Status changed to Needs review 9 months ago
  • πŸ‡¦πŸ‡ΊAustralia acbramley

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

  • πŸ‡³πŸ‡ΏNew Zealand quietone

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

  • πŸ‡¦πŸ‡ΊAustralia acbramley

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

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    Updated field description and associated screenshot.

  • Pipeline finished with Failed
    9 months ago
    Total: 913s
    #100020
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    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

  • Status changed to Needs work 9 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    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?

  • Status changed to Needs review 9 months ago
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    @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.

  • Pipeline finished with Success
    9 months ago
    Total: 637s
    #101004
  • πŸ‡«πŸ‡·France andypost

    it may need upgrade test

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    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.

  • Pipeline finished with Success
    9 months ago
    Total: 754s
    #101052
  • Status changed to RTBC 9 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    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!)

  • Status changed to Needs work 9 months ago
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    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.

  • Status changed to Needs review 9 months ago
  • πŸ‡¦πŸ‡ΊAustralia acbramley

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

  • Pipeline finished with Success
    9 months ago
    Total: 481s
    #109846
  • Status changed to RTBC 8 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Feedback has been addressed

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

  • Status changed to Needs work 8 months ago
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    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.

  • Pipeline finished with Success
    8 months ago
    Total: 647s
    #113286
  • Assigned to acbramley
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    Another thing I thought was too trivial to warrant a test πŸ˜…

    Will get it sorted soon.

  • Issue was unassigned.
  • Status changed to Needs review 8 months ago
  • πŸ‡¦πŸ‡ΊAustralia acbramley
  • Pipeline finished with Success
    8 months ago
    Total: 534s
    #113325
  • Status changed to RTBC 8 months ago
  • πŸ‡³πŸ‡ΏNew Zealand danielveza Brisbane, AU

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

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

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

    • alexpott β†’ committed 9f4b1f4e on 10.3.x
      Issue #3043330 by acbramley, nterbogt, phenaproxima, Sam152, tim....
  • Status changed to Fixed 8 months ago
  • Status changed to Needs work 8 months ago
  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    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.

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    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.

  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    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 ? 🀞

  • πŸ‡¬πŸ‡§United Kingdom catch
  • πŸ‡©πŸ‡ͺGermany rkoller NΓΌrnberg, Germany

    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) Fixed 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?

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    Yes please to follow up

  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    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.

  • πŸ‡¦πŸ‡ΊAustralia acbramley

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

  • πŸ‡©πŸ‡ͺGermany rkoller NΓΌrnberg, Germany

    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) Fixed 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

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Should this be marked fixed?

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

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

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    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

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    πŸ“Œ 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.

  • Status changed to Needs review 8 months ago
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    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

  • Status changed to RTBC 8 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Release note seems fine to me.

  • Status changed to Fixed 8 months ago
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    Thanks.

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

Production build 0.71.5 2024