Remove the 'disabled' region from Block UI

I like the disable region, but I can see how this can make new user confuse.

the blocks stay in the region they are, but are visually styled to be disabled.

I think is the way to go, in order to fix this issue. However, what I did not see in this issue is a enable/disable button. We would have to provide an easy way (button) to enable the or disable from this page.

We need some guidance on what the visual representation of a disabled block would be (when left in its region).
The API for disabling a block is already in place.

Well here is some shoddy CSS just to demonstrate what needs to be done.
I've also appended (disabled) to the label just for now.

Here is a picture showing the working enable/disable links.

Here are some test fixes.

@tim.plunkett,

Great work, this patch makes the block page looks way better than the current block page. This patch looks good to me.

#2508547: Placing a block with same machine name of region causes region to disappear in admin UI fixes a bug with regions in the Block UI and refactors some code, so postponing on that.

Also while this patch is functionally sound, I just made up the actual UI changes and I'm sure @bojhan or @LewisNyman will want to have their say.

This is essentially blocked on #2514998: Reduce fragility in the monolithic BlockListBuilder, but if @LewisNyman comes back with a patch sooner, we can try to work around it.

This is amazing, the visual styling is a little bit off - but I like that we can just use text for the state.

@Lewis lets experiment a bit with the table style.

Reroll, still needs better CSS than #999/#eee :)

I agree the new concept makes a lot of sense and it looks sane. Given this is a normal task, committers will very likely request a beta evaluation (https://www.drupal.org/core/beta-changes).

As far as update hooks are concerned, into which region will we put all of the blocks that are in "Disabled"?

Wouldn't this make the UI hard to use because the disabled blocks would clutter up the list? Imagine that you have 100 disabled blocks. Now you want to drag one block to a different region. The disabled blocks may make it so you need to scroll the screen to drag the block.

Edit: We could always add a hide disabled blocks feature later.

In IRC we discussed preventing the user from updating if there are any blocks left in the disabled region.
However, once they update the code, it's too late, the blocks are gone from the UI.
Secondly, once they fix that, there is *nothing left to update*, so the update hook is pointless.

This patch tries a different, less ideal, approach.
It simply disables each block, and places them into the system_default_region()
However, it uses the Entity API directly, aren't we not supposed to do that? I know I saw another issue discussing this recently.

@tim.plunkett: Yeah update hooks are supposed to use as little of the API as possible, **especially** where hooks may be invoked, events may be fired, etc. Because those will run the new code assuming the new structures of things, while the data is not yet updated. While for this change itself that looks like does not make sense to consider, people will run several updates at once and may run multiple version updates in one batch (if not keeping updated with each version independently). So other updates may introduce changes to data structure, etc. that this code will be problematic to run with.

Here's an version using configFactory directly, but still using entity.query

If it passes WCAG AA, then this looks good to me :)

Looks great, thanks @LewisNyman!
Now we just need update tests and a beta eval.

rerolling a patch

The radical solution in #2514150: Advertise the block region demonstration in a more prominent way would effectively solve this issue.

Yes, except this is an incremental fix, that is much much more.
Also we'd need to solve the upgrade path anyway.
Let's keep working on this.

@tim.plunkett,

I agree we should continue working on this aswel. Just wanted to explain why I added the relationship.

Fixed the reroll.

Awesome, does this need anything else?

Works as expected ( Only issue i had was that it was not immediately obvious how to disable block -- i just clicked the configure button and found no way to disable block ) . Maybe the expand part of the configure button could be highlighted more.

Another option would be to add a checkbox to the vertical tabs at the bottom. Which would be in line with node's published checkbox and keep the form clean and simple.

No, this belongs on the listing page. People will get used to it. Menu links and nodes are not config, they don't treat "disabled" the same way, and are not analogous.

This is blocked on an upgrade path.

Search pages, filter formats, and views all have their disable functionality solely on the listing page. Blocks are to be the 4th.

I would say that menu links are wrong, and should be changed.

