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

I tested this on Drupal 10.0.x and the problem still exists.

I made a small investigation about that.
Seems like the region set in the core/modules/block/src/BlockForm.php and it's taken from the request.

    $region = $entity->isNew() ? $this->getRequest()->query->get('region', $entity_region) : $entity_region;
    $form['region'] = [
      '#type' => 'select',
      '#title' => $this->t('Region'),
      '#description' => $this->t('Select the region where this block should be displayed.'),
      '#default_value' => $region,
      '#required' => TRUE,
      '#options' => system_region_list($form_state->getValue('theme', $theme), REGIONS_VISIBLE),
      '#prefix' => '<div id="edit-block-region-wrapper">',
      '#suffix' => '</div>',
    ];

But, when we redirect to this form, regions don't pass through request.
So, first I add the route parameter for the action link ("Add custom block"). And later, when we create our custom block, add this parameter to the form state redirect method in core/modules/block_content/src/BlockContentForm.php

        $form_state->setRedirect(
          'block.admin_add',
          [
            'plugin_id' => 'block_content:' . $block->uuid(),
            'theme' => $theme,
            'region' => $this->getRequest()->query->get('region'),
          ]
        );

Reworked patch. Now all tests must pass.

I have applied the Patch #12 Successfully for drupal 9.3.x-dev. Before applying the patch the region we have selected in the place block function is not displayed in the Region select box. But after the patch the region will displayed as default.

Rinku Jacob 13, thank you for testing.

But after the patch the region will displayed as default.

Do you mean, that after applying the patch, a region that was selected before will be displayed in the settings form?

@ Kostia Bohach I have meant like that . But mistakely i wrote that. After applying the patch, a region that was selected before will be displayed in the settings form. In screenshots we can see the difference.Sorry for the mistake.

This seems like a perfect use case for a test.

Wow quick turn around on those tests! Good job!

Tested patch #21

Going to block layout
Place block
Create new custom block
Saving that block
Verified the region I originally clicked to place the block was remembered.

This seems like a nice improvement to usability.

1. +++ b/core/modules/block/src/Controller/BlockLibraryController.php
   @@ -104,6 +104,16 @@ public function listBlocks(Request $request, $theme) {
   +    $add_action_url = isset($build['local_actions']['block_content_add_action']['#link']['url'])
   +      ? $build['local_actions']['block_content_add_action']['#link']['url']
   +      : NULL;
   

   This can be written as:
   $add_action_url = $build['local_actions']['block_content_add_action']['#link']['url'] ?? NULL;

2. +++ b/core/modules/block/src/Controller/BlockLibraryController.php
   @@ -104,6 +104,16 @@ public function listBlocks(Request $request, $theme) {
   +    if ($add_action_url instanceof Url && $region) {
   

   I think we shouldn't do the && region here. I think adding the param if it is NULL should be okay - it'll just be ignored and then we don't have to think about regions called '0'.

3. +++ b/core/modules/block_content/src/BlockContentForm.php
   @@ -68,13 +68,14 @@ public function save(array $form, FormStateInterface $form_state) {
   -        $form_state->setRedirect(
   -          'block.admin_add',
   -          [
   -            'plugin_id' => 'block_content:' . $block->uuid(),
   -            'theme' => $theme,
   -          ]
   -        );
   +        $route_parameters = [
   +          'plugin_id' => 'block_content:' . $block->uuid(),
   +          'theme' => $theme,
   +        ];
   +        if ($region = $this->getRequest()->query->get('region')) {
   +          $route_parameters['region'] = $region;
   +        }
   +        $form_state->setRedirect('block.admin_add', $route_parameters);
   

   I think this could be:

           $form_state->setRedirect(
             'block.admin_add',
             [
               'plugin_id' => 'block_content:' . $block->uuid(),
               'theme' => $theme,
               'region' => $this->getRequest()->query->get('region')
             ]
           );
   

   For the same reason as above.

4. There's no test coverage of the change to \Drupal\block\Controller\BlockLibraryController::listBlocks() - if I remove the new code the test still passes.

@alexpott,
Made changes as per comment #24 for points 1,2,3.
Needs review.

Fixed custom commands failing issue.

Tested #26

Went to block layout
Clicked Add block into header region
Create custom block
Next page the header region was auto selected

Tested again with a region in the middle
Same correct behavior.

We've lost the test coverage that was present in #21 and the missing test coverage noted by #24.4 still does not exist.

Good catch. @Ratan Priya was there a reason for removing the tests?

Going back to #19 and starting from there to be safe.

Fyi this needs more work for the solution. If you go to /admin/structure/block/library/stark?region=content the region parameter isn't set.

Updated solution to cover additional scenario

Updated tests also.

Looks like test case failures are random. Test results seems to be green.

Patch #31 Applied successfully in Drupal 9.4.x and Drupal 9.5.x.

Steps:
1. Go to Structure->Block Layout
2. Click Place block any regions
3. Than Click Add custom block
4. Check the regions are not auto selected.
5. Apply the Patch
6. Regions are auto selected in the block configuration.

Applied patch #31 successfully in Drupal 9.5.x. and working fine.Adding screenshots to refer after and before patch.

Tested on Drupal 9.5

- Issue summary is clear and steps to reproduce have been provided
- The patch applies cleanly
- Was able to reproduce the issue following the steps in #34 and the patch fixes the issue
- A couple of suggestions with respect to the code

The comment wordings could have been better

+    // Adds to the "Add custom block" action url route parameters, to provide
+    // correct region setting after creation.

Can we make use of dependency injection here

+    if ($region = \Drupal::request()->query->get('region')) {
+      $options['query']['region'] = $region;
+    }

Fixed above CCF.

Started back at #31

Updated the comment slightly
Added the current route to the LocalActionDefault

More convinced we will need a trigger_error :(

I think a few permissions were missing in the test case.
Failing test case is passing on local now.

Oops, probably something like this also be a good change.
Updated the patch.
Cancelled the previous test case run and hiding the patch.

Looks like my local 10.1.x head was not updated.
Re-rolled the patch.

Fixing the label change happened in latest head.
Hiding failed patches.

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 !

Before Implementation

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!