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 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()
? - Status changed to Needs review
over 2 years ago 12:49pm 8 February 2023 - Status changed to Needs work
over 2 years ago 3:32pm 14 February 2023 - ๐บ๐ธ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.
- Status changed to Needs review
3 months ago 2:49am 16 April 2025 - ๐ฆ๐บAustralia acbramley
Rebased, fixed some minor issues, and added test coverage.
- ๐บ๐ธ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.
- ๐ฆ๐บ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
andsystem.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?
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.
- ๐ณ๐ฟ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.
- Status changed to Needs work
11 days ago 7:22pm 25 June 2025 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.
- ๐ฆ๐บ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.
- ๐ฆ๐บ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.
- ๐ฆ๐บ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 theisLayoutBuilderEnabled
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 acbramley
All feedback has been addressed, thanks for the review @larowlan