And nodes cannot be disabled, only unpublished, completely unrelated.

Still needs tests.

Still needs tests.

This is looking great! Zero nits :)

We need test coverage for double escaping, it's currently broken and possibly fixed by #2568045: Make it impossible to double escape with #plain_text

Incremented the update hook. Created the updateTest (nothing in it); Ran out of time.

fix one of the failing tests by rerolling the tests for some changes in head, possibly from the disabled block issue.

This looks more rc deadline/minor version target to me.

Assigning to @webchick as this is Drupal product facing to see if it can be "rc eligible" or target.

Fix failures.

Found a small bug. Not sure if we need to fix it. Needs work anyway for the upgrade test.

A block that is enabled before running the upgrade can become disabled.
condition: A block is placed more than once. It is enabled in a region and the second is disabled.

While I definitely still think we should do this, I can't see how we could possibly do this now, during RC vs. in a minor release. It breaks UI, strings, documentation, adds an update hook, all kinds of things.

Moving to 8.1.x unless I'm missing something.

Added some progress on an upgrade path test.

Minimal update path tests.

Tests are not running for this issue.

The issue is marked for 8.1.x, which has 2+ year old code, so tests don't run.
If you'd like, feel free to upload patches to an issue like #2374115: IGNORE: Patch testing issue just to get test results.

Will testbot pickup the patches...?

Hello everyone,

I found 'disabled' section in "Manage form display" and "Manage display". For some content types we may have to create more fields. Can we build something this new feature for those area?

@imrancluster that is unrelated to the Block UI

Re-rolled against 8.2.x to start work on remaining tasks

I can still found the regions that had not block assigned. Please check the screenshot.

correct me if i am wrong.

@Manjit.Singh I don't understand, that is 100% supposed to happen.
We're only removing the "Disabled" section of this page.

@Manjit.Singh I am new to this issue but from reading of the issue summary and looking at the code this issue is not to remove regions from the Block Layout page that currently don't have any blocks assigned to them but rather to remove Disabled region from the block layouts page.

Here is an unpatched 8.1.x sight layout page

Regions with no blocks should still show up. That way you still have the "Place Block" button.

Removing non-relevant task and tags.

There already is an upgrade path block_update_8004 and tests for the upgrade path, BlockRemoveDisabledRegionUpdateTest

I am looking over the code but looks like there no remaining tasks and this code be RTBC.

This looks good. Changing to RTBC!

I really like this change!

+++ b/core/modules/block/src/BlockInterface.php
@@ -15,11 +15,6 @@
-   * Denotes that a block is not enabled in any region and should not be shown.
-   */
-  const BLOCK_REGION_NONE = -1;

Okay one more thing to discuss.
Are we allowed to remove this constant in 8.y.x?

Also this obsoletes #2724283: BlockInterface::BLOCK_REGION_NONE is integer, Block::$region is string

#82: I wonder if we should keep that and then move this:

+++ b/core/modules/block/src/BlockListBuilder.php
@@ -366,12 +367,6 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
-      if ($entity->getRegion() == BlockInterface::BLOCK_REGION_NONE) {
-        $entity->disable();
-      }
-      else {
-        $entity->enable();
-      }

into Block::save(), which would then effectively retain BC?

Restoring the constant, deprecating it, and merging in the test coverage from #2724283: BlockInterface::BLOCK_REGION_NONE is integer, Block::$region is string.

I think #83 makes sense because there could be custom that is trying to disable blocks currently by moving them to the disabled region.

#83 I disagree, I don't think we should change Block::save that way.

We can just rely on the existing functionality of block_rebuild.

