Provide listing of custom block entities

Will review early tomorrow

I tested this and everything works as expected. Bot's happy and the sooner we get this in the better as it's blocking other critical issues.

@tim.plunkett ahh, I cross posted with that :)

Have reviewed, just the items at #3 items 2 and 3 need fixing.
Lets leave #3 item 1 as is, makes sense to have them both in the same format rather than two different places.
Thanks @timplunkett for knocking this out so fast and sorry for derailing the other issue.

Here's the fixes from #3
Don't credit me in the commit.

Looks good now them changes are fixed.

Sorry, but I tested this manually last night, and the workflow is crazy. @tim.plunkett and I discussed this at length and it's mostly due to outstanding issues with the custom blocks workflow, but this patch unfortunately makes it that much more confusing.

I understand and support the desire to get this in as an intermediate step to unblock #2058321: Move the 'place block' UI into the block listing, but I think we need to go about it somewhat differently.

Couple actionable steps off the top of my head:

1. The redirect of the custom block creation form to the place blocks form is really unexpected. @tim.plunkett pointed out that this is out of scope for this issue, but at a minimum, it should only happen when the button is clicked from the place blocks form, and not when the form is submitted from a different path. That's the unexpected thing that completely threw me. (I use Drupal 8 every day, and I was just lost.)

   I think/hope this should just be a matter of setting a destination parameter.

   In #2058321: Move the 'place block' UI into the block listing this changes to "when the button is clicked on admin/structure/blocks".

2. The custom block listing page can't be a child tab under custom block types. No one will ever find that.

   A start, in scope for this patch, would be to make them both top-level items. Kevin also has some workflow designs from earlier in the release cycle that we could use to inform the placement. There's an out-of-scope usability problem here as to whether custom blocks are structure or content, but it definitely can't be a child page under "custom block types".

   We have three reasonable choices:

   • They're siblings under admin/structure.
   • The custom block list is under admin/content.
   • Custom block types is the child tab.

   In this issue, we should pick one and go with it, because anything is better than what's in HEAD currently, which is nothing. Either one can reference the other in help text.

3. We need a hook_help() on the custom block page that explains how it works.

4. We should probably have a "Place block" operation in the list controller for each block.

5. Let's get followups filed to expose custom block data to views (active) and to replace the list controller with a view (postponed). This patch is a slight abuse of the list controller. It works fine, even elegantly under the circumstances, but the ELC is designed for config entities, and we decided way back to standardize on Views for content entities.

6. There are out-of-scope issues that need to be filed for the custom block workflow as it stands in HEAD. We need to be able to reference those in order to clarify the scope of this patch and show that some of the UX WTFs are existing bugs rather than regressions.

Sorry to expand the scope so much, but I really think this can't go in like it is.

@@ -0,0 +1,110 @@
+    $this->assertTitle('Edit custom block Antelope | Drupal');
...
+    $this->assertTitle('Are you sure you want to delete Albatross? | Drupal');
...
+    $this->assertText('There is no Custom Block yet.');

Don't we use t() in that case?

@@ -0,0 +1,110 @@
+
+    // Verify that the text of the label and machine name does not appear in
+    // the list (though it may appear elsewhere on the page).
+    $this->assertNoFieldByXpath('//td', 'Albatross', "No label found for deleted 'Albatross' entity.");
+
+    // Confirm that the empty text is displayed.

Shouldn't we also test that the block is really deleted and not just disappears?

We need to at least add a help text for the list controller in this patch. It is part of the docs gate, even if it's been poorly followed in a lot of cases, including in previous BAP followups.

I'll take a stab at it in a bit. Edit: or delegate it; we'll see. :)

http://drupal.org/simpletest-tutorial-drupal7#t specifies that t() should be used where @dawehner recommends. Until that's changed, we should. Not a big deal either way, but that's the standard.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockListTest.phpundefined
@@ -0,0 +1,110 @@
+    $this->assertLinkByHref('block/1');

Also, as I mentioned to @tim.plunkett last night, it annoys me that we're assuming a serial ID of 1 here. Assumptions like that about the state of the test site are bad, even if this particular one is unlikely to be broken in the foreseeable future. :) I certainly won't block the patch over it, but if it were a node I would have asked the patch author to use drupalGetNodeByTitle() or such.

