Allow layouts to provide dynamic regions

Interesting idea.

would allow devs or contrib modules to develop custom complex rules of layout building, and store them in layouts configurations

A proof of concept of this would be helpful. For example, how would you build on #8 to solve #3053145: Allow layout builder sections to be nested.?

It works, I'm already doing it.

Are you able to share that code? If not, that's a shame but understandable. But if you are, it would be helpful in reviewing #8 to see #10 in action.

#8 patch does not apply

So far this seems like a very promising approach. I've tested the patch in #18 and it says it didn't apply (when I try to apply patch via composer), however I see the changes are being applied. Here's the output from my the patch failure:

patching file lib/Drupal/Core/Layout/DynamicRegionsProviderInterface.php

patching file lib/Drupal/Core/Layout/LayoutDefault.php
Hunk #1 succeeded at 31 (offset -2 lines).
patching file lib/Drupal/Core/Layout/LayoutDefinition.php

patching file modules/layout_builder/src/Element/LayoutBuilder.php

Hunk #2 succeeded at 248 (offset -3 lines).

Hunk #3 FAILED at 284.

1 out of 3 hunks FAILED
 -- saving rejects to file modules/layout_builder/src/Element/LayoutBuilder.php.rej


patching file modules/layout_builder/src/Form/MoveBlockForm.php

Hunk #2 FAILED at 127.

1 out of 2 hunks FAILED -- saving rejects to file modules/layout_builder/src/Form/MoveBlockForm.php.rej


patching file modules/layout_builder/tests/modules/layout_builder_test/src/Plugin/Layout/LayoutBuilderTestDynamicRegionsPlugin.php

patching file modules/layout_builder/tests/modules/layout_builder_test/templates/layout-builder-test-dynamic-regions-section.html.twig

patching file modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php

Hunk #1 succeeded at 948 (offset -48 lines).

patch '-p4' --no-backup-if-mismatch -d 'web/core' < '/var/folders/4v/z5dd6lyj6r397nfwm_qwtxqc0000gn/T/5d6850ea455fb.patch'
Executing command (CWD): patch '-p4' --no-backup-if-mismatch -d 'web/core' < '/var/folders/4v/z5dd6lyj6r397nfwm_qwtxqc0000gn/T/5d6850ea455fb.patch'
patching file Core/Layout/DynamicRegionsProviderInterface.php

can't find file to patch at input line 32
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/core/lib/Drupal/Core/Layout/LayoutDefault.php b/core/lib/Drupal/Core/Layout/LayoutDefault.php
|index 175844fe1e..296aac6cb9 100644
|--- a/core/lib/Drupal/Core/Layout/LayoutDefault.php
|+++ b/core/lib/Drupal/Core/Layout/LayoutDefault.php
--------------------------
File to patch: 
Skip this patch? [y] 
Skipping patch.
1 out of 1 hunk ignored

can't find file to patch at input line 51
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/core/lib/Drupal/Core/Layout/LayoutDefinition.php b/core/lib/Drupal/Core/Layout/LayoutDefinition.php
|index c87b618d11..de20c3c212 100644
|--- a/core/lib/Drupal/Core/Layout/LayoutDefinition.php
|+++ b/core/lib/Drupal/Core/Layout/LayoutDefinition.php
--------------------------
File to patch: 
Skip this patch? [y] 
Skipping patch.

2 out of 2 hunks ignored

can't find file to patch at input line 85
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------

|diff --git a/core/modules/layout_builder/src/Element/LayoutBuilder.php b/core/modules/layout_builder/src/Element/LayoutBuilder.php
|index 80dcff90f7..8388352014 100644
|--- a/core/modules/layout_builder/src/Element/LayoutBuilder.php
|+++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
--------------------------
File to patch: 
Skip this patch? [y] 
Skipping patch.

3 out of 3 hunks ignored

can't find file to patch at input line 121
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/core/modules/layout_builder/src/Form/MoveBlockForm.php b/core/modules/layout_builder/src/Form/MoveBlockForm.php
|index b3fd9dcbf8..da2543baad 100644
|--- a/core/modules/layout_builder/src/Form/MoveBlockForm.php
|+++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
--------------------------
File to patch: 
Skip this patch? [y] 
Skipping patch.

2 out of 2 hunks ignored

patching file tests/modules/layout_builder_test/src/Plugin/Layout/LayoutBuilderTestDynamicRegionsPlugin.php

patching file tests/modules/layout_builder_test/templates/layout-builder-test-dynamic-regions-section.html.twig

can't find file to patch at input line 206
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------

|diff --git a/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
|index c5efc3110d..e08faf1db8 100644
|--- a/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
|+++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
--------------------------
File to patch: 
Skip this patch? [y] 
Skipping patch.

1 out of 1 hunk ignored

   Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2019-08-27/implement_dynamic_layout_regions-3075939-18.patch

I do have it working in a test environment, and implemented the functionality from your sandbox module. Everything seems to be working, not sure why the patch is failing.

Thanks for your work Luke_Neke.

No I was running against latest core stable release 8.7.6 - Aug 07 2019. You're probably right about it being a local issue. Either way, the patch DOES seem to apply as the files have the changes and the new files are where they need to be.

I've tested the functionality and it works great. It seems to be a promising approach to nested subsections and something the will become a realized need as more and more people start using layouts. Sure you can build endless layouts ahead of time, but sometimes you don't know what layout you need until you're actually building the page. This offers site builders that flexibility.

We'd likely need some method in our custom layout class for subsection removals. I wonder how you might handle that. Overall great work so far.

Still needs a review. Just tagging.

