Problem/Motivation

#3060985: Layout Builder attempts to builds sections to determine if it is disabled was meant to remove considerable unnecessary in layout builder when it's not enabled for a certain view mode/entity being rendered.

However, the solution there only works when all section storage plugins implement this interface and return appropriately.

In #3418328: Optimise Layout builder EntityViewDisplay I identified that navigation does not, and so as soon as the navigation module is enabled, the previous gains from the linked issue vanish, as we could see in #3560118: In Umami, replace Toolbar with Navigation

The performance test changes here are a straight revert of the changes in the previous issue, this is because they test anonymous users that do not have access to either navigation nor toolbar, so it makes no difference with module is enabled.

To be investigated: Is the problem maybe instead/also that \Drupal\layout_builder\SectionStorageInterface::isApplicable incorrectly always returns TRUE, when it should only be applicable to it's own specific context?

Steps to reproduce

Proposed resolution

Additionally to this, do we want to deprecate not implementing this interface and integrate it into the main one in D13?

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3574461

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

berdir created an issue. See original summary.

berdir opened merge request !14828

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs work

Moved over the MR from the other issue, this doesn't do the deprecation yet.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review

I had a look at isApplicable(), that happens later when the (too) expensive context stuff did already run. Pretty confusing to have both, but not much we can do about that.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Possible to get a test case around the new trigger_error. Layout builder is so tricky want to make sure that does get seen correctly.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

We don't need a test just to check that @trigger_error gets called IMO, there's no actual logic there to test as such.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

It does need a rebase on performance tests though. Was hoping to get this in with a clean revert, that makes it a bit more tedious.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review

Rebased the performance test and adjusted the numbers.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Since this doesn't need test coverage LGTM.

catch closed merge request !14828

  • catch committed cd4b7ce4 on 11.x 
    task: #3574461 Deprecate not implementing \Drupal\layout_builder\...

  • catch committed f08dfd49 on main 
    task: #3574461 Deprecate not implementing \Drupal\layout_builder\...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Well it has test coverage, just not for the trigger_error().

Committed/pushed to main and 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

quietone’s picture

quietone commented

Tweaked the CR and published.

Status: Fixed » Closed (fixed)

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