+++ b/core/modules/block/block.install
@@ -124,5 +124,29 @@ function block_update_8003() {
 /**
+ * Disables blocks that are placed into the "disabled" region.
+ */

Seems like it should be "Disable blocks" rather than "Disables blocks".

Leaving at needs review for a substantive review.

While in review, any thought regarding comment #16: The disabled blocks could clutter the UI? I don't want to delay progress, but it's something to consider.

Honestly, I consider disabling a block to extremely temporary. Creating new blocks is simple, so I would generally delete any block I left disabled for a significant period of time.

Just review the #86 patch and it's working fine. 'Disabled' regions is no more available. Now enable, disable link is coming with every configure dropdown. Screenshot is attached for your reference.

Thanks for testing the patch @Maninders, but @tim.plunkett in #90 set it to "needs review" for a thorough review of the actual code changes. Setting back to needs review for that.

Reviewing from #86

Seems like it should be "Disable blocks" rather than "Disables blocks".

@Manjit.Singh nice catch! I thought you were wrong but checked: https://www.drupal.org/coding-standards/docs#updateN
And says

Note that the first line should start with an imperative verb, so it will make sense to people running update.php.

+++ b/core/modules/block/src/Entity/Block.php
@@ -28,11 +28,14 @@
+ *     "disable" = "/admin/structure/block/manage/{block}/disable",

No trailing comma for annotations

+++ b/core/modules/block/block.module
@@ -144,17 +144,12 @@ function block_rebuild() {
+          drupal_set_message(t('The block %info was assigned to the invalid region %region and has been disabled.', ['%info' => $block_id, '%region' => $block->getRegion()]), 'warning');

In my patch I am changing this to not display this message if the BlockInterface::BLOCK_REGION_NONE was used to disable the block. It seems if this is just deprecated we should not show an error message.

Looks Great!

Here is patch with those small changes. It these are ok I think RTBC

Annotations can have trailing commas as of late 2013: https://github.com/doctrine/annotations/pull/11

That change related to the message needs test coverage. I'll fix that shortly.

I did more thinking on the change to block_rebuild in #94. I don't think we should add new usages of the constant. -1 is now an invalid region, I don't think it's wrong to display that. The functionality is the same as in HEAD regardless of the message.
Also restoring the trailing comma.

@tim.plunkett ok I see the point about the message. Looks good

I also have updated the annotations doc page to reflect the changes to trailing spaces https://www.drupal.org/node/1882526

RTBC!

+++ b/core/modules/block/block.install
@@ -124,5 +124,29 @@ function block_update_8003() {
+  $block_ids = \Drupal::entityManager()
+    ->getStorage('block')
+    ->getQuery()
+    ->condition('region', -1)
+    ->execute();

I don't think we should use the entity API in hook_update_N(). I think this fix could be done in a post update and using the config entity API.

Also should we detect and handle blocks saved with REGION_NONE in the config entity - since nothing is supposed to have REGION_NONE anymore but once the update is run it is perfectly possible to save such a block through the API and then on a hook_rebuild it'll be moved to the default region.

+++ b/core/modules/block/block.module
@@ -144,17 +144,12 @@ function block_rebuild() {
+          $block
+            ->setRegion(system_default_region($theme))
+            ->disable()
+            ->save();

On hook_rebuild it will be moved to the default region but also disabled.

I'll look into the post_update change.

Also \Drupal::entityManager() is *deprecated*

Working on this.

@alexpott pointed out that due to block_rebuild(), we don't actually need an update path, just to trigger a cache flush by putting in an empty post_update hook. Additionally, block_rebuild was not handling disabled blocks in the wrong regions. Fixed that and the test coverage. Finally, added a Block::preSave() to address concerns about contrib/custom modules expecting $block->setRegion(-1)->save() to work even without a cache rebuild.

Super excited if we can get this into 8.2 :)

So we have a major problem with that addition of Block::preSave().
Migrate has a simple mapping of region names, but those differ for each theme. For example, Bartik no longer has a region named "footer".
Migrate has no ability to provide different mappings per theme.
Furthermore, it tries to migrate things for themes like Bluemarine, which no longer exists. system_region_list() will always fail for that in tests unless we ship with a fake theme named bluemarine, so not sure what to do there.

So either we're blocked on Migrate being fixed, or we go without the Block::preSave() fix.

Option 1 is blocked on #2753939: BlockRegion process plugin should be source theme-aware and also another issue that @phenaproxima hasn't opened yet either.
Option 2 is just reverting the one Block::preSave() hunk introduced in #102.

I think that block migrations are fundamentally broken see #2753939-2: BlockRegion process plugin should be source theme-aware

We could just add @todo's to fix the migration tests because migrations don't make make sense when block_rebuild() is going to come along and change what you've just migrated.

That's a reasonable workaround for me.

+++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
@@ -123,7 +124,9 @@ public function testBlockMigration() {
+    // @todo https://www.drupal.org/node/2753939 This block is the footer region
+    // but Bartik in D8 does not have this region.

@@ -138,7 +141,9 @@ public function testBlockMigration() {
+    // @todo https://www.drupal.org/node/2753939 The bluemarine theme does not
+    // exist.

+++ b/core/modules/block/tests/src/Kernel/Migrate/d7/MigrateBlockTest.php
@@ -109,7 +110,9 @@ public function testBlockMigration() {
+    // @todo https://www.drupal.org/node/2753939 This block is the footer region
+    // but Bartik in D8 does not have this region.

The second line should be indented.
That can be fixed on commit.

We need a CR because as the recent patches show some tests for contrib might break.

For #110

Posted CR and restored RTBC status: https://www.drupal.org/node/2778489

Fixed those nits just to make it easier to get this in.

Updating test coverage added recently.

Committed/pushed to 8.3.x, thanks!

I'm not seeing anything that makes this time-sensitive for the 8.2.x beta (i.e. the issue summary does not mention anything), so leaving it fixed against 8.3.x for now.

Published change record at https://www.drupal.org/node/2778489

http://i.imgur.com/n34QWnF.gifv

This is a longstanding UX issue, maybe worth a release notes mention?

Disabled blocks go to the end of the list; They don't stay in order by weight. However, when you enable the block it goes back in order using the previous weight. Is that how it should be?

It is by design, \Drupal\block\Entity\Block::sort() does three things:

    // Separate enabled from disabled.
    // Sort by weight.
    // Sort by label.

Changing that would need a new issue.

Some feedback on the new design:

+ I like the "place block" button right where I need it.
+ The search function is great, but also quite necessary:
- The list of available block can get a little long.
+ I like how you can see what type of block it is. I currently have two "contact us" blocks, and this helps differentiate them.

Something I have more trouble with:
- I've accidentally deleted a custom block (and the view that generated it) because I didn't realize that "remove" now means "remove the entire block form existence" instead of "remove it form this spot in the layout".
- I have no idea how I actually remove disabled items from the block layout page. It's getting a bit cluttered.

I would propose a simplification:
> Remove the "disable" option from the interface. After all, if I want to re-add a block, I now have the nice 'place block' buttons. But maybe I'm missing a use-case?
> The "Remove" button should then just remove the block for the layout page.

I'm really lost about this change unless I'm not seeing the correct behaviour.

After upgrading 8.2 > 8.3 I have 60 disabled blocks in the first page region ('Nav Header') rather than is the 'Disabled' region at the bottom of the page.

Is what should be happening?

No, they should not all be in the 'Nav Header'. That sounds like a bug.

Remove the "disable" option from the interface. After all, if I want to re-add a block, I now have the nice 'place block' buttons. But maybe I'm missing a use-case?

You might have blocks that are disabled. You want to be able to quickly enable them without having to re-add them.

@lokapujya I understand the functionality. But how often does this happen in the real world? When do you temporarily need to disable a block?

For me personally this never happens. And if it would, I wouldn't have a problem with removing the block and re-adding it, especially if it would fix the issues with the interface I mentioned above.

Suppose you have a block that is experimental, or you are doing AB testing. You might have multiple alternative blocks that you want to demo. The block has a lot of configuration. You don't want to add the block and retype everything and possibly misconfigure something. Then you may need to disable and then enable the block.

Changing such a key part of Drupal admin UI for an issue only followed by 39 people seems an odd decision making process.