Layout Builder attempts to builds section to determine if it is disabled

Created on 11 June 2019, about 6 years ago
Updated 30 January 2023, over 2 years ago

Problem/Motivation

As long as the module is installed, \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::buildMultiple() will run for every entity displayed on the site.
The concept of Layout Builder "being enabled" for a given view mode is actually determined by any number of plugins, only 2 of which are provided by core.

This presents a performance hit for cases where the entity is rendered multiple times, or when it is possible to know that no SectionStorage plugin is active.

Proposed resolution

Allow SectionStorage plugins to indicate that they are not relevant for a given entity display (entity type, bundle, and view mode).

Remaining tasks

User interface changes

N/A

API changes

TBD

Data model changes

N/A

Release notes snippet

Original report

We ran into notices on a client site, which result in a combination of content moderation, layout builder, tokens module and pathauto. The notice appeared after saving a new node, and was triggered by token generation of path auto. Problem is that, layout builders builds its sections in \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::buildMultiple() - even if it's not enabled.

Another issue / probably a bug is that this triggers entity validation, i.e. when layout builder tries to validate whether a context is satisfied, it validates the entity. Potentially another bug - maybe there is already some issue for it?

This in turn lets the content moderation constraint try to load revision without ID (NULL), what results in the notice. Potentially another bug.

Warning: array_flip(): Can only flip STRING and INTEGER values! in Drupal\Core\Entity\ContentEntityStorageBase->loadMultipleRevisions() (line 584 of core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php).

Drupal\Core\Entity\ContentEntityStorageBase->loadMultipleRevisions(Array) (Line: 551)
Drupal\Core\Entity\ContentEntityStorageBase->loadRevision(NULL) (Line: 156)
Drupal\content_moderation\Plugin\Validation\Constraint\ModerationStateConstraintValidator->getOriginalOrInitialState(Object) (Line: 113)
Drupal\content_moderation\Plugin\Validation\Constraint\ModerationStateConstraintValidator->validate(Object, Object) (Line: 191)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateConstraints(Object, '00000000438d59cb0000000052d7805e', Array) (Line: 146)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode(Object) (Line: 153)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode(Object, Array, 1) (Line: 99)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validate(Object, Array, NULL) (Line: 90)
Drupal\Core\TypedData\Validation\RecursiveValidator->validate(Object, Array) (Line: 368)
Drupal\Core\Plugin\Context\ContextDefinition->isSatisfiedBy(Object) (Line: 77)
Drupal\Core\Plugin\Context\ContextHandler->Drupal\Core\Plugin\Context\{closure}(Object)
array_filter(Array, Object) (Line: 78)
Drupal\Core\Plugin\Context\ContextHandler->getMatchingContexts(Array, Object) (Line: 65)
Drupal\Core\Plugin\Context\ContextHandler->checkRequirements(Array, Array) (Line: 31)
Drupal\Core\Plugin\Context\ContextHandler->Drupal\Core\Plugin\Context\{closure}(Object)
array_filter(Array, Object) (Line: 37)
Drupal\Core\Plugin\Context\ContextHandler->filterPluginDefinitionsByContexts(Array, Array) (Line: 92)
Drupal\layout_builder\SectionStorage\SectionStorageManager->findByContext(Array, Object) (Line: 297)
Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay->buildSections(Object) (Line: 261)
Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay->buildMultiple(Array) (Line: 28)
Drupal\custom_elements\CustomElementsLayoutBuilderEntityViewDisplay->buildMultiple(Array) (Line: 221)
Drupal\Core\Entity\Entity\EntityViewDisplay->build(Object) (Line: 456)
Drupal\Core\Entity\EntityViewBuilder->viewField(Object, Array) (Line: 243)
Drupal\Core\Field\FieldItemList->view(Array) (Line: 1626)
field_tokens('entity', Array, Array, Array, Object)
call_user_func_array('field_tokens', Array) (Line: 403)
Drupal\Core\Extension\ModuleHandler->invokeAll('tokens', Array) (Line: 304)
Drupal\Core\Utility\Token->generate('entity', Array, Array, Array, Object) (Line: 941)
token_tokens('node', Array, Array, Array, Object)
call_user_func_array('token_tokens', Array) (Line: 403)
Drupal\Core\Extension\ModuleHandler->invokeAll('tokens', Array) (Line: 304)
Drupal\Core\Utility\Token->generate('node', Array, Array, Array, Object) (Line: 196)
Drupal\Core\Utility\Token->replace('[node:field_seo_title]', Array, Array, Object) (Line: 212)
Drupal\pathauto\PathautoGenerator->createEntityAlias(Object, 'insert') (Line: 364)
Drupal\pathauto\PathautoGenerator->updateEntityAlias(Object, 'insert') (Line: 86)
pathauto_entity_insert(Object)
call_user_func_array('pathauto_entity_insert', Array) (Line: 403)
Drupal\Core\Extension\ModuleHandler->invokeAll('entity_insert', Array) (Line: 206)
Drupal\Core\Entity\EntityStorageBase->invokeHook('insert', Object) (Line: 835)
Drupal\Core\Entity\ContentEntityStorageBase->invokeHook('insert', Object) (Line: 526)
Drupal\Core\Entity\EntityStorageBase->doPostSave(Object, ) (Line: 720)
Drupal\Core\Entity\ContentEntityStorageBase->doPostSave(Object, ) (Line: 303)
Drupal\multiversion\Entity\Storage\Sql\NodeStorage->doPostSave(Object, ) (Line: 451)
Drupal\Core\Entity\EntityStorageBase->save(Object) (Line: 838)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->save(Object) (Line: 240)
Drupal\multiversion\Entity\Storage\Sql\NodeStorage->save(Object) (Line: 394)
Drupal\Core\Entity\EntityBase->save() (Line: 293)
Drupal\node\NodeForm->save(Array, Object)
call_user_func_array(Array, Array) (Line: 111)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers(Array, Object) (Line: 51)

