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

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.

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

+++ 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()?

Added patch fixed CCf #33

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.

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

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

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

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

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

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?

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.

Rebased MR 7340

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

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.

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

@acbramley I am sure it would.

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

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.

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

@tim.plunkett Not interested in your opinion.

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?

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

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

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

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

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

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

@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

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

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.

Rebased

Attempt to simplify the title after reading through the comments.

Although the IS mentions sort of 'knock-on' effects of the main issue, might help to distinguish which of those 'knock-on's' getting 'knocked on the head' by this issue. Are there any follow-ups required?

I think the previous title was a bit more succinct in what we're fixing here.

The "knock on" effects are exactly what we're fixing here, the performance benefits of simply not attempting to build any sections if layout builder is not enabled are the fix.

I've updated the IS a bit.

@acbramley Okay. I should have said 'knock-on' effects described in comments #1 - #7 involving 'contrib' modules and various hooks. Perhaps those comments are not relevant now? They date from 2019. Or are they addressed in the MR?