Work with layout builder

I'm getting an error with Drupal 10:

2023/04/04 00:30:23 [error] 5890#5890: *2535 FastCGI sent in stderr: "PHP message: Uncaught PHP Exception TypeError: "Drupal\collapsiblock\EventSubscriber\LayoutBuilderBlockComponentRender::attachCollapsiblock(): Argument #1 ($event) must be of type Symfony\Component\EventDispatcher\Event, Drupal\layout_builder\Event\SectionComponentBuildRenderArrayEvent given" at /home/vagrant/WG/wg/web/modules/contrib/collapsiblock/src/EventSubscriber/LayoutBuilderBlockComponentRender.php line 48"

I found a related ticket: https://www.drupal.org/project/rules/issues/3259445 → The patch there suggests swapping to Contracts\EventDispatcher and using object instead of Event for now.
https://www.drupal.org/files/issues/2022-01-23/3259445-12-events.patch →

This needs to target the 4.0.x branch now, 3.0.x is discontinued and will go out of support when Drupal 9 does.

I've updated this patch for Drupal 10. Here the patch and the interdiff.

Sorry for my mistake: The previous patch is not correct, here the complete patch and the interdiff for 4.1 brach running with Drupal 10.1.x

Upload rebased patch which supports latest dev fixing the following crash:

The website encountered an unexpected error. Please try again later.

Error: Undefined constant "COLLAPSIBLOCK_ACTION_OPTIONS" in collapsiblock_form_alter() (line 106 of modules/contrib/collapsiblock/collapsiblock.module).
Drupal\Core\Extension\ModuleHandler->alter('form', Array, Object, 'layout_builder_update_block') (Line: 840)
Drupal\Core\Form\FormBuilder->prepareForm('layout_builder_update_block', Array, Object) (Line: 284)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 73)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)

Also updated the logic so that the third party settings are only generated if the block has a specific override stating that it's no longer using the default settings. This ensures that blocks won't have their configs updated unnecessarily.

Uploading a patch based on the MR 19

The last submitted patch, 31: collapsiblock-3166181-mr-19.patch, failed testing. View results →

I found the reason why the testLayoutBuilderBlockWillCollapse() test fails.
The test tries to display a node field with a block, but the ig does not provide that the block should display node fields, but only blocks, respectively, in the UI form appear, but in fact it does not affect the display of the field.

I plan to do the following:
- delete the test testLayoutBuilderBlockWillCollapse() (leave only the test for the form)
- create a new file called LayoutBuilderUITest and describe the test there
- create a content type in it
- switch to the default display
- add a block and save the layout
- go to the page and check the current state
- JS click on the tag to change the display
- check if it has changed

I've added the test

Thanks @bobi-mel.

Your test seems to be a good one for when editors have control over layout builder, but we need the other test for when layout builder is only used to set the default layout. Please re-instate and fix the test you removed to progress this issue.

There are some variable naming issues in the new test that I am happy to tidy up once the functionality is sorted out.

Just a bit of admin, setting the correct target version, and let's stick with the MR now that we've got pipelines running.

This is looking great. I'm testing with LB and it provides the collapse functionality as expected. However, there is only 1 cookie being set, which either has the value 1:1 or 1:0 depending on the last action on a block. So, when I collapse 1 block, it set the cookie to 1:0 and after a reload of the page, all blocks are collapsed. Shouldn't that work on a pre-block-basis?

Yes it should be keying the cookie value by block ID. I'm guessing the IDs work differently in LB, possibly no id attribute at all on blocks.

Hello @darvanen

I have returned the test I deleted and made some changes to it. I changed the previous method of adding a field as a block because it did not add the wrapper for the block provided by the module.
Please check my changes.

Thanks for that @bobi-mel!

Can we get a fresh fork/MR on this issue please? There are a number of unrelated changes on the current MR, conflicts with HEAD, and the pipeline isn't running.

Hi @darvanen

I've rebased the 4.x branch into this branch.
Please check it

This is looking fantastic! Thank you

Has #37 been addressed?

There's also one small nitpick in gitlab comments.

I've tested the latest version of this MR with the 4.x-dev release and can confirm, that #37 is partly resolved, but not completely.

Some blocks are still identified with 1, but others (e.g. masquerade, menu or group operations) are identified with their string ID.

Two blocks that I've found with the identifier 1 are the block "Who's new" and all blocks from views.

Would be great if those could also be identified with some unique ID.

Wonderful, that's a big improvement, thanks @bobi-mel.

I think we still need to address #43 as we're adding an id to each block. Perhaps we could simply generate a uuid for each block?

HI @darvanen

I studied this issue and was able to reproduce it by adding two identical blocks to the LayoutBuilder Layout. The EventSubscriber that was added to implement the module logic functionality for LayoutBuilder does not guarantee the uniqueness of the ID for the blocks. I propose to add to the logic of our EventSubscriber to the ID of the block its weight without a lower dash as a separator to guarantee the uniqueness of the key for the cookie.
@see https://git.drupalcode.org/project/collapsiblock/-/merge_requests/19/dif...