Anyway, the root of the problem is that layout builder does stuff when it should not, so let's try to fix this here.

๐Ÿ› Bug report
Status

Needs work

Version

10.1 โœจ

Component
Layout builderย  โ†’

Last updated 2 days ago

Created by

๐Ÿ‡ฆ๐Ÿ‡นAustria fago Vienna

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.

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.

  • The Needs Review Queue Bot โ†’ tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

    Consult the Drupal Contributor Guide โ†’ to find step-by-step guides for working with issues.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    Reroll #29 against 9.5.x and added some defense for OverridesSectionStorage::isSupported.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia kim.pepper ๐Ÿ„โ€โ™‚๏ธ๐Ÿ‡ฆ๐Ÿ‡บSydney, Australia
    +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/DefaultsSectionStorage.php
    @@ -407,4 +409,31 @@ public function setContext($name, ComponentContextInterface $context) {
    +    if ($bundle) {
    ...
    +    }
    

    I feel like this could be its own function, e.g. getSectionId()?

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia Akram Khan Cuttack, Odisha

    Added patch fixed CCf #33

  • Status changed to Needs review over 2 years ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia Akram Khan Cuttack, Odisha
  • Status changed to Needs work over 2 years ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request โ†’ as a guide.

    As a bug this will need a test case showing the issue.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    Rerolled onto an MR and fixed up a few typing/code style things.

  • Merge request !7340Issue #3060985 โ†’ (Open) created by acbramley
  • Pipeline finished with Success
    over 1 year ago
    Total: 616s
    #138211
  • Status changed to Needs review 3 months ago
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    Rebased, fixed some minor issues, and added test coverage.

  • Pipeline finished with Canceled
    3 months ago
    Total: 157s
    #474515
  • Pipeline finished with Failed
    3 months ago
    Total: 951s
    #474517
  • Pipeline finished with Success
    3 months ago
    Total: 1002s
    #474533
  • Pipeline finished with Canceled
    3 months ago
    Total: 261s
    #475532
  • Pipeline finished with Success
    3 months ago
    #475535
  • Pipeline finished with Canceled
    3 months ago
    Total: 122s
    #475552
  • Pipeline finished with Success
    3 months ago
    Total: 681s
    #475553
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Seems already been reviewed and feedback has been addressed. Didn't see anything additional.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    Agreed with @kimpepper's feedback on the MR. Didn't do an in-depth review of everything.

  • Pipeline finished with Failed
    2 months ago
    Total: 138s
    #490733
  • Pipeline finished with Failed
    2 months ago
    Total: 161s
    #490736
  • Pipeline finished with Failed
    2 months ago
    #490747
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    Much nicer solution, thanks for the links. I didn't see any decisions on ๐ŸŒฑ [policy] Standardize how we implement in-memory caches Needs work with how these memory cache services should be setup wrt. service id names or how specific/generic they should be but I've loosely tried to follow what other things are doing in core already with cache.asset_memory and system.module_admin_links_memory_cache

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    I've tried passing the display's cache tags into the set() call so, in theory, it should be invalidated automatically when the display is saved but that doesn't seem to be the case. Must be missing something?

  • Pipeline finished with Failed
    2 months ago
    #490776
  • Pipeline finished with Success
    2 months ago
    Total: 549s
    #490781
  • The Needs Review Queue Bot โ†’ tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide โ†’ to find step-by-step guides for working with issues.

  • First commit to issue fork.
  • Pipeline finished with Failed
    about 2 months ago
    Total: 499s
    #495096
  • Pipeline finished with Success
    about 2 months ago
    Total: 637s
    #495110
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States dcam

    Rebased MR 7340

  • Pipeline finished with Failed
    about 2 months ago
    Total: 258s
    #495353
  • ๐Ÿ‡ณ๐Ÿ‡ฟNew Zealand danielveza Brisbane, AU

    This has gone through multiple rounds of review, all threads have been resolved. I think it's ready to be RTBC.

  • Pipeline finished with Success
    about 2 months ago
    Total: 1107s
    #495369
  • Status changed to Needs work 11 days ago
  • The Needs Review Queue Bot โ†’ tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide โ†’ to find step-by-step guides for working with issues.

  • First commit to issue fork.
  • Pipeline finished with Failed
    11 days ago
    Total: 5997s
    #531617
  • Pipeline finished with Success
    11 days ago
    Total: 590s
    #531737
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    @oily thank you for rebasing many of my issues, it would be great if you could also ensure tests pass and follow up any failures in the future.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom oily Greater London

    @acbramley I am sure it would.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    @oily I don't appreciate the snark mate. I'm just trying to give some advice.

    I've seen you rebasing issues a lot over the past few days, some of which didn't need rebasing. If you're trying to help move an issue along and gain credit, doing unnecessary rebases is listed under Examples of what will usually not receive credit on https://www.drupal.org/about/core/policies/maintainers/how-credit-is-gra... โ†’ .

    This one did have a conflict in OpenTelemetryNodePagePerformanceTest, which it seems like you fixed, but then the fix failed tests.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom oily Greater London

    You were dressing up with a thin veneer of politeness impudent and unwelcome advice. Now that the veneer is removed. And you try to discredit my contributions and my professional image in front of a potentially wide audience. I have no problem with the rules.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States tim.plunkett Philadelphia

    @oily except that @acbramley's advice here is appreciated and welcomed, and correct.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom oily Greater London

    @tim.plunkett Not interested in your opinion.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom oily Greater London

    RE: #54 'This one did have a conflict in OpenTelemetryNodePagePerformanceTest, which it seems like you fixed, but then the fix failed tests.'

    I do not know how to fix the tests. Anything wrong with that?

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States volkswagenchick San Francisco Bay Area

    It seems that emotions in this discussion may be escalating, which can lead to misunderstandings and unproductive exchanges. To ensure that all participants are heard and respected, we encourage a brief pause from the conversation to gain perspective. It's essential that every member of the community feels valued and respected during collaboration.

    In our community, we strive to be constructively honest and relentlessly optimistic. This means taking the time to understand decisions and the reasoning behind them before expressing disagreement. We ask that you suspend judgment and actively listen, asking questions and engaging with openness. Please be mindful of the tone and impact of your comments, as even well-intended messages can be misinterpreted.

    If you need support navigating this discussion, the Drupal community offers resources for conflict resolution. We encourage you to take advantage of these tools to maintain a positive and productive dialogue.

    For more information, please refer to Drupalโ€™s Values and Principles of be constructively honest, and relentlessly optimistic โ†’ and Drupalโ€™s Values and Principles of seeking first to understand, then to be understood โ†’ . Additionally, there are resources offered by the Drupal community to aid conflict resolution โ†’ should those be needed.

    This reminder is provided by the Drupal Community Health Team as part of an initiative to foster positive discourse. For more information, please visit https://www.drupal.org/project/drupal_cwg/issues/3129687 โ†’ .

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    @oily I think you've misunderstood where I'm coming from here.

    I do not know how to fix the tests. Anything wrong with that?

    No there's not, I'm happy to help if you would like to understand that more.

    I tried to resolve a merge conflict in this issue. Anything wrong with that?

    Of course not, I was referring to the issues that did not have conflicts.

    If people want to build a list of black marks against my contributions

    Nobody is doing that.

    I believe they were nearly all 90+ commits behind 11.x when I merged.

    This is what I was trying to explain, there is no need to rebase random issues that are X commits behind HEAD. As I understand it, you are looking for issues to work on to gain credits, that's fine, but rebasing issues will not achieve a credit. I rebase issues that I am working on myself when I come back to them and they are behind, there is no reason for me to go searching for issues to click the rebase button on, that is just wasted effort.

    I would really recommend you find something that you are interested in working on, either specific issues or a specific subsystem, and try to contribute in other, more valuable ways. If you give me an idea on what you are interested in then I can try to find some issues that may suit. For example you've mentioned a few times that you don't know how to write tests, we could look for some issues with the Needs tests tag and go through how to write a test for them.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom oily Greater London

    #59 "..To ensure that all participants are heard and respected, we encourage a brief pause from the conversation to gain perspective.."

    So why #60? I am following #59. Suggest the others involved do the same.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom oily Greater London

    Chaging to needs work after creating a review of the code. A couple of perhaps nitty suggestions on code comments.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom oily Greater London
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    I don't agree with the suggestion on the added interface, the other one is for existing documentation so needs a separate issue.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom oily Greater London

    Re: #64 Can you please explain why. Please reference the comments I have made. A reasoned response is much appreciated.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    Left some comments on the MR
    I realise both @kim.pepper and @catch agreed that using memory cache over static cache was preferred, but given we already have a memory cache for entities and the isLayoutBuilderEnabled call is fairly light weight, I'm not sure whether we're doing premature optimisation here at the cost of added complexity.

    It would be good to see what impact just using the existing memory cache has

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    @acbramley pointed out that the choice of notSupported vs isSupported was mentioned in #6, so feel to disregard that feedback - I don't feel that strongly about it

  • Pipeline finished with Failed
    7 days ago
    Total: 339s
    #534664
  • Pipeline finished with Success
    7 days ago
    Total: 819s
    #534670
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    All feedback has been addressed, thanks for the review @larowlan

Production build 0.71.5 2024