Updated: Comment #37

Note to committers: Please give @tim.plunkett commit credit as I merged in his code from #2077911: Feed block post deletion is broken and untested

Problem/Motivation

Some block types could possibly expose a large number of block derivatives. As the Block UI now lists all blocks, this would result in UI problems. It can also result in better UX in general.

The other blocks that use derivatives are:

- Aggregator Feed
- Aggregator Category
- Language switcher (content, user interface, ...)
- Menus
- Custom blocks
- Views

Proposed resolution

Remove derivatives where it is useful. That means aggregator feed and category blocks right now.

Remaining tasks

Decide if other blocks should be converted too. The only questionable one IMHO is custom_blocks, not sure what the goal is there with the UX/UI.

Possibly alter admin labels of the changed blocks.

User interface changes

Instead of Something feed, Whatever feed,

API changes

See screenshots in #2055853-5: [meta] Improve the place block UX; Separate interaction from the create block UX; Improve the existing blocks-by-theme layout. Instead of a list of blocks to add, there's a only one and you configure it then on the add block form.

This issue now fixes (or makes unnecessary) 3 other major issues:
* #2077911: Feed block post deletion is broken and untested - Merged, bug fixed, with tests
* #2057355: Aggregator Category blocks do not render - Noticed bug too, different implementation resolves that, no tests at all for category blocks. Maybe change the other issue to a add tests task?
* #398556: Change Aggregator block options from number to boolean - Removes the block configuration that the other issue wants to change to a boolean completely. We *could* also do that and have them not show up in the select, not sure if that's necessary.

Original report by @catch

The blocks as plugins patch set up most core blocks which can have multiple versions (menus, aggregator feeds etc.) to be derivatives. This means that a block exists in the library UI for each item.

An alternative would be to provide a single block (i.e. 'feed'), then when this is selected, as part of the configuration get the user to select which feed will show in the block.

This means less blocks shown initially, but an extra required step after selecting a block, if it's a multiple/reference block.

IMO a block for each items was only done in < 8.x because we didn't have block instances. Now block instances are there, there doesn't seem much reason to do that any more.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

I agree, this seems like it would be much better UX than the current setup.

xjm’s picture

Issue tags: +Blocks-Layouts, +Plugin system
Berdir’s picture

Issue tags: -Blocks-Layouts, -Plugin system

Will do an example implementation of this for aggregator feeds once #293318: Convert Aggregator feeds into entities is in. (Hint: Review and RTBC! ;))

Berdir’s picture

Issue tags: +Blocks-Layouts, +Plugin system
xjm’s picture

We could also just have a select box on the block instance configuration form for which thing (menu, aggregator feed, whatever) it should be.

Bojhan’s picture

I think this is basically what I am suggesting in #1871746: Add modal block browser, so totally love this idea :)

Berdir’s picture

Status: Active » Needs review
FileSize
9.22 KB

Proof of concept using the aggregator feed block.

- Removed the derivate
- Remove duplicated block setting from feeds
- Cleaned up the build() part to use the entity render system. Looks like we forgot to update this while doing the conversion.

Status: Needs review » Needs work

The last submitted patch, aggregator-feed-instance-setting-1888702-7.patch, failed testing.

Berdir’s picture

Do we have an agreement on this approach? Then I'll convert categories and menus too.

I think the language blocks can stay, not sure about custom blocks yet. It would certainly limit possibly very long list of custom blocks something that can be a problem in 7.x.

EclipseGc’s picture

I am generally very against this proposal. I think it's well understood at this point that we need a better block placement mechanism. I'd prefer to build that with this in mind, than to rework the core blocks to better fit what is currently in core. Derivatives are a really important usability issue as they can encompass a lot of configuration w/o direct user input, and with proper display of what's available I think that could go a long long way towards making the system easier to use.

Eclipse

Bojhan’s picture

@EclipseGc The purpose of this phase, is to clean things up - frankly building completely new systems to handle the fact that a system could end up with dozens maybe even hundreds of blocks. Seems unfeasible, so if you have a suggestion how the UI would work with what you think should be in core - please share, because I think blocks and layouts has been a UI fail, whatever outcome of any of these issues is significantly less than we set out to do - its a reality we must face, and it will negatively impact our users.

EclipseGc’s picture

And it will negatively impact users even more if more content is converted to blocks (which this is a conversion phase for these sorts of things too right now, so...)

I understand what you're saying here, but adding categories to blocks, and contexts to blocks all seem like the sorts of things that can help to inform a user interface further and give us something more usable. Likewise, pre-configuring what we can about blocks is a UI win, and will be more so with more complicated blocks (for example if fields become blocks, which I sincerely hope they do).

