Breadcrumbs block within Layout Builder causes errors when moving blocks

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.

Can confirm the issue in D10 following the steps from the issue summary.
Applying patch and clearing cache fixes the issue

Moving to NW for a change record though to announce the new service for layout builder.

We could also add a uuid regex to the routing file, which would prevent accessing the route with unexpected arguments.

The attached patch prevent the error from being thrown.

Fix for patch not applying

Thank you for your patch, alecsmrekar, it seems to work.

It seems to work, but I am however unclear on why this issue pops up at all. why is the `layout_builder.move_block_form`-route being triggered when the arguments clearly call for the `layout_builder.move_block` route.

I'm wondering if changing the path for one of the two routes would be a more stable solution?
Two routes with very similar paths and arguments, that seems like asking for trouble to me.

Perhaps all non-publicly accessible routes (like ajax routes) should get some prefix, for example.

Adding an entirely new BreadcrumbBuilder for this block feels excessive. I propose we simply check if the block is in preview and display the fallback text if it is. This was suggested in #19 and that issue has now landed so this is possible.

Added a MR with this approach with test coverage.

The Needs Review Queue Bot → tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Tests on MR is green now.

I like the solution in the MR that relies on the preview flag. I'm going to remove the "Needs change record" tag because this approach does not need a CR.

I ran the test only job and it failed, which is good.

I'm hardly impartial because I wrote the first fail test for this in #20, but I like my test better. To me the new test doesn't clearly demonstrate that the breadcrumb block is currently malfunctional; instead it looks like we're just focused on changing the preview text. I think asserting the class .block-system-breadcrumb-block is more robust and illustrative than asserting the preview text (though asserting the preview text in addition to the class would be good.)

I also personally prefer the approach of adding a few assertions to an existing test, which is almost certainly more performant than a dedicated functional test. There's something to be said for the separation of concerns, though, so I won't put up a fight there.

I think asserting the class .block-system-breadcrumb-block is more robus

Don't personally feel strongly about this, can add it in along with the preview test check.

I also personally prefer the approach of adding a few assertions to an existing test

I disagree with this one for a couple of reasons

This issue is in the breadcrumbs block, not Layout Builder, so the test should live in the system module, not Layout Builder
Layout Builder tests are already doing too much in single files and need to be split, so I don't want to add more bloat.

Moving the block is vital to triggering the error, so I think the test needs to move the block.

I'm 50/50 on this one. FunctionalJS tests are slow and I don't think it's needed in this case. I fully understand writing a test that runs the exact steps to trigger the issue, but in this case I think we can be confident that the changes to the build function of the breadcrumbs block also resolve the dragging issue, and they're being tested in this issue.

That being said, I don't feel strongly about this one and I can see the other side of the argument for sure. If someone would want to rewrite it as a FunctionalJS test I wouldn't block it

This being a bug, we're really supposed to add a test that triggers the error unless the issue satisfies a special set of criteria: https://www.drupal.org/about/core/policies/core-change-policies/core-gat... →

Looking at the functional test on the MR again, if all we're asserting is that the build function returns a certain string, then we could probably get away with a Unit test and make it really fast.

Rewrote the test to be FunctionalJS that follows the unhappy path

1) Drupal\Tests\system\FunctionalJavascript\Block\BreadcrumbLayoutBuilderPreviewTest::testBreadcrumbsInLayoutBuilder
RuntimeException: Unable to complete AJAX request.
/builds/issue/drupal-3160785/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php:117
/builds/issue/drupal-3160785/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php:38
/builds/issue/drupal-3160785/core/modules/system/tests/src/FunctionalJavascript/Block/BreadcrumbLayoutBuilderPreviewTest.php:83
ERRORS!
Tests: 1, Assertions: 7, Errors: 1.

Coverage appears to be there

I followed the simple approach for testing in the summary and can confirm the MR does fix the issue.

Believe this one is good.

I read through the issue summary and comments. There are no unanswered questions. I didn't review the MR or test the change.

I confirm that #29 fixes the issue.

To me the FunctionalJavaScript test is good but in the wrong place - it is primarily testing the functionality of Layout Builder even if the block belongs to system.module. If we ever move LB out of core then the test would have to go with it. Perhaps we should also add a small kernel test in system.module to prove that the block behaviour is correct outside of LB?