When Placing a Block on 'Configure Block' page the originally selected region is lost

Started back at #31

Updated the comment slightly
Added the current route to the LocalActionDefault

More convinced we will need a trigger_error :(

The last submitted patch, 44: 2833116-44.patch, failed testing. View results →

Tested patch #50 following steps in issue summary and it resolves the issue.

Still think we will need a trigger_error but maybe the committer will surprise me.

Doing RTBC triage.

The first thing I notice is that the issue summary is incomplete and the steps to reproduce are out of date. And since this is making a change to the user interface there should be before and after screenshots. However, maybe manual testing here is fine.

I read the comments. I agree with #24 that this is a nice usability improvement. The changes asked for in #24 appear to have been done. In #42 there should be an explanation for why the current route is added to LocalActionDefault and that should be in the proposed resolution as well. And a reminder that anyone who works on a patch should ask for reviews and someone else to set the issue to RTBC.

I then looked at the patch.

1. +++ b/core/modules/block/src/Controller/BlockLibraryController.php
   @@ -104,6 +104,14 @@ public function listBlocks(Request $request, $theme) {
   +    // Add url route parameters to provide correct region setting
   +    // after creation.
   

   Needs to be wrapped to 80 columns.

2. +++ b/core/modules/block_content/src/Plugin/Menu/LocalAction/BlockContentAddLocalAction.php
   @@ -20,6 +20,12 @@ public function getOptions(RouteMatchInterface $route_match) {
   +    // If the current request has a region query parameter.
   

   This isn't a complete sentence.

3. +++ b/core/modules/block_content/tests/src/Functional/BlockContentListTest.php
   @@ -69,6 +69,22 @@ protected function setUp(): void {
   +   * Tests that region value is retained when we create new block.
   

   This can be simpler. "Tests the region value when a new block is saved."

4. +++ b/core/lib/Drupal/Core/Menu/LocalActionDefault.php
   @@ -37,11 +44,14 @@ class LocalActionDefault extends PluginBase implements LocalActionInterface, Con
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, RouteProviderInterface $route_provider, Request $request) {
   

   See policy for adding constructor arguments → . Although my later testing questions whether these changes are needed.

5. +++ b/core/modules/block_content/tests/src/Functional/BlockContentListTest.php
   @@ -69,6 +69,22 @@ protected function setUp(): void {
   +      'info[0][value]' => $this->randomString(),
   

   Since this is testing placement and not the value can we skip generating a random value and just use, say, 'foo'.

I then tested on a fresh install of Drupal 10.1x, standard install. First, I confirmed the problem using the steps in the IS. The I applied the patch and confirmed the fix. I then removed the changes to the menu system (core/modules/menu_ui/src/Plugin/Menu/LocalAction/MenuLinkAdd.php and core/modules/menu_ui/src/Plugin/Menu/LocalAction/MenuLinkAdd.php), cleared cached and retested. The fix was still working but the new test was failing. That needs to be looked into. I didn't look further myself.

So, back to NW for comment changes and then a question about the changes to the menu system.

Addressed #54 changes but not sure of the cause of the failed tests. For pointer 4, we can remove the unused class object but are unsure of its original purpose. Updated patch. Please review.

Issue summary needs to be updated

and #54.4 hasn't been done.

Hiding patches and converted to MR.

@quietone the request is used to get the query parameter which can't get from route_match. To get around that we would probably have to update the url to include region also. That seemed like the least desirable approach

@smustgrave, thanks. I don't know enough about this so I will leave this for others to RTBC.

The conversion to MR lost a change, I have commented in the MR.

@smustgrave
The issue has been resolved following the patch in MR #5562. When creating a custom block within the region, it displays correctly in the drop-down menu, automatically selecting the respective region
Thanks !

After Implementation

Nitpick: Is it normal for `url` to be lower-case in comments? It’s an acronym.

Neither my comment nor the screenshot review above should be credited.

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

Left a review

@larowlan Addressed all 3 issues.
Tests are green now.

Resolved all the threads fixed by @mohit_aghera.

Rebased the MR as there was a merge conflict.

Retested by going through block layout > place block into content region > custom block > verified region is saved.

Updating issue credits

Left some comments on the MR to simplify this

Addressed feedback.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

Fixed

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

Test only failed as expected. Looks good to go!

Thanks @acbramley!

Committed and pushed 1f94e17f9f to 11.x and a0138ae5ef to 10.3.x. Thanks!

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