Eclipse

Berdir’s picture

We don't have to replace everything. It might e.g. still make sense for menus as we do not expect hundreds of them. On the other side, the widely popular menu_block module in 7.x seems to prove that people do understand the concept of creating a "menu block" and then chose which menu (parts) they actually want to display for that specific block instance.

drupal.org e.g. had the problem that there were so many feed blocks that it crashed the blocks admin page and they had to set block to 0 on all of them. So to avoid that, we need to keep the block attribute of the feed table, which is not actually used anymore for anything else than preventing a feed from showing up in that list.

Right now in 7.x,when you have a lot of custom blocks/beans it can become very hard to keep an overview of them, so the usability argument IMHO works both ways round. Ways of categorizing/grouping would also help, but that's a concept that we don't have yet.

EclipseGc’s picture

Well, feeds are an entity, chances are the block should be moved to requiring an entity of that type, and then you could inject one contextually when you need it (which you specify why feed to load elsewhere) so there'd only be one feedblock instead of 100s. This conversation works both ways in that regard. There are some things we blow up and expand (derivatives are great for that) and there are others we collapse. The one I'm actually concerned about is custom blocks, and that probably deserves even more discussion since we're currently treating it like derivatives but it could totally be argued that we'll end up with 100s of those on some sites.

I'm just saying, before we run down this rabbit trail, let's at least explore the option of an improved UI for blocks. There are a bunch of edge cases here, but I think there's a pretty good "this and no more" argument to be made about certain UI aspects, and I think other UI aspects could probably expand a bit w/o significant raising of eyebrows.

Eclipse

xjm’s picture

Issue tags: +Usability

Hmm, I actually disagree with @EclipseGc that derivatives as a concept have anything to do with UX... however, I think the specific change is a UX improvement, not a regression.

@Bojhan, any chance that you could provide a quick mockup of what the interface is like after the change? I'd like to be able to show it to people for the usability question.

EclipseGc’s picture

I'm going to reiterate that mockups of the current system aren't useful in my opinion since we should be striving to replace the existing UI with something that represents the available blocks better. This is high on my own priority list once #1927608: Remove the tight coupling between Block Plugins and Block Entities is done.

Eclipse

xjm’s picture

I just looked at #1871746: Add modal block browser and there are several mockups there, but the part that is missing from the mockups is (e.g.) which menu the menu block is for, etc.

xjm’s picture

@EclipseGc, this issue is relevant in either the current UI or the "fixed" UI.

EclipseGc’s picture

I agree the issue is relevant, because it informs what happens in the new UI. I would very very much like to put categories on block plugin definitions so that we can have a UI that sorts them by category, and hopefully also context (when we get blocks utilizing context). So removing derivatives now, before a UI that leverages them has even been attempted seems premature to me. That's all I'm trying to say here.

Eclipse

ParisLiakos’s picture

I very like this, instead of what we have now. That would make block admin far more usable
Also i want to x-post: #398556: Change Aggregator block options from number to boolean

Berdir’s picture

Status: Needs work » Needs review
FileSize
16.2 KB

- Rerolled to make it possible to manually test this and fix some of the failures. The multilingual block tests will need to switch to a different type of block.
- Removed a bunch of unecessary code.
- I'm not sure if block categories would really help here. All aggregator feed blocks would show up in that category right, so you still have 100 or even more in a list?
- This would also make it relatively easy to support multiple feeds
- Not sure why $block->settings is protected, how do I now check the feed(s) or change the count of a block? (see @todo's)

Status: Needs review » Needs work

The last submitted patch, aggregator-feed-instance-setting-1888702-21.patch, failed testing.

Bojhan’s picture

@xjm Could you elaborate a bit more on whats missing?

catch’s picture

- I'm not sure if block categories would really help here. All aggregator feed blocks would show up in that category right, so you still have 100 or even more in a list?

This.

EclipseGc’s picture

I'm fine with a case by case exception to the approach. We certainly wouldn't want a block for every node in the system. That being said, Aggregator sounds more like it's a block that should be consuming context than either a configurable or derivative based block. We already have the technical facility to handle that approach, we are still missing the UI, and this is sort of what I'm getting at. I'm not trying to say that the existing approach is perfect for all the existing blocks. I am, in fact, quite sure that there are flaws, and I think Aggregator is probably a great example. But aggregator also bears more resemblance to a node content/field/property block than it does to a derivative/configurable system, and this is exactly why I want to explore other UI options before we make drastic changes.

Also, you should be able to use $plugin->setConfig($property, $value); to set any block configuration, and getConfig() to get the array of the current configuration.

Eclipse

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Usability, -Blocks-Layouts, -Plugin system, -Block plugins

Status: Needs review » Needs work
Issue tags: +Usability, +Blocks-Layouts, +Plugin system, +Block plugins

The last submitted patch, aggregator-feed-instance-setting-1888702-21.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
16.97 KB

Re-roll. Need to check how the new block UI affects this.

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, aggregator-feed-instance-setting-1888702-29.patch, failed testing.

Berdir’s picture

This should work better :)