@@ -73,5 +73,5 @@ function custom_block_delete_form_submit($form, &$form_state) {
+  $form_state['redirect'] = 'admin/structure/custom-blocks';

I think we should explicitly set this in the 'Edit' link in the list controller using the destination parameter. Because now the converse is weird, clicking 'add custom block' from the place block ui sends you to the wrong place. IE I think if you 'add custom block' from the block UI, you should get the 'place block' form for that block. So this changes as follows: if you click edit link in controller, destination sends you back to the listing. If you click 'add link' in list controller, you come back. If you click the add link in blocks UI, you go to the 'place block' form. To get this working I had to use hook_menu_local_tasks() until #2045267: LocalTaskBase and LocalTaskInterface has to work with routes containing parameters is in, at which point we could add and then use \Drupal\custom_block\Plugin\Menu\CustomBlockAdd::getPath(). At this point in time local action plugins don't support query parameters, which sucks, so added a @todo.

I really think this should be admin/content/blocks instead of admin/structure but thats for another discussion, as part of the meta, and is a trivial change once we get *something* in.

With respect to the views integration we had a vicious circular dependency, the custom block revision ui needed the views integration and the views integration (to be complete) needed links to the revision UI. I say we change the focus of those issues. The existing custom block views integration issue forgets about revisions, then we add views integrations for revisions and revision UI in the other issue.

Also changes 'There is no Custom Block yet' to 'There are no Custom Blocks yet' which is more correct in terms of phrasing.

And adds hook_help(), moving the salient bits from block.module (where custom_block was spawned from in D7 codebase).

And fixes #15 and #20

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockTypeListController.phpundefined
@@ -66,4 +66,13 @@ public function buildRow(EntityInterface $entity) {
+    // @todo Remove this once https://drupal.org/node/1981644 is in.

We should refer to the issue for which this one's marked duplicate. (Here and the other spot.)

And move all but the list controller path from hook_help() into #2062761: Update hook_help() for custom_block modules

Fixes #22 and #23

Fixes #25

@@ -8,12 +8,13 @@
+      return 'Allows the creation of custom blocks through the user interface.';

Coding standards dictate any empty line after case breaking statements (return|break)

I think we're good here, but will leave for someone else to rtbc.

When you add a custom block from admin/structure/custom-blocks and you have multiple block types you don't end up back at structure/custom-blocks.

@@ -9,6 +9,21 @@
+    case 'admin/help#custom_block':
+      return 'Allows the creation of custom blocks through the user interface.';
+
+    case 'admin/structure/custom-blocks/types':
+      $output = '<p>This page provides a list of all custom-block types on your site. Each custom-block type can consists of different fields and display settings. From here you can manage the fields on each custom-block type as well as create new custom-block types.</p>';
+      return $output;

Shouldn't these be running through t()?

@@ -0,0 +1,118 @@
+    $this->assertTitle(t('Custom blocks | Drupal'));
...
+      $this->assertTitle(t('Edit custom block Antelope | Drupal'));
...
+    $this->assertFieldByXpath('//td', t('Albatross'), "Label found for updated 'Albatross' entity.");
...
+    $this->assertTitle(t('Are you sure you want to delete Albatross? | Drupal'));
...
+    $this->assertNoFieldByXpath('//td', t('Albatross'), "No label found for deleted 'Albatross' entity.");

Please use placeholders ... as we don't want to generate new strings. I guess the same counts for the other lines ....

+++ b/core/modules/block/custom_block/custom_block.moduleundefined
@@ -9,6 +9,21 @@
+function custom_block_help($path, $arg) {
+  switch ($path) {
+    case 'admin/help#custom_block':
+      return t('Allows the creation of custom blocks through the user interface.');
+
+    case 'admin/structure/custom-blocks/types':
+      $output = '<p>' . t('This page provides a list of all custom-block types on your site. Each custom-block type can consists of different fields and display settings. From here you can manage the fields on each custom-block type as well as create new custom-block types.') . '</p>';
+      return $output;
+
+  }
+}

s/can consists/can consist

Consider instead:

"The page lists user-created blocks. Blocks are derived from block types. A block type can consist of different fields and display settings. From the block types secondary tab you can manage these fields as well as create new block types.

I hope that tim is not 100% killed.

Let's turn our sights to #2062817: Custom block edit path is incorrect in hook_admin_paths() and the configure path is missing from the .info once this one is committed.

Derp, didn't mean to downgrade from RTBC. Testing and reviewed. Kevin O'Leary went over the UI with me in person and indicated approval.

The first point I mentioned in #32 still seems to be an issue for me:

To demonstrate from a fresh install what I mean.

• Add two custom block types
• Add a custom block from admin/structure/custom-blocks

Expected Behaviour

You arrive back at: admin/structure/custom-blocks

Actual Behaviour

You arrive at: admin/structure/block/list/bartik

Ok, sat down to review three sites tonight:

1. Just HEAD. #2062715: [META] Many UI/UX issues with custom blocks. covers the current state of that pretty well.
2. HEAD + this issue (#38).
3. HEAD + both patches (#44).

(Note: I tried not to read any of the other reviews so I could go into this fresh. I will probably repeat some of what's above, and will go back after and reconcile this after.)

With this patch on its own, the differences you'll note over the status quo are:

Label of menu item under admin/structure changed from "Custom block types" to "Custom blocks" (I think this is a good change; "Custom block types" looked weird just under "Content types"):

https://www.evernote.com/shard/s306/sh/1d494ec5-489b-4895-89f6-089baa812...

Under there, the first thing you'll notice is "Basic block" is gone (?!!) (It's actually there, but buried under a "types" sub-tab). Instead you get an empty listing with a grammar error. (this is being looked at in #1850080: Entity type labels lack plurality, cannot generate UI text based on label if plural is needed). On the bright side, the button label is fixed, so yay.

https://www.evernote.com/shard/s306/sh/d048a046-5f15-468d-ae33-aa38e074a...

I worry "Types" will not be found by anyone here. Though, post-review and looking back at previous replies, I can see why this is done. Moving custom blocks under "Content" opens up a huge can of worms (even if I think that's where it belongs ;)), and configuring types is definitely the lesser-used option, so makes sense for it to be buried over the custom block listing itself. We could argue that it's analogous to what we do with Roles on the People page.

---

Ok, let's try adding a block. In HEAD, this requires going back to admin/structure/block, then "Place block," then "Add custom block." Ouch. This is also why we need this patch for the other; if the place blocks UI goes away, the thing that the button's hinged off of no longer exists. With the patch, it's just at the top of the block listing table, same as with nodes. Yay.

In either case, you get a form that looks like this:

https://www.evernote.com/shard/s306/sh/5e211e07-ab17-4e16-b4a3-14b3dfb08...

It jumps to an "Add Basic block custom block" form immediately, because there's only one possible type (nodes do this, too). If there were multiple, you'd get a selection here, just as you do on node/add.

I didn't test the multiple block type flow, but looks like benjy did and found issues. Tim argues that those issues are an existing problem in HEAD and therefore shouldn't be dealt with here. I kind of prefer to see it fixed here so we can call this done, but not sure how involved it is.

In both flows, you end up at a listing with your block, and some operations you can do:

https://www.evernote.com/shard/s306/sh/1b60080b-af51-4228-8dd1-e280d2a11...

And here, you also are stranded, because the only thing you can do is either "Edit" or "Delete" your block. There's no operation to "Place" the block. This seems like a fairly obvious thing to add. Tim notes that there is no UI for selecting a theme; to me this'd just be a drop-down using the states system. However, since this is a pre-existing issue, and since it might need more detailed UX discussion, it can be a follow-up.

Also of note, hitting "Edit" on the block boots you out of the admin UI and back to Bartik. Wah-wah. This happens in HEAD too, though, so another follow-up.

In the current HEAD UI, you'd go to the place blocks page and see your block there now. With the new patch, same thing except you see it on the block listing in the sidebar:

https://www.evernote.com/shard/s306/sh/9004213e-a7e5-4d1b-b35e-4d0c36b27...

The place block pages look the same, other than HEAD uses vertical tabs and this patch seems to use Details elements, which makes a funky CSS problem when viewing at a narrower width:

https://www.evernote.com/shard/s306/sh/c9f8ac54-7ceb-4a39-bb06-7a1f76c98...

The CSS issues with narrower widths seem to have been introduced in this patch, so I'd like to see them fixed here if that is correct. If it's not correct (which is possible, since I was mooshing my windows all over the place to take screenshots), then follow-up it is.

Also, knowing Dries, he will want to see that title changed to the action you actually clicked on to get there.

---

Overall, this seems pretty nice, and definitely pushes things forward. None of the problems identified by either me nor others seem like they're critical/major; just normal stuff, all of which already exist in the current code, so I think this can probably be committed once the CSS issue is fixed, pending an answer on whether benjy's issue is easy to fix here, as well as the title change on place blocks form. It might be nice to get catch/alex to look too, though, if they're available.

Code looked fine to me, other than...

+++ b/core/modules/block/custom_block/custom_block.module
@@ -25,6 +40,19 @@ function custom_block_menu_local_tasks(&$data, $router_item, $root_path) {
+      $data['actions']['block/add/o'] = array(
+        '#theme' => 'menu_local_action',
+        '#link' => $item,
+      );

block/add/o? Is that a typo, or what does that mean?

Tentative needs work for the last paragraph or so in #45.

Yeah thats a typo

Also of note, hitting "Edit" on the block boots you out of the admin UI and back to Bartik. Wah-wah. This happens in HEAD too, though, so another follow-up.

There's an RTBC issue for that - details on the #2062715: [META] Many UI/UX issues with custom blocks.

...not anymore, there's not! ;)

@@ -47,7 +47,7 @@ function custom_block_menu_local_tasks(&$data, $router_item, $root_path) {
-      $data['actions']['block/add/o'] = array(
+      $data['actions']['block/add'] = array(

Was that a vim issue?

@@ -9,6 +9,21 @@
+  switch ($path) {
+    case 'admin/help#custom_block':
+      return t('Allows the creation of custom blocks through the user interface.');
...
+    case 'admin/structure/custom-blocks/types':

Maybe also a help message for admin/structure/custom-blocks would be helpful.

@@ -0,0 +1,50 @@
+  public function buildHeader() {
+    $header = parent::buildHeader();
+    unset($header['id']);
+    return $header;
+  }

It would be great to have "block description or just description" instead of "label" as the table header.

Thank you very much!

So the custom block help page looks at bit lonely. The help on admin/help/block is actually more useful. However whilst this page is actually more help the link it gives for adding custom blocks is admin/structure/block/list/bartik/add/custom_blocks - should this not be admin/structure/custom-blocks now?

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockListController.phpundefined
@@ -0,0 +1,51 @@
+class CustomBlockListController extends EntityListController {
...
+  public function buildHeader() {
+    $header = parent::buildHeader();
+    $header['label'] = t('Block description');
+    unset($header['id']);
+    return $header;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function buildRow(EntityInterface $entity) {
+    $row = parent::buildRow($entity);
+    unset($row['id']);
+    return $row;
+  }

Are we sure we want to the parent and then unset something it did we don't like pattern here. These methods won't really get any longer either.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockListTest.phpundefined
@@ -0,0 +1,122 @@
+    $this->assertLink($link_text);
+    $this->clickLink($link_text);

Is it necessary to both assetLink and clickLink?

In discussing with @timplunkett in IRC he pointed to #2062761: Update hook_help() for custom_block modules to improve the hook_help() implementation.

And we going to cover the inheritance issue with parent::buildHeader() and parent::buildRow() in an issue to correct all config entities. I think the pattern of calling parent and unsetting stuff is wasteful and will lead to unexpected bug in the future. Plus calling parent:: blindly leads is an anitpattern - inheritance is the tightest form of coupling.

And the other comment about the test is so minor I'm happy to ignore.

Committed a0d3030 and pushed to 8.x. Thanks!

This patch still introduced a major usability issue, as described in #42, that hasn't yet been fixed. I don't see any followup issue for it here, so I filed: #2068053: Custom block creation form redirect is inconsistent

Closed #2068053: Custom block creation form redirect is inconsistent as dupe of #2064591: Missing test coverage for multiple custom block types

No, it wasn't.... try it... watch the video. :)