1. +++ b/core/lib/Drupal/Core/Layout/LayoutDefault.php
   @@ -33,7 +33,13 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   +    $regionNames = $this->getPluginDefinition()->getRegionNames();
   +    if ($this instanceof DynamicRegionsProviderInterface) {
   +      $regionNames = array_merge(array_keys($this->getDynamicRegions()), $regionNames);
   +    }
   

   This is the crux of the patch, but still doesn't feel right to me.

   Consider that this is changing only one of the 8 places that getRegionNames() is called in core.

   One other idea would be to add a getRegionNames() method to LayoutInterface, and have LayoutDefault delegate to the plugin, allowing classes to provide their own implementation. And then document on the plugin's method that it is only intended for internal use...

2. +++ b/core/lib/Drupal/Core/Layout/LayoutDefault.php
   @@ -33,7 +33,13 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   +    foreach ($regionNames as $region_name) {
   

   Nit: should be $region_names

3. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -250,8 +251,12 @@ protected function buildAdministrativeSection(SectionStorageInterface $section_s
   +    if ($layout instanceof DynamicRegionsProviderInterface) {
   +      $regions = array_merge($layout->getDynamicRegions(), $regions);
   +    }
   

   Similar problem to the above

4. +++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
   @@ -126,7 +127,13 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
   +      $regions = $layout_definition->getRegions();
   +      if ($layout instanceof DynamicRegionsProviderInterface) {
   +        $regions = array_merge($layout->getDynamicRegions(), $regions);
   +      }
   

   And again

5. +++ b/core/modules/layout_builder/tests/modules/layout_builder_test/src/Plugin/Layout/LayoutBuilderTestDynamicRegionsPlugin.php
   @@ -0,0 +1,38 @@
   + *   template = "layout-builder-test-dynamic-regions-section"
   

   Nit: include trailing comma

6. +++ b/core/modules/layout_builder/tests/modules/layout_builder_test/templates/layout-builder-test-dynamic-regions-section.html.twig
   @@ -0,0 +1,9 @@
   +    {% if content.dynamicregion %}
   +      <div {{ region_attributes.dynamicregion }}>
   +        {{ content.dynamicregion }}
   +      </div>
   

   Ideally this would actually be dynamic, and not need to be hardcoded. Compare to layout.html.twig which uses getRegionNames dynamically right now.

Nothing in the issue fork...

I performed Novice Triage on this issue. I added the Easy out of the Box, NorthAmerica2021, Novice and Needs issue summary update tags.

I think the issue summary could use an update based on some pending questions brought up in Tim's review. Additionally the patch probably needs to be re-rolled at this point and we can fix the issues in the code review.

After that, start a thread in the #easy-out-of-the-box Slack channel to ask for help / discuss Tim's 1st point in the review in #30.

Just a re-roll against 9.2.x.

Looks like we need to add "dynamicregions" to the cspell dictionary following instructions in this change record: https://www.drupal.org/node/3122084

Back to Needs work, removed needs reroll.

I'm going to apply the patch and make the changes, test, then upload a new patch.

I added dynamicregions to the cspell dictionary and recombined that with the latest patch I hope I did this correctly.

I was asked to look at this by James LeMosy.

I'm not sure this is needed, there's already an api in core for dynamic regions, LayoutDefinition::getRegions can be subclassed.

Here's a video of it in use.

https://drupal.slack.com/archives/C0T77ULN9/p1598562327038800

And here's a contrib module that does something similar: https://www.drupal.org/project/lb_tabs

So I think this can be closed works as designed.

Thanks, larowlan. So, if it is already part of core, what needs to be done to make it functional? It seems like progress on this has stalled, but it is still a frequently-requested feature. I've tried my hand at picking up where this issue left off, but I have had a hard time understanding how to use the API.

I guess the next step is to convey what part of it isn't functional. The links above show it is in use already, so it would be good to understand where the gaps are

Sorry, "make it functional" was not exactly what I meant. I am just unclear how to use the API to achieve the original goal of this issue, which is to be able to add multiple layouts within sections.

Ok, so I think perhaps we need more docs and examples

I'll try and find some time for that next week

Thanks, that would be much appreciated!

I wrote a blog-post about achieving dynamic tabs

That's the code that corresponds with this video

Hi @larowlan, the video you've linked to twice on here isn't viewable. It looks like you have to be part of the Drupal Association Slack? Can you repost the video so we can see how your dynamic tabs works?

Sure

Also, I recommend joining the slack, the community is a huge part of the power of Drupal

Thanks, that's helpful to see how it worked.

I couldn't get signing up for Drupal Slack to work before (though I'm a new Slack user). It required either a Google or Apple account or an @association.drupal.org email. The "Continue with Google" method wouldn't work. But I found the link under "Joining" here and was able to create an account on there. Thanks.

AFAICT @larowlan showed in the blog post he created 3 years ago at https://www.previousnext.com.au/blog/creating-layout-plugin-dynamic-regions that this is already supported?

@Luke_Nuke: can you confirm that your use case can indeed be achieved without modifying Drupal core?

Can we close this issue? 🤞

That would be great news and +1 for what @WimLeers sad: If the scope of this issue is already covered in core let's move on and close this. Would be great if @Nuke_Luke can confirm this. Apart from that I will go and test now to confirm. Other users who can confirm that is works already would be helpful too. Regarding our policies I set it to PMNMI for now(?)

Solutions for how to achieve this have been posted in this video and it has been over three months since the last update where this was marked as Postponed (maintainer needs more info).

Therefore I'm marking this one as closed. Please feel free to reopen or open a follow up ticket if you think there is still work to be done here.

Thanks!