Status: Needs review » Needs work

The last submitted patch, aggregator-feed-instance-setting-1888702-31.patch, failed testing.

Berdir’s picture

Switched that test to use menus instead. Why did I chose aggregator in the first place, way more complicated :p

Also switched the category block. Pretty sure that has no test coverage, the build() method should also split the id, just like the feed build() method?

We might want to update the block labels...

ParisLiakos’s picture

this looks great..and i think the diffstat is going to be great too:)

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorCategoryBlock.phpundefined
@@ -65,8 +78,8 @@ public function blockSubmit($form, &$form_state) {
+    if ($category = db_query('SELECT cid, title, block FROM {aggregator_category} WHERE cid = :cid', array(':cid' => $cid))->fetchObject()) {

just in case this is rerolled: since we are touching this, could we make it aggregator_category_load?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorFeedBlock.phpundefined
@@ -45,6 +45,17 @@ public function access() {
+    $feeds = entity_load_multiple('aggregator_feed');

inject this?

tim.plunkett’s picture

Priority: Normal » Major

This is now a major bug since #2058321: Move the 'place block' UI into the block listing went in. Unless the categories are collapsed by default, or the UI is redesigned in #2055853: [meta] Improve the place block UX; Separate interaction from the create block UX; Improve the existing blocks-by-theme layout altogether, this would potentially render the blocks UI unusable.

Also, it should happen anyway :)

webchick’s picture

I agree with that. The UX win here is huge (probably the DX win, too).

I'm not sure if there are API changes required to get this done (tagging as needs issue summary update), but if there are, consider them approved.

Berdir’s picture

Title: Consider configuration selection instead of derivatives for most blocks » Use configuration selection instead of derivatives for some blocks
Category: task » bug
FileSize
7.01 KB
29.77 KB

Re-roll, inject storage controller and database. So much glue code ;)

This now changes the aggregator feed and category blocks. The other blocks that use derivates are:

- Language switcher (content, user interface, ...)
- Menus
- Custom blocks
- Views

Views and language switcher derivates stay, I guess menus for now should too. Not sure what to do with custom blocks, Are there any special plans what to do with the UI of them? What we have right now is still a bit weird I think. I think using the category to group them by custom block type would make sense if we keep the derivative. And if not, we need to come up with a nice admin label.. "+ Existing custom block" maybe? How to make the UI flow work for adding a new one, where you don't want to select?

Berdir’s picture

Issue summary: View changes

Updated issue summary.

klonos’s picture

Issue summary: View changes

...made update comment number a link and fixed minor typo.

larowlan’s picture

With custom blocks not fussy either way, guess it comes down to the UX team here provided that we retain the decoupling between content and config, i.e. we store the uuid and not the serial id (as we do now). We already enforce unique values for the 'info' field so selection makes sense (the names will be unique).

aspilicious’s picture

Berdir’s picture

Re-roll only for now, thought that the delete stuff was broken already, have to adapt it anyway here.

Status: Needs review » Needs work

The last submitted patch, aggregator-feed-instance-setting-1888702-40.patch, failed testing.

tim.plunkett’s picture

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/AggregatorRenderingTest.php
@@ -70,11 +72,13 @@ public function testBlockLinks() {
+    // @todo: What to do with this? Remove? $block->settings is protected and
+    //   can not be changed?
+    /*$block->settings['block_count'] = 0;

From Block:

public function preSave(EntityStorageControllerInterface $storage_controller) {
    $this->set('settings', $this->getPlugin()->getConfiguration());
  }

So you have $block->getPlugin()->setConfiguration($array) available to you.

Also note #2077911: Feed block post deletion is broken and untested.

Berdir’s picture

Ok, merged that other issue into this, improved the tests a bit with a second feed and making sure that the right block got actually deleted.

Fixed the annotations, those , get me all the time on merge conflicts.

This means that this issue now fixes (or makes unnecessary) 3 other major issues:
* #2077911: Feed block post deletion is broken and untested - Merged, bug fixed, with tests
* #2057355: Aggregator Category blocks do not render - Noticed bug too, different implementation resolves that, no tests at all for category blocks. Maybe change the other issue to a add tests task?
* #398556: Change Aggregator block options from number to boolean - Removes the block configuration that the other issue wants to change to a boolean completely. We *could* also do that and have them not show up in the select, not sure if that's necessary.

Status: Needs review » Needs work

The last submitted patch, aggregator-feed-instance-setting-1888702-43.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
37.58 KB

Re-roll. Why is everyone trying to improve those derivative classes :p

Berdir’s picture

Issue summary: View changes

???

Berdir’s picture

Issue summary: View changes

Add commit credit note

tim.plunkett’s picture

jibran’s picture

ParisLiakos’s picture

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorCategoryBlock.php
@@ -16,18 +16,55 @@
 class AggregatorCategoryBlock extends BlockBase {

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorFeedBlock.php
@@ -10,17 +10,69 @@
+class AggregatorFeedBlock extends BlockBase implements ContainerFactoryPluginInterface {

so how this works?
shouldnt AggregatorCategoryBlock implement ContainerFactoryPluginInterface too?
also no use statements are being added for eg ContainerInterface...so i guess we have *zero* tests for here..sigh

tim.plunkett’s picture

I don't know if it has tests or not, but if it doesn't implement ContainerFactoryPluginInterface, the create() method won't even be looked at, so the missing use statement shouldn't be a problem.

ParisLiakos’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

it still has a Connection typehint in the constructor which has no use

Berdir’s picture

Yeah, that was some crappy code.

Re-roll, fixed injection, also injected the category storage and added some basic tests. This means that the bug report about the already in HEAD broken category blocks doesn't have to add tests anymore, so can be closed as a duplicate :)

To lazy to compile a test-only patch with the previous patch + the new tests :)

Status: Needs review » Needs work

The last submitted patch, aggregator-feed-instance-setting-1888702-51.patch, failed testing.

Berdir’s picture

Re-roll and fixed the test.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

looks great! thanks @berdir!

catch’s picture

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Entity/Feed.php
@@ -226,14 +214,15 @@ public static function preDelete(EntityStorageControllerInterface $storage_contr
+    if (\Drupal::moduleHandler()->moduleExists('block')) {

Does this need a @todo for #2031859: Invoke an event[s] when a plugin ID disappears when it goes in?

Assuming it's the same thing, I have similar concerns to that issue, but we can keep them there since it's critical.

Otherwise this looks great. Not committing now purely due to time.

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/aggregator/aggregator.install
@@ -200,13 +200,6 @@ function aggregator_schema() {
-      'block' => array(
-        'type' => 'int',
-        'not null' => TRUE,
-        'default' => 0,
-        'size' => 'tiny',
-        'description' => "Number of items to display in the feed's block.",
-      )
     ),

Sorry I missed this yesterday. There's no upgrade path at all?

catch’s picture

Title: Use configuration selection instead of derivatives for some blocks » Change notice: Use configuration selection instead of derivatives for some blocks
Category: bug » task
Status: Needs review » Active
Issue tags: -Needs issue summary update +Needs change record

No upgrade path because there is no upgrade path for blocks as berdir reminded me on irc.

Committed/pushed to 8.x, thanks!

This could use a change notice. Also it's the sort of thing which could go into module interface guidelines/HIG or similar.

yoroy’s picture

First stab at what *I think* is going on here: https://drupal.org/node/2094663

What are possible other use cases? Would be good to have more than one example.

Berdir’s picture

Title: Change notice: Use configuration selection instead of derivatives for some blocks » Use configuration selection instead of derivatives for some blocks
Category: task » bug
Status: Active » Fixed
Issue tags: -Needs change record

@yoroy that looks ok to me. Not sure about other examples, this seems quite obvious to me. If you want another block example, that could be Simplenews: Instead of exposing multiple pre-defined newsletter subscription (each for each newsletter, one with newsletter selection, ..), expose a single subscription form block with configuration options.

Another example that already does this in 7.x, by implementing it's own system, is https://drupal.org/project/menu_block.

Created https://drupal.org/node/2094941. Didn't include any code examples, don't think it's that useful.

yoroy’s picture

Status: Fixed » Active

Ok, thanks for looking. But I don't think my docs count as a change notification?

tim.plunkett’s picture

Status: Active » Fixed

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

Anonymous’s picture

Issue summary: View changes

Update related issues