change the code to the following $id = Html::getId('collapsiblock-wrapper-' . $build['#configuration']['id'] . $build["#weight"]);

The cookie will look like this: {%22who7%22:1%2C%22who8%22:0}

Let me know what you think of my idea and we can add it

#46: I like this idea a lot better than my idea of generating a UUID.

Thanks @bobi-mel, did the change work as intended? If so perhaps you could set it to NR and we'll see if @jurgenhaas or someone else has the bandwidth to try it out?

If it is satisfactory the last thing required will be a test to lock in that behaviour.

Hmm, I've tried that again, but with no luck. I can see the extra ID component in the div container, but my cookie still only uses the final number for each block, e.g.

10:0
11:0
12:0
13:0

I guess the ID is taken from the button element, and not the div container? The button still only has the weight ID.

Also, I can see 2 potential extra issues with this approach:

• The same block could appear in 2 sections of LB and have the same weight value in each of those sections.
• What happens to those block on different pages?

Hi @darvanen @jurgenhaas
I tried to reproduce the potential problems described in #49.

I got the following results:

1) How to add the same block to different sections, they will have different weights and each will keep their behavior
2) I added a block that will be added to each node when it is created. When changing it from collapsed to expanded, its state is saved for each node.
3) I tried to create a page structure by manually adding the same blocks in the same sequence to two different nodes, for example: a Related Content block below it is a Who's Online block. Their state is saved and displayed on each of them in the same way.
If the structure is different, the behavior is also different.
4) @jurgenhaas try deleting cookies in your browser manually and check again, I have a hunch that this is the problem.

I think that adding weight solves the problem with the uniqueness of the cookie keys.

@bobi-mel by any chance did you try adding the same block to two different sections on the same node?

@bobi-mel I did delete all cookies and still get to the described behaviour. Wondering how you added the block to the node create page, are you using Layout Builder? I'm asking, because the described problem is only about LB, not about the classic block layout framework of Drupal.

@darvanen

I tried to add the same block to different sections of the same node and their behavior does not affect each other, it works as expected

@jurgenhaas

I added blocks in two ways:
1) The first is to have the block added to each new node that is created. I added it as follows:
- Followed this link - /admin/structure/types/manage/article/display/default/layout
- Clicked on the Add block button and added the selected block

2) The second way is to edit the Layout for a specific node:
- I followed the link - /node/5/layout
- Click on the Add block button and add the block

Regarding the value 0 or 1 after ":" in the cookie.
The value "0" will be set for the key of the block whose state has changed and is now set to be minimized.
The value "1" has analogous logic only for the block whose current state is "expanded" after changes.
Accordingly, if you changed the display of blocks and set the current state for all "minimized", then all keys in the cookies will be with the value "0"

@darvanen

After adding weight to the block ID, the testLayoutBuilderBlockWillCollapse test was broken. I tried to quickly retrieve the block from the entity node, but I haven't found a good solution yet.
When running tests, the weight of the test block is always "2".
Please clarify if it will be good if we define the Xpath for Title and Content in the test as follows.

    $collapsiblockTestBlockTitleXpath = $this->assertSession()->buildXPathQuery('//*[@id=:blockId]//h2', [
      ':blockId' => '#collapse-blocknode' . $nid . 'body . 2',
    ]);
    $collapsiblockTestBlockContentXpath = $this->assertSession()->buildXPathQuery('//*[@id=:blockId]//div[2]/p', [
      ':blockId' => 'collapse-blocknode' . $nid . 'body' . '2-content',
    ]);

Perhaps you have better ideas than mine, I will be grateful for your advice

I've now built a 3-column layout for the basic page node type and added 3 blocks. The page then looks like this, and the markup underneath. The cookie only contains 10:0 or 10:1, depending upon the latest change, on any of those buttons. Note, this is on Drupal 10.3.0-beta1:

@bobi-mel let's wait to fix the test until we've resolved the issue properly.

I suspect that weight alone won't be enough, we might need the delta or ID for section and column as well.

Hi @darvanen @jurgenhaas

I discussed with my colleagues the problem of uniqueness for each block cookie and we came to the conclusion that the only reliable way to guarantee the uniqueness of cookies in Layout Builder is to add a UUID to the block ID.
I added these changes and also added changes for tests to search for a block by class.
Check it out.

Left one small comment/query :)

Commented on your question

I've tested the latest version of the MR, and it works like a charm. Great work, everybody. It even works in the context of the dashboards → module, which is built on top of LB.

That's good enough for me, we probably could have done better test coverage for those last problems, but I'm happy with where it is for now. If they break in a later release I will insist on tests.

Going to make this a minor RC.

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