Problem/Motivation

This is a followup issue for #1535868: Convert all blocks into plugins. #1874598: Add BlockTestBase should also be completed before or as a part of this issue.

  • In custom_block.module, custom blocks are read from and written to the database table directly.
  • There are currently no hooks for contributed modules to respond to custom block CRUD.

Proposed resolution

  • Convert custom blocks to the entity system.
  • Add full test coverage, including unit tests for CRUD, test coverage for hook implementations, etc.

Remaining tasks

Sandbox is here:
http://drupalcode.org/sandbox/larowlan/1790736.git/tree/refs/heads/custo...

  • Convert custom block derivative plugins to be keyed by machine name, add machine name to {custom_block} - avoid use of bid where possible
  • Add view mode to block instance config and use this in the block render callback
  • Finish the storage controller (not yet pushed)
  • Tests, Tests, Tests
  • Form alters to add in translation support switch to block type configuration form

Related issues

Follow up tasks

CommentFileSizeAuthor
#111 custom-block-types-or-block-types.jpg117.17 KBDries
#111 adding-a-new-block.jpg319.37 KBDries
#111 quote-block-type.jpg203.15 KBDries
#111 add-custom-block.jpg390.89 KBDries
#111 add-custom-block-2.jpg515.1 KBDries
#104 1871772-103.patch117.63 KBEclipseGc
#104 1871772-103-interdiff.txt6.43 KBEclipseGc
#100 custom-blocks-content-entities-1871772.100.interdiff.txt591 byteslarowlan
#100 custom-blocks-content-entities-1871772.100.patch117 KBlarowlan
#91 custom-blocks-content-entities-1871772.91.interdiff.txt25.5 KBlarowlan
#91 custom-blocks-content-entities-1871772.91.patch116.59 KBlarowlan
#88 custom-blocks-content-entities-1871772.88.interdiff.txt27.84 KBlarowlan
#88 custom-blocks-content-entities-1871772.88.patch113.53 KBlarowlan
#80 custom-blocks-content-entities-1871772.80.interdiff.txt23.03 KBlarowlan
#80 custom-blocks-content-entities-1871772.80.patch114.42 KBlarowlan
#77 custom-blocks-content-entities-1871772.77.interdiff.txt728 byteslarowlan
#77 custom-blocks-content-entities-1871772.77.patch115.12 KBlarowlan
#75 custom-blocks-content-entities-1871772.75.interdiff.txt729 byteslarowlan
#75 custom-blocks-content-entities-1871772.75.patch115.12 KBlarowlan
#73 custom-blocks-content-entities-1871772.73.interdiff.txt31.91 KBlarowlan
#73 custom-blocks-content-entities-1871772.73.patch115.12 KBlarowlan
#66 custom-blocks-content-entities-1871772.66.interdiff.txt19.3 KBlarowlan
#66 custom-blocks-content-entities-1871772.66.patch114.19 KBlarowlan
#57 custom-blocks-content-entities-1871772.57.interdiff.txt4.83 KBlarowlan
#57 custom-blocks-content-entities-1871772.57.patch112.89 KBlarowlan
#55 custom-blocks-content-entities-1871772.55.interdiff.txt1.34 KBlarowlan
#55 custom-blocks-content-entities-1871772.55.patch113.35 KBlarowlan
#53 custom-blocks-content-entities-1871772.53.interdiff.txt60.01 KBlarowlan
#53 custom-blocks-content-entities-1871772.53.patch113.21 KBlarowlan
#51 custom-blocks-content-entities-1871772.51.interdiff.txt470 byteslarowlan
#51 custom-blocks-content-entities-1871772.51.patch111.21 KBlarowlan
#49 custom-blocks-content-entities-1871772.49.patch111.44 KBlarowlan
#45 custom-blocks-content-entities-1871772.45.interdiff.txt469 byteslarowlan
#45 custom-blocks-content-entities-1871772.45.patch112.46 KBlarowlan
#42 custom-blocks-content-entities-1871772.42.interdiff.txt13.24 KBlarowlan
#42 custom-blocks-content-entities-1871772.42.patch112.46 KBlarowlan
#39 custom-blocks-content-entities-1871772.39.interdiff.txt51.56 KBlarowlan
#39 custom-blocks-content-entities-1871772.39.patch101.94 KBlarowlan
#27 custom_block_flow.jpg171.06 KBtkoleary
#24 custom-blocks-content-entities-1871772.24.interdiff.txt27.34 KBlarowlan
#24 custom-blocks-content-entities-1871772.24.patch62.26 KBlarowlan
#18 custom-blocks-content-entities-1871772.18.do-not-test.patch54.82 KBlarowlan
#13 Screen Shot 2013-01-07 at 3.54.08 PM.png14.63 KBlarowlan
#13 Screen Shot 2013-01-07 at 3.54.17 PM.png19.62 KBlarowlan
#13 Screen Shot 2013-01-07 at 3.57.22 PM.png22.42 KBlarowlan
#13 Screen Shot 2013-01-07 at 3.57.34 PM.png20.27 KBlarowlan
#13 Screen Shot 2013-01-07 at 8.42.27 AM.png39.51 KBlarowlan
#13 Screen Shot 2013-01-07 at 8.42.38 AM.png29.85 KBlarowlan
#8 broken_custom_block_form.png9.02 KBxjm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein’s picture

andypost’s picture

Depending on what the block is ... (content || config) ... I think #1833872: Allow ConfigEntity types to be fieldable needs to be fixed too

xjm’s picture

#2 is not applicable here, since custom block definitions should be normal content entities. Block instances should be configuration entities (in a separate issue, #1871696: Convert block instances to configuration entities to resolve architectural issues), but they also don't need fields.

#430886: Make all blocks fieldable entities has been closed as a duplicate of this issue. Edit: Or someone keeps reopening it. But it is obsolete following the blocks plugin conversion.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue tags: +Needs tests
xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

larowlan’s picture

So I wanted to start plugging around on this, so figured I'd clone the sandbox from #1535868: Convert all blocks into plugins and use the 1535868-blocks branch as the basis for my work.
Is the sandbox 1535868-blocks branch in the summary up to date?
I get a 'page not found' trying to edit a custom block (by clicking the edit link for a custom block on the custom block listing page at admin/structure/block/list/block_plugin_ui%3Abartik/add/custom_blocks_library)

xjm’s picture

Yes, the branch is up to date. I'm not able to reproduce the problem you describe. Is it a fresh install from a fresh clone? If so could you provide explicit STR?

xjm’s picture

Wait, I get the same message if I go to admin/structure/block/list/block_plugin_ui%3Abartik/add/custom_blocks_library by pasting it in, but I don't know how you got to that path.

xjm’s picture

Ah, I found it; there's another local task:

broken_custom_block_form.png

Looks like this was going to be the separate admin interface for custom blocks, but it was never finished. In custom_block_admin_custom_block() there's only markup for this form, with no submit handlers or anything. I'll just remove this fragment of a UI from the plugins patch in favor of whatever interface we add here.

larowlan’s picture

Nice, so that means the ui for the custom blocks is part of this issue ala admin/content/node or admin/content/blocks in bean land?

xjm’s picture

Something like that. The exact workflow is to be determined, but having them as entities will simplify creating the admin interface. Edit: see the followup issue: #1875058: Provide separate interface for editing custom blocks

xjm’s picture

Status: Postponed » Active
larowlan’s picture

Assigned: Unassigned » larowlan

Spoke with EclipseGC on irc.
Being bold.
If I get nowhere in a week, I'll unassign.

larowlan’s picture

hass’s picture

Do we want the custom blocks to be revisionable? translatable?

Yes, for sure!

EclipseGc’s picture

Also yes

larowlan’s picture

Added sandbox to issue summary

larowlan’s picture

Issue summary: View changes

Updated issue summary.

larowlan’s picture

Issue summary: View changes

Updated to-do and added sandbox

larowlan’s picture

Updated remaining tasks

larowlan’s picture

WIP - making good progress, onto tests and then translations next.
@timplunkett has asked that the config entity (block type) be split into a separate follow up, will do that once tests are done.

Demo video of status
http://youtu.be/A5zcbAtAKJ0

larowlan’s picture

Issue summary: View changes

updated to-do

larowlan’s picture

Updated remaining tasks

hass’s picture

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
@@ -0,0 +1,240 @@
+    // Clear the page and block caches.
+    cache_invalidate_tags(array('content' => TRUE));

Is it possible to clear only this block from cache? Otherwise the complete site may be in performance troubles.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Core/Entity/CustomBlock.php
@@ -0,0 +1,207 @@

@@ -0,0 +1,207 @@
+<?php
+
+/**
+ * @file
+ * Definition of Drupal\node\Plugin\Core\Entity\Node.
+ */

Node?

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/block/block/CustomBlock.php
@@ -105,12 +87,19 @@ public function blockSubmit($form, &$form_state) {
+        '#markup' => t('Block with name %name does not exist, please <a href="!url">create it</a>.', array(

'Block with name %name does not exist. <a href="@url">Add custom block</a>.'

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/block/block/CustomBlock.php
@@ -105,12 +87,19 @@ public function blockSubmit($form, &$form_state) {
+          '!url' => 'block/add'

@url, url() seems missing?

Gábor Hojtsy’s picture

@larowlan: now that #1535868: Convert all blocks into plugins landed, is this still readily applicable?

Also wanted to add that on top of making blocks support content translation right away this would also be beautiful for making custom blocks readily in-place editable (as in #1882482: [meta] Unify editing approaches in Drupal).

Gábor Hojtsy’s picture

Never mind my #1535868: Convert all blocks into plugins applicability question, I looked at the video. Looks powerful :) :) Congrats.

xjm’s picture

@Gábor Hojtsy, making custom blocks content entities was always part of the plan following #1535868: Convert all blocks into plugins. :)

larowlan’s picture

Added translation support by porting to use EntityNG et al (thanks @berdir for guidance).
Revision support appears to be working.
Lets see how much breakage there is against HEAD before I start on new tests.

larowlan’s picture

Also above fixes issues at #20

Status: Needs review » Needs work

The last submitted patch, custom-blocks-content-entities-1871772.24.patch, failed testing.

tkoleary’s picture

FileSize
171.06 KB

@larowlan This is awesome work and something that I think we have needed for a long time. However, in it's current state there are some pretty big usability problems with it which I have outlined in the attached image. I don't think I'm introducing any fundamentally new ideas here, just trying to better employ existing patterns and create consistency across UIs. Also a chunk of this probably applies more to eclipseGC and his implementation of plugin blocks which I am also taking apart.

tim.plunkett’s picture

It seems like that conflates making a new block type with making a new block entity.

Also, the description part with the checkbox, for example, is not a pattern found anywhere that I know of outside the Views UI)

I think that a custom bundle UI should be separate from this issue.

tim.plunkett’s picture

I guess what I mean is, let's just transition the backend over to using content entities, and worry about the UI separately, like we have for every other conversion.

tkoleary’s picture

@tim.plunkett I'm ok with that as long as it gets done. :)

tkoleary’s picture

@tim.plunkett Should I start a new issue for that and link to this one?

larowlan’s picture

thanks @tkoleary, I agree with much of your analysis. You are correct in saying much of this is down to existing state. I think the best approach is to file follow ups for each of the issues. The description of block types will need to be in a follow up (assuming the block type concept is removed from this patch as per @timplunkett's request to keep this patch smaller).

tkoleary’s picture

@tim.plunkett But re: "seems like that conflates making a new block type with making a new block entity."

I don't follow. The design maps exactly to the node flow.

EclipseGc’s picture

Actually the problem with the mockups is that it conflates creating new custom blocks with placing new block instances, and more importantly, puts those forms on the same page (which is a technical limitation I ran into LAST time I tried to do this, and one I'm not willing to be blocked on). That's why I asked for it to be wizarded together the way I did. Otherwise it seems pretty close to what you've outlined here.

Eclipse

Gábor Hojtsy’s picture

Actually the problem with the mockups is that it conflates creating new custom blocks with placing new block instances,

Where is that problem? When I try to add a new custom block now it asks me the block description, title, body AND the region on the page at admin/structure/block/list/block_plugin_ui%253Abartik/add/custom_blocks. It clearly does not currently create a new instance, because although it looks like it does on the outset it says on the form fields, that it would merely change the one existing custom block body and description if I enter something, so essentially current Drupal 8 can only have up to one custom block on the whole site(?).

Is this what got you to the technical limitation you have run into earlier?

tkoleary’s picture

@eclipseGC The mockup assumes the user is creating a new custom block from an (already existing) custom block type. If it is necessary to also create a new instance of the (first) use of this custom block then that should be done in the background IMHO.

larowlan’s picture

larowlan’s picture

Progress update: 4 failing tests to go, will continue on Monday morning.
Needed some of the field_attach_* changes from #1778178: Convert comments to the new Entity Field API to get field output with EntityNG, sunk a heap of time into debugging that :(.

larowlan’s picture

Ok, this adds tests for all new functionality as follows

  • content tests
  • creation tests
  • hook/load tests
  • revision tests
  • save tests
  • translation ui tests
  • block type tests
  • edit tests

sandbox commits

68ee443 Remove unit test base, not used
6057914 Added tests
97de390 Entity BC changes required for viewing fields
799c458 Don't set default langcode to string
b87ef32 Only setup body field on block type create
35cd81a Get contextual links working
1bbccb2 Set default view mode, not full - full not always available in early install
ef22ded Add text as dependency

still to do

  • Get 3 failing tests in head to pass (and any more this breaks)
  • Split bundle ui from this patch into follow up (assuming folks still want it as two patches)

Status: Needs review » Needs work

The last submitted patch, custom-blocks-content-entities-1871772.39.patch, failed testing.

xjm’s picture

Issue tags: +Block plugins
larowlan’s picture

New commits

2e98cb0 Fixes test coverage
8a9b66d Provides upgrade path for schema changes

Updating remaining tasks on issue summary.

larowlan’s picture

Issue summary: View changes

Updated remaining tasks

larowlan’s picture

Assigned: larowlan » Unassigned

Status: Needs review » Needs work

The last submitted patch, custom-blocks-content-entities-1871772.42.patch, failed testing.

larowlan’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests, -Entity system, -D8 upgrade path, -VDC, -Blocks-Layouts, -Block plugins

The last submitted patch, custom-blocks-content-entities-1871772.45.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Needs tests, +Entity system, +D8 upgrade path, +VDC, +Blocks-Layouts, +Block plugins

The last submitted patch, custom-blocks-content-entities-1871772.45.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
111.44 KB

re-roll against head after #1778178: Convert comments to the new Entity Field API went in.

hass’s picture

Status: Needs review » Needs work

There is at least one debug() call

larowlan’s picture

at least it was clean, could have been much worse given my usual standards

larowlan’s picture

Ok, so this is green now.
We need to decide if the custom bundle ui should be taken out of this patch or added as is and cleaned up in #1875058: Provide separate interface for editing custom blocks.
Not fussy either way.

larowlan’s picture

Ok, so here's latest patch after #1871696: Convert block instances to configuration entities to resolve architectural issues went in.
It won't pass, I can't get CustomBlockTranslationUITest.php to pass locally, so something else in core has changed that I'm not across.
Basically, the translated field values are not being stored properly. Will try and debug it again tomorrow morning but hoping someone might notice this and go oh, that was '#123456 Random translation ui change you didn't know about'.

Also found changes in #611294: Refine permissions for Field UI were breaking some of the new tests too, updated that to request a change notice.

Had a discussion with @sun on irc about this, he advocates removing the machine name from the plugin ids, and revert back to the serial bids, leaving the 'content staging' question for another issue. I'm kind of torn, because we are running out of time and I feel that being able to deploy layouts that include custom blocks will be pivotal to my ability to do my job effectively for the Drupal 8 life-cycle. If we don't get it done in another issue after this point, we might have let something slip here. That said, I see his point about there needing to be a larger discussion about content staging. I guess it all depends where layouts /SCOTCH ends up. This patch still uses machine name. I will rip it out if thats the consensus we come to.

Patch still includes the bundle ui, same deal again.

Patch also includes some coding standards cleanup (run through coder_sniffer) for the new tests.

A shiny pony to anyone who can fix the translation test (latest code is in the sandbox).

Let's see what else is failing (please bot).

Status: Needs review » Needs work

The last submitted patch, custom-blocks-content-entities-1871772.53.patch, failed testing.

larowlan’s picture

heh, can't grant 'administer custom_block fields' permission without field_ui enabled, dropping that perm for all but the block type test.

Status: Needs review » Needs work

The last submitted patch, custom-blocks-content-entities-1871772.55.patch, failed testing.

larowlan’s picture

So, turns out EntityNG you unset all the values except langcode during ::init method.
Also cleaned up the language config element on bundle, didn't need to use the long-way round comment bundle method.

If this comes back green I consider it ready for real reviews and for the discussion around machine names and the bundle ui.

Status: Needs review » Needs work
Issue tags: -Needs tests, -Entity system, -D8 upgrade path, -VDC, -Blocks-Layouts, -Block plugins

The last submitted patch, custom-blocks-content-entities-1871772.57.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Needs tests, +Entity system, +D8 upgrade path, +VDC, +Blocks-Layouts, +Block plugins

The last submitted patch, custom-blocks-content-entities-1871772.57.patch, failed testing.

larowlan’s picture

Looks like random test failures, postponing for now until #1893800: Something is very, very wrong with update.php / upgrade tests... demons suspected is resolved

larowlan’s picture

Status: Needs work » Needs review
larowlan’s picture

Yay, green again

Ok, this is ready for real reviews and the discussion around bundle ui and machine name.

thanks in advance

andypost’s picture

Patch looks awesome! There's a few code-related issues

+++ b/core/modules/block/block.install
@@ -194,6 +194,220 @@ function block_update_8006() {
+  db_create_table('block_custom_revision', array(
...
+  db_add_field('block_custom', 'vid', array(
...
+  db_add_field('block_custom', 'uuid', array(

+++ b/core/modules/block/custom_block/custom_block.install
@@ -33,17 +32,83 @@ function custom_block_schema() {
+  $schema['block_custom_revision'] = array(
+    'description' => 'Stores contents of custom-made blocks.',

custom_block <=> {block_custom*} tables?
Suppose naming should be unified.
Also changing the tables means no way for contrib to upgrade data. Suppose module should use own tables by migrating {block_custom} data to them. See #1860986: Drop left-over tables from 8.x

+++ b/core/modules/block/custom_block/custom_block.admin.inc
@@ -0,0 +1,101 @@
+function custom_block_type_list() {
+  drupal_set_title(t('Custom block types'));

Place title to hook_menu()

+++ b/core/modules/block/custom_block/custom_block.admin.inc
@@ -0,0 +1,101 @@
+function custom_block_type_add() {
+  drupal_set_title(t('Add custom block type'));

same

+++ b/core/modules/block/custom_block/custom_block.module
@@ -19,19 +22,121 @@ function custom_block_menu() {
       $items['admin/structure/block/list/' . $plugin_id . '/add/custom_blocks'] = array(
         'title' => 'Add custom block',
...
-        'page callback' => 'block_admin_add',
-        'page arguments' => array('custom_block:custom_block', $theme),
-        'access callback' => TRUE,
+        'page callback' => 'drupal_goto',
+        'page arguments' => array(
+          'block/add',
+          array('query' => array('theme' => $theme))
+        ),
+        'access arguments' => array('administer blocks'),

A kind of magic???

+++ b/core/modules/block/custom_block/custom_block.module
@@ -53,3 +158,160 @@ function theme_custom_block_block($variables) {
+function custom_block_type_load($id) {
...
+function custom_block_load($bid) {

Please use [module]_arg_load() pathern here

+++ b/core/modules/block/custom_block/custom_block.module
@@ -53,3 +158,160 @@ function theme_custom_block_block($variables) {
+function custom_block_type_uri(CustomBlockType $block_type) {

[module]_entity_uri()

+++ b/core/modules/block/custom_block/custom_block.pages.inc
@@ -0,0 +1,164 @@
+function custom_block_page_view(CustomBlock $block) {
...
+  drupal_set_title($block->label());

entity_label for hook_menu() is enough

+++ b/core/modules/block/custom_block/custom_block.pages.inc
@@ -0,0 +1,164 @@
+function custom_block_add_page() {
+  drupal_set_title(t('Add custom block'));

same

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockTypeListController.php
@@ -0,0 +1,58 @@
+   * Overrides Drupal\Core\Entity\EntityListController::buildRow().
+   */
+  public function buildRow(EntityInterface $entity) {
+    $row['type'] = check_plain($entity->label());
+    $row['operations']['data'] = $this->buildOperations($entity);

Please use parent::buildRow() as default and make a title as link, we need consistency for admin listings in core, more in #1855402: Add generic weighting (tabledrag) support for config entities (DraggableListController)

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Core/Entity/CustomBlock.php
@@ -0,0 +1,214 @@
+    return $this->bid->value;
...
+    return $this->type->value;
...
+    return $this->info->value;
...
+    $duplicate = parent::createDuplicate();
+    $duplicate->set('vid', NULL);
+    $duplicate->set('bid', NULL);
+    $duplicate->set('machine_name', NULL);
...
+    parent::init();
+    // We unset all defined properties, so magic getters apply.
+    unset($this->bid);
+    unset($this->info);
+    unset($this->vid);
+    unset($this->log);
+    unset($this->machine_name);
+    unset($this->uuid);
+    unset($this->type);
+    unset($this->new);

Different access methods - is there any reason for?

larowlan’s picture

Thanks @andypost, keep the reviews coming!

While I think of it, in regards to using bid/machine_name in block config - quoting @timplunkett from #916388-143: Convert menu links into entities

If we store content entities in a config entity, we'd have to use the UUID. Not sure if that's a good thing or not, but not sure what the other options are.

The same applies here, at the moment I'm using machine name over bid (HEAD uses bid), but uuid works for me too.

larowlan’s picture

Please use [module]_arg_load() pathern here

We can't because these will be seen as implementations of hook_ENTITY_TYPE_load() in both instances.

Different access methods - is there any reason for?

During init, the various properties are raw values, we use the unset() method so that magic getters/setters apply. However the ->set()'s can go.

Made all the other suggested changes and merged in 8.x.

Added to list of deprecated tables in #1860986: Drop left-over tables from 8.x

Updated for hook_entity_bundle_info() and hook_entity_view_mode_info().

Go bot go!

larowlan’s picture

Green again!

Berdir’s picture

Here's a review of the entity related stuff...

+++ b/core/modules/block/block.installundefined
@@ -194,6 +194,239 @@ function block_update_8006() {
+      'bid' => array(
+        'type' => 'serial',
+        'unsigned' => TRUE,
+        'not null' => TRUE,
+        'description' => "The block's {custom_block}.bid.",

I think the goal is towards standardizing the primary key of an entity as 'id'. (and having something like user_id, node_id, custom_block_id for references).

As this is completeley new code, it makes sense to start with that.

+++ b/core/modules/block/block.installundefined
@@ -194,6 +194,239 @@ function block_update_8006() {
+      'vid' => array(

Not sure what is the plan for the revision id but I'd go for revision_id.

+++ b/core/modules/block/block.installundefined
@@ -194,6 +194,239 @@ function block_update_8006() {
+    'primary key' => array('bid'),

Nitpick: I think the primary key usually comes first, I was actually confused for a second and wondered why there is none :)

+++ b/core/modules/block/block.installundefined
@@ -194,6 +194,239 @@ function block_update_8006() {
+  db_create_table('custom_block_revision', array(

I think at least the label (info?) should be part of the revision table as well, so that it can be revisioned..

+++ b/core/modules/block/block.installundefined
@@ -194,6 +194,239 @@ function block_update_8006() {
+    $block->vid = $revision['vid'];
+    $block->langcode = LANGUAGE_NOT_SPECIFIED;
+    $block->type = 'basic';
+    $block->machine_name = preg_replace('^([a-z|0-9|\_]', '', $block->info);
+    $revision = array(
+      'bid' => $block->bid,
+      'log' => 'Initial value from 7.x to 8.x upgrade'
+    );
+    drupal_write_record('custom_block', $block);
+    drupal_write_record('custom_block_revision', $revision);

Hm. I think we do want to move away from dwr(). My suggestion would be to use normal db_insert() queries here. You do have a fixed list of properties that you know you want to store...

+++ b/core/modules/block/block.installundefined
@@ -194,6 +194,239 @@ function block_update_8006() {
+/**
+ * Generate a UUID for all blocks.
+ */
+function block_update_8008(&$sandbox) {

Is there a reason for not doing this at the same time?

The previous update function already assumes that there aren't so many custom blocks or it would need a batch function too.

+++ b/core/modules/block/block.installundefined
@@ -194,6 +194,239 @@ function block_update_8006() {
+    // Used below when updating the stored text format of each body.
+    $sandbox['existing_text_formats'] = db_query("SELECT format FROM {filter_format}")->fetchCol();

Wondering if this should depend on the filter update function that converts filter formats to CMI and get the information from there.

+++ b/core/modules/block/custom_block/custom_block.admin.incundefined
@@ -0,0 +1,99 @@
+ * @param Drupal\custom_block\Plugin\Core\Entity\CustomBlockType $block_type

\Drupal\...

+++ b/core/modules/block/custom_block/custom_block.moduleundefined
@@ -19,19 +22,121 @@ function custom_block_menu() {
+        'page callback' => 'drupal_goto',

Woah, drupal_goto() as a page callback is ... interesting ;)

We want to remove that function (it is a major task, actually), so I think we need to find another solution here.

+++ b/core/modules/block/custom_block/custom_block.moduleundefined
@@ -19,19 +22,121 @@ function custom_block_menu() {
+  $items['block/add/%custom_block_type'] = array(
+    'title callback' => 'entity_page_label',
+    'title arguments' => array(2),
+    'page callback' => 'custom_block_add',
+    'page arguments' => array(2),
+    'access arguments' => array('administer blocks'),
+    'description' => 'Add custom block',
+    'file' => 'custom_block.pages.inc',

Should we already introduce an access controller and use that?

+++ b/core/modules/block/custom_block/custom_block.moduleundefined
@@ -19,19 +22,121 @@ function custom_block_menu() {
+  $items['block/%custom_block_machine_name'] = array(
+    'title callback' => 'entity_page_label',
+    'title arguments' => array(1),
+    'page callback' => 'custom_block_page_view',
+    'page arguments' => array(1),
+    'access arguments' => array('view custom blocks'),
+    'file' => 'custom_block.pages.inc',

- Not sure if we really want to use the machine name here (not exactly sure what the idea/goal of that is)

- What exactly is this page for? A block isn't really meant to be displayed like that or not? Is it for admins to see how it looks? Is the view custom blocks permissions specific to this page or is it always checked when blocks are displayed?

+++ b/core/modules/block/custom_block/custom_block.moduleundefined
@@ -53,3 +158,194 @@ function theme_custom_block_block($variables) {
+function custom_block_machine_name_load($machine_name) {
+  $block_ids = entity_query('custom_block')->condition('machine_name', $machine_name)->execute();
+  if (!empty($block_ids) && ($block_id = reset($block_ids))) {
+    return entity_load('custom_block', $block_id);

This could use entity_load_by_properties() I think.

+++ b/core/modules/block/custom_block/custom_block.pages.incundefined
@@ -0,0 +1,159 @@
+ * Page callback: Displays a single node.

I know where you copied that from :)

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockStorageController.phpundefined
@@ -0,0 +1,126 @@
+    // Invalidate the block cache to update custom block-based derivatives.
+    if (module_exists('block')) {
+      drupal_container()->get('plugin.manager.block')->clearCachedDefinitions();

We depend on block.module, so the check isn't necessary in this case.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Core/Entity/CustomBlock.phpundefined
@@ -0,0 +1,208 @@
+  public function label($langcode = NULL) {
+    return $this->info->value;

The label method is a bit tricky, because this overrides the configurability. You might want to leave it away and use the default implementation unless there is a problem with that.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Core/Entity/CustomBlockType.phpundefined
@@ -0,0 +1,65 @@
+ * Definition of Drupal\custom_block\Plugin\Core\Entity\CustomBlockType.

Search and replace Definition of with Contains :) And \Drupal\...

+++ b/core/modules/field/field.attach.incundefined
@@ -1430,6 +1430,9 @@ function field_attach_view(EntityInterface $entity, EntityDisplay $display, $lan
   // Remove the BC layer now.
   $entity = $entity->getOriginalEntity();
 
+  // Remove the BC layer now.
+  $entity = $entity->getOriginalEntity();

Duplicate code, probably a merge left-over...

larowlan’s picture

Thanks berdir, great feedback.
Will address all these points shortly.
The block page view callback is required for translation entity. It won't add it's tabs and menu callbacks if there is no page view of the entity. If we can get around that in a generic fashion, happy to remove it. I didn't think adding a chunk of conditional code in that module, specific to custom block was the way to go and also thought duplicating translation entity code in a hook menu alter in custom block module seemed no better. What are your thoughts?

Berdir’s picture

Ah, I see. IMHO, having an edit tab should be enough to be able to get the translate tab, I'd say that's something that should be improved in translation_entity.module. I'd suggest you talk with @plach about that.

larowlan’s picture

The label method is a bit tricky, because this overrides the configurability. You might want to leave it away and use the default implementation unless there is a problem with that.

This is only used in the block library. The label can be configured when placing the instance and all uses of the instance will fetch that label (from the instance config entity). i.e. they are two distinct entities, the block instance (config entity) where the label used when placing the block is set and the other the base entity (custom block entity) which provides the plugin that the instance encapsulates.
Have sent plach a message on irc. Thanks.

larowlan’s picture

The label method is a bit tricky, because this overrides the configurability. You might want to leave it away and use the default implementation unless there is a problem with that.

Berdir clarified that he meant, I'm overriding EntityNG::label here, which means the label entity key can't be overridden.
Makes sense now.

larowlan’s picture

I think this covers all of @berdir's points except

Wondering if this should depend on the filter update function that converts filter formats to CMI and get the information from there.

I just removed that hunk, it wasn't used.

Status: Needs review » Needs work

The last submitted patch, custom-blocks-content-entities-1871772.73.patch, failed testing.

larowlan’s picture

missed the entity key changes after renaming table cols

Status: Needs review » Needs work

The last submitted patch, custom-blocks-content-entities-1871772.75.patch, failed testing.

larowlan’s picture

groan, revision_id != revision_vid

Status: Needs review » Needs work

The last submitted patch, custom-blocks-content-entities-1871772.77.patch, failed testing.

larowlan’s picture

So there's a shipload of vid and bid in the test code and a few refs in the controllers, need to rename those too. Have some grep results and will resolve asap.

larowlan’s picture

This should be closer now

Status: Needs review » Needs work

The last submitted patch, custom-blocks-content-entities-1871772.80.patch, failed testing.

larowlan’s picture

That test passes locally. Kicking off again

larowlan’s picture

Status: Needs work » Needs review
larowlan’s picture

Green again

larowlan’s picture

This one ready for serious reviews and the conversations about machine name/uuid/bid in the plugin id and the block type ui.
Tag update.

hass’s picture

Is there a need to get this patch committed before 18th February deadline? It would be extremly bad if this is not going in D8...

EclipseGc’s picture

+++ b/core/modules/block/custom_block/custom_block.admin.incundefined
@@ -0,0 +1,99 @@
+  return entity_list_controller('custom_block_type')->render();

This should now be:

return drupal_container()->get('plugin.manager.entity')->getListController('custom_block_type')->render();
+++ b/core/modules/block/custom_block/custom_block.admin.incundefined
@@ -0,0 +1,99 @@
+  drupal_set_title(t('Edit %label custom block type', array('%label' => $block_type->label())), PASS_THROUGH);

I prefer to see this on the menu callback if possible. If not that's fine too.

+++ b/core/modules/block/custom_block/custom_block.moduleundefined
@@ -53,3 +133,203 @@ function theme_custom_block_block($variables) {
+/**
+ * Entity URI callback.
+ *
+ * @param Drupal\custom_block\Plugin\Core\Entity\CustomBlockType $block_type
+ *   A custom block type entity.
+ *
+ * @return array
+ *   An array with 'path' as the key and the path to the custom block type as
+ *   the value.
+ */
+function custom_block_custom_block_type_uri(CustomBlockType $block_type) {
+  return array(
+    'path' => 'admin/structure/custom-blocks/manage/' . $block_type->id(),
+  );
+}
+
+/**
+ * Entity URI callback.
+ *
+ * @param Drupal\custom_block\Plugin\Core\Entity\CustomBlock $block
+ *   A custom block entity.
+ *
+ * @return array
+ *   An array with 'path' as the key and the path to the custom block as the
+ *   value.
+ */
+function custom_block_custom_block_uri(CustomBlock $block) {
+  return array(
+    'path' => 'block/' . $block->machine_name->value,
+  );
+}

Is there a reason these aren't methods on the entity classes? They support that.

+++ b/core/modules/block/custom_block/custom_block.moduleundefined
@@ -53,3 +133,203 @@ function theme_custom_block_block($variables) {
+  foreach (config_get_storage_names_with_prefix('custom_block.type.') as $config_name) {

I think there's a listAll() method or something that should do this for you. Maybe I'm full of crap. Entities aren't my forté.

+++ b/core/modules/block/custom_block/custom_block.moduleundefined
@@ -53,3 +133,203 @@ function theme_custom_block_block($variables) {
+    @list($base, $derivative) = explode(':', $plugin_id);

Do we have to? maybe this is the best way to do it, I dunno.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.phpundefined
@@ -0,0 +1,227 @@
+class CustomBlockFormController extends EntityFormControllerNG {

++ for being on top of the NG train here.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockStorageController.phpundefined
@@ -0,0 +1,124 @@
+/**
+ * @file
+ * �tom_block\CustomBlockStorageController.

?? looks like a bad character in dreditor.

I've not made it all the way through, but I am reviewing this. In general there are a lot of things that have changed in the entity system that need some tweaking here. if timplunkett could review it that'd be great. I'll try to finish my review tomorrow. I'll also try to install this tomorrow and give it a functional test.

Eclipse

larowlan’s picture

So this fixes the issues identified by @EclipseGC at #87 and #27.2 from @tkoleary

Also removes the machine_name in favour of uuid as discussed with @sun in IRC and @timplunkett quote at #65

larowlan’s picture

Green again - I think we're getting close!

tim.plunkett’s picture

No architectural problems left, IMO. Just some nitpicks and then RTBC.

+++ b/core/modules/block/custom_block/custom_block.moduleundefined
@@ -5,6 +5,12 @@
+use Drupal\custom_block\Plugin\block\block\CustomBlock as CustomBlockPlugin;

This class should probably just be renamed. Our standard is that the class names should be differentiable from each other.

+++ b/core/modules/block/custom_block/custom_block.moduleundefined
@@ -53,3 +143,168 @@ function theme_custom_block_block($variables) {
+    @list($base, $derivative) = explode(':', $plugin_id);

Why the @? We should try to avoid that now.

+++ b/core/modules/block/custom_block/custom_block.moduleundefined
@@ -53,3 +143,168 @@ function theme_custom_block_block($variables) {
+    $row['1']['data']['#links']['edit'] = array(

That '1' is unfortunate. Maybe the upstream code should use string keys?

+++ b/core/modules/block/custom_block/custom_block.pages.incundefined
@@ -0,0 +1,154 @@
+<?php
+/**
+ * @file

Missing blank line.

+++ b/core/modules/block/custom_block/custom_block.pages.incundefined
@@ -0,0 +1,154 @@
+  if (isset($_GET['theme']) && in_array($_GET['theme'], array_keys(list_themes()))) {
...
+      'query' => array('theme' => $_GET['theme'])
...
+  if (isset($_GET['theme']) && in_array($_GET['theme'], array_keys(list_themes()))) {
...
+    $block->setTheme($_GET['theme']);

Can we use Request here?

+++ b/core/modules/block/custom_block/custom_block.pages.incundefined
@@ -0,0 +1,154 @@
+  if (count($types) == 1) {

This should be if ($types && count($types) == 1) {, because count() returns 1 if its a non-array. Yeah. Seriously.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.phpundefined
@@ -0,0 +1,222 @@
+   * Overrides Drupal\Core\Entity\EntityFormController::prepareEntity().
...
+   * Overrides Drupal\Core\Entity\EntityFormController::form().

Here and elsewhere, missing a slash (\Drupal\...)

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.phpundefined
@@ -0,0 +1,222 @@
+  public function form(array $form, array &$form_state, EntityInterface $block) {
+
+    // Override the default CSS class name, since the user-defined custom block

Extra blank line.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.phpundefined
@@ -0,0 +1,222 @@
+    $language_configuration = module_invoke('language', 'get_default_configuration', 'custom_block', $block->type->value);

Are we allowed to do that? Isn't that the same as just calling the function?

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.phpundefined
@@ -0,0 +1,222 @@
+      '#default_value' => !empty($block->log->value) ? $block->log->value : '',

Can't this just rely on the default NULL value?

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.phpundefined
@@ -0,0 +1,222 @@
+   * Updates the custom block object by processing the submitted values.
+   *
+   * This function can be called by a "Next" button of a wizard to update the
+   * form state's entity with the current step's values before proceeding to the
+   * next step.
+   *
+   * Overrides Drupal\Core\Entity\EntityFormController::submit().

The Overrides goes at the top, with a blank line after it

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.phpundefined
@@ -0,0 +1,222 @@
+    foreach (module_implements('custom_block_submit') as $module) {
+      $function = $module . '_custom_block_submit';
+      $function($block, $form, $form_state);

What's the use case for this? Shouldn't this be covered by hook_ENTITYTYPE_presave/insert/update?

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockStorageController.phpundefined
@@ -0,0 +1,119 @@
+    }
+
+  }

blank line

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockStorageController.phpundefined
@@ -0,0 +1,119 @@
+      'description' => t("The revision log message."),

Unneeded double quotes

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockTypeListController.phpundefined
@@ -0,0 +1,62 @@
+<?php
+/**
+ * @file

Missing blank line

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockTypeListController.phpundefined
@@ -0,0 +1,62 @@
+        'weight' => 11,
...
+        'weight' => 12,

These should be in increments of at least 5

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Core/Entity/CustomBlock.phpundefined
@@ -0,0 +1,202 @@
+   *
+   * @var \Drupal\Core\Entity\Field\FieldInterface
+   */
+  public $id;
...
+   * @var \Drupal\Core\Entity\Field\FieldInterface
+   */
+  public $revision_id;
...
+   * @var \Drupal\Core\Entity\Field\FieldInterface

I guess this is technically correct, but I'm so used to actually having helpful info like string/array/int...

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Core/Entity/CustomBlock.phpundefined
@@ -0,0 +1,202 @@
+  protected function init() {
+    parent::init();
+    // We unset all defined properties, so magic getters apply.
+    unset($this->id);
+    unset($this->info);
+    unset($this->revision_id);
+    unset($this->log);
+    unset($this->uuid);
+    unset($this->type);
+    unset($this->new);

Why not theme?

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Core/Entity/CustomBlock.phpundefined
@@ -0,0 +1,202 @@
+   * Entity uri callback.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Core/Entity/CustomBlockType.phpundefined
@@ -0,0 +1,80 @@
+   * Entity uri callback.

This should be "Overrides \Drupal\...."

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Core/Entity/CustomBlock.phpundefined
@@ -0,0 +1,202 @@
+      'path' => 'block/' . $this->id(),
+      'options' => array()

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Core/Entity/CustomBlockType.phpundefined
@@ -0,0 +1,80 @@
+  public function uri() {
+    return array(
+      'path' => 'admin/structure/custom-blocks/manage/' . $this->id(),
+      'options' => array()

You need to include the entity and entity_type here

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Derivative/CustomBlock.phpundefined
@@ -40,17 +40,14 @@ public function getDerivativeDefinition($derivative_id, array $base_plugin_defin
+    $custom_blocks = entity_load_multiple('custom_block', entity_query('custom_block')->execute());

What? Why not just entity_load_multiple('custom_block'), no $ids passed in?

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/block/block/CustomBlock.phpundefined
@@ -1,6 +1,6 @@
 <?php
-
 /**

Put that line back! :)

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/block/block/CustomBlock.phpundefined
@@ -108,12 +75,18 @@ public function blockSubmit($form, &$form_state) {
+    list(, $uuid) = explode(':', $this->getPluginId());

There is a method for this in DerivativeDiscoveryDecorator.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockCreationTest.phpundefined
@@ -0,0 +1,105 @@
+use Exception;
...
+    catch (Exception $e) {

Don't `use` top level classes, just catch (\Exception $e) { inline

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockCreationTest.phpundefined
@@ -0,0 +1,105 @@
+  public function setUp() {
+    parent::setUp();

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockRevisionsTest.phpundefined
@@ -0,0 +1,92 @@
+  public function setUp() {

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockSaveTest.phpundefined
@@ -0,0 +1,109 @@
+  public function setUp() {
+    parent::setUp();
+    $this->drupalLogin($this->adminUser);

protected function setUp(), and put a blank after the parent:: call please

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockRevisionsTest.phpundefined
@@ -0,0 +1,92 @@
+class CustomBlockRevisionsTest extends CustomBlockTestBase {
+  protected $blocks;
+  protected $logs;

Missing blank line and docblocks

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockRevisionsTest.phpundefined
@@ -0,0 +1,92 @@
+    $this->assertTrue($loaded->revision_id->value > $default_revision->revision_id->value, 'Revision id is greater than default revision id.');
+  }
+}
diff --git a/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockSaveTest.php b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockSaveTest.php

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockSaveTest.phpundefined
@@ -0,0 +1,109 @@
+    $this->assertEqual($block->label(), 'CustomBlock ' . $block->id->value, 'Custom block saved on block insert.');
+  }
+}
diff --git a/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockTestBase.php b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockTestBase.php

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/PageEditTest.phpundefined
@@ -0,0 +1,77 @@
+    $this->assertNotIdentical($block->revision_id->value, $revised_block->revision_id->value, 'A new revision has been created.');
+  }
+}
diff --git a/core/modules/block/custom_block/tests/modules/custom_block_test/custom_block_test.info b/core/modules/block/custom_block/tests/modules/custom_block_test/custom_block_test.info

Missing blank line before end of class.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/PageEditTest.phpundefined
@@ -0,0 +1,77 @@
+   * Sets the test up.
+   */
+  public function setUp() {
+    parent::setUp();
+  }

Remove this :)

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.phpundefined
@@ -144,35 +148,42 @@ public function testCustomBlock() {
+      "block_body[$langcode][0][format]" => 'full_html'

Missing trailing comma

+++ b/core/themes/seven/template.phpundefined
@@ -63,6 +63,27 @@ function seven_node_add_list($variables) {
+ * Displays the list of available custom block types for creation.

This should state that its overriding a theme function.

larowlan’s picture

Also can _wdm_ get a commit mention here as he helped with this during the code sprint in Sydney.

This fixes all issues identified by @timplunkett except:

There is a method for this in DerivativeDiscoveryDecorator.

I don't have that in scope, $this->discovery is an annotation based discovery, ie I can't get the derivative decorator in that scope.
I added a todo for that (for now) for when #1874498: Provide and use API methods for retrieving base plugin and derivative names from block plugin IDs lands.

larowlan’s picture

Also re

Are we allowed to do that? Isn't that the same as just calling the function?

That seems to be the way other entities are doing it.
I guess its simpler than

if (module_exists('language')) {
  ....
}
larowlan’s picture

Back to green!

plach’s picture

@larowlan:

That seems to be the way other entities are doing it.

This is ok for now, altough it's ugly: we shouldn't require entity-defining modules to repeat this code every time. I think #1834542: Implement hook_entity_create() to setup the default entity language should be the proper solution. Moreover when every entity will be converted to NG we will be able to automatically define the language_select form item in the EntityFormController::form() method based on field definitions, as we do now for configurable fields in field_attach_form(). See #1728780: Build and prepare a basic entity form through the Entity Property API.

tkoleary’s picture

@larowlan

Ok I just tested this. I believe it functions as designed but it is definitely a usability fail. Problem is I can't tell whether the fail is in this feature or in blocks as plugins.

Let me just outline it and maybe you can enlighten me.

So lets start with "As a user I want to create a custom block and place it on a page in my site." + "As a user who has created a custom block, when I return later I want to be able to edit the content in the custom block"

So As usual I go to structure, blocks, add block, then I see "add custom block" all pretty good so far. I load "add custom block",

Problem #1: why does it not load in the overlay? This is strange to the user.

Continuing I create a title (bananaphone) and some content and save the block.

Problem #2: because I am not in the overlay when I save the block I am not returned to the blocks UI, I have to navigate back there.

Continuing to blocks UI I browse through the blocks.

Problem #3: where is my custom block?

So I guess that maybe I have to go back to the block library to find it. So I do that. Once there I see the block and it has a drop button.

Problem #4: I mus click "add block" to go to the block library when what I want to do is edit/configure not add.

Problem #5: The thing I want to do, add my block to the blocks UI, is not an option in the drop button.

I look down the list to find my block

Problem # 6: Even though the field I used when I created the block was labeled "title" the culumn that the titles are in is now labebed "Subject" (?!)

So I find my block and I guess that maybe if I configure it I can get it to appear in the blocks UI. I do that and I see "region". I select sidebar first and click save. Now I am back in blocks UI and I look for my block.

Problem #7: The column where I see (what I thought was) the title of my block is labeled "block" (?!!)

I find my block in sidebar first, good.

Now I return at a later point and I decide I want to change the body of my block. I go to structure/blocks and look for my custom block with the title bananaphone. I find the block.

Problem #8: The dropbutton does not have an option to "edit" (?!!!)

I figure maybe if I go to a site page and check the contextual links I might find the edit option there. I do that and it is there. I click edit and I an able to change the body of my block but I decide that I want to also edit the title of my block, but I don't see title, only "description" which is "bananaphone"

Problem #9: Why did the title automatically become the description?

Problem #10: edit does not show me the title field even though the users expectation is that the title is part of the content and I do not know that if I change the description it will also change the title.

Gábor Hojtsy’s picture

@tkoleary: indeed, most of the issue you found are either issues existing in the Drupal 8 blocks UI or with integrations with this new feature and the blocks UI. I think what's critical to understand here is this is a huge patch that would be good to have to land sooner than later and the usability issues might be easier to rework afterwards, since people don't need to juggle with a 120k+ patch in a 100+ comment issue :)

tkoleary’s picture

@Gabor

Noted. I'll work on designing fixes.

larowlan’s picture

Hi @tkoleary, thanks for taking the time to review in this length responses as follows

Problem #1: why does it not load in the overlay? This is strange to the user.
hmm, I have hook_admin_paths defining block/add, block/add/* but because we get their via a RedirectResponse, this must not respect overlay. So that is beyond this patch, will try and find if we have an existing issue for that. If I trick the url to /#overlay=block/add%3Ftheme%3Dbartik it works.

Problem #2: because I am not in the overlay when I save the block I am not returned to the blocks UI, I have to navigate back there.
Yeah if I get to /#overlay=block/add%3Ftheme%3Dbartik it works from there, so same issue here, RedirectResponse doesn't play nice with overlay.

Problem #3: where is my custom block?
Problem #4: I mus click "add block" to go to the block library when what I want to do is edit/configure not add.
So these are solved by 2.

Problem #5: The thing I want to do, add my block to the blocks UI, is not an option in the drop button.
This is an existing issue since blocks as plugins

Problem # 6: Even though the field I used when I created the block was labeled "title" the culumn that the titles are in is now labebed "Subject" (?!)
Problem #7: The column where I see (what I thought was) the title of my block is labeled "block" (?!!)
These are existing too, we need new issues to resolve these, they are great novice tasks.

Problem #8: The dropbutton does not have an option to "edit" (?!!!)
I played around this morning with trying to add it. It is available to edit in the block library, but the plugin info is not available in this form so I can't add the button. I think this is a good follow-up candidate, it requires changes to that form. Correct me if I'm wrong though, I think the block page will go eventually anyway? In favour of layouts and master layouts?

Problem #9: Why did the title automatically become the description?
Problem #10: edit does not show me the title field even though the users expectation is that the title is part of the content and I do not know that if I change the description it will also change the title.

Yes, this is the two entity forms issue. One entity (the block instance - the configure link) contains the title. The title is per instance and can be overridden. The other entity (the edit link) is the actual block entity - where the other fields are found. Again I think moving to layouts/master layouts will remove some of this confusion, as the title will be part of the layout.

So I think we have the following 'follow-ups'. Some of which may already exist:
*Make RedirectResponse respect overlay.
*Unify use of subject/label/title in block ui
*If we don't end up with layouts/master layouts - make plugin info available in form data at admin/structure/block so other modules can extend operations.

Thanks again!

hass’s picture

larowlan’s picture

Re-roll against head.
Also, putting my hand up to maintain custom_block.module

EclipseGc’s picture

Problem #1: why does it not load in the overlay? This is strange to the user.
hmm, I have hook_admin_paths defining block/add, block/add/* but because we get their via a RedirectResponse, this must not respect overlay. So that is beyond this patch, will try and find if we have an existing issue for that. If I trick the url to /#overlay=block/add%3Ftheme%3Dbartik it works.

This is because of the use of all the redirects. This is unnecessary. I did 2 simple things to change this:


/**
 * Implements hook_menu_local_task_alter().
 */
function custom_block_menu_local_tasks_alter(&$data, $router_item, $root_path) {
  // Make sure we're at least in the right general area.
  if (substr($root_path, 0, 26) == 'admin/structure/block/list') {
    // This is the path we want to match.
    $admin_path = array(
      'admin',
      'structure',
      'block',
      'list',
      'add',
    );
    // We're going to want an array so we can remove the plugin id.
    $path_map = explode('/', $root_path);
    $before = array_slice($path_map, 0, 4);
    $after = array_slice($path_map, 5);
    $path_map = array_merge($before, $after);
    // If the path is absolutely equal to $admin_path, we have a winner.
    if($path_map === $admin_path) {
      $item = menu_get_item('block/add');
      if ($item['access']) {
        $data['actions']['output'][] = array(
          '#theme' => 'menu_local_action',
          '#link' => $item,
        );
      }
    }
  }
}

Normally I hate this hook, but with the router changing completely in core, it has its uses and this is one of those instances. The intro logic is fun since I array_slice paths and stuff but it totally works. This removes the first of two redirects you have. The second is of course on block/add itself where you redirect when there is only one valid custom block type, and switching that to this fixes that as well:


function custom_block_add_page() {
  $options = array();
  $request = drupal_container()->get('request');
  if (($theme = $request->attributes->get('theme')) && in_array($theme, array_keys(list_themes()))) {
    // We have navigated to this page from the block library and will keep track
    // of the theme for redirecting the user to the configuration page for the
    // newly created block in the given theme.
    $options = array(
      'query' => array('theme' => $theme)
    );
  }
  $types = entity_load_multiple('custom_block_type');
  if ($types && count($types) == 1) {
    $type = reset($types);
    return custom_block_add($type);
  }

  return array('#theme' => 'custom_block_add_list', '#content' => $types);
}

Note that my first snippet of code allows you to remove the menu items that were doing the initial redirecting (that whole drupal_container() foreach loop can go).

I'm testing the patch now, this was the first thing I noticed and it was annoying enough I just fixed it. Will keep posting stuff all night I'm sure.

Eclipse

EclipseGc’s picture

Problem #2: because I am not in the overlay when I save the block I am not returned to the blocks UI, I have to navigate back there.
Yeah if I get to /#overlay=block/add%3Ftheme%3Dbartik it works from there, so same issue here, RedirectResponse doesn't play nice with overlay.

Yeah I think removing the redirects fixed this. I definitely ended up back in the blocks UI after adding a custom block.

Eclipse

hass’s picture

#101 I'm not really sure, but it may not work if you customize your system menu as the path is changing. Than the (substr($root_path, 0, 26) == 'admin/structure/block/list') may not match.

EclipseGc’s picture

FileSize
6.43 KB
117.63 KB

My version of this patch. I've not done a detailed review of the code, but enough other have that I'm pretty happy with the actual UI at this point. It's not perfect, but then it's building on an imperfect system and it delivers on all the things I wanted functionality wise. I'm very happy with this at this point.

Eclipse

EclipseGc’s picture

Oh, I also cleaned up some whitespace issues. Maybe git will stop complaining now.

EclipseGc’s picture

@hass

That's not any less true with real menu items.

Eclipse

tkoleary’s picture

@larowlan

Thanks for the detailed reply. Looking much better already.

Correct me if I'm wrong though, I think the block page will go eventually anyway? In favour of layouts and master layouts?

True, but there should be an edit link there as well.

sun’s picture

Status: Needs review » Reviewed & tested by the community

The most important aspect here is this:

This introduces the SECOND entity in core that has REVISIONS! :)

That's so very utterly badly needed. Really. We've to standardize, clean up, and also abstract so many things in that space that this very argument alone should require us to commit this as soon as possible.

Reality is, the smaller details about the user interface and interaction, and coding patterns and whatnot, can still be happily adjusted in the next 12-24 months. In the grand scheme of things, those absolutely are secondary.

Here's my review: Pretty much all of the issues I'm raising can be addressed in follow-up issues; ideally we want to create them ahead of time.

+++ b/core/modules/block/block.install
@@ -194,6 +195,246 @@ function block_update_8006() {
+  db_create_table('custom_block', array(
...
+  db_create_table('custom_block_revision', array(

Normally we handle this case via hook_schema_0() implementations + update_module_enable(), but we can clean that up later.

+++ b/core/modules/block/custom_block/custom_block.admin.inc
@@ -0,0 +1,98 @@
+function custom_block_type_edit(CustomBlockType $block_type) {
+  return entity_get_form($block_type);

Normally we add drupal_set_title('Edit %label block type', ...) here, but we can improve this later.

(page title != menu link title)

+++ b/core/modules/block/custom_block/custom_block.install
@@ -33,17 +32,80 @@ function custom_block_schema() {
+      // Defaults to NULL in order to avoid a brief period of potential
+      // deadlocks on the index.
+      'revision_id' => array(

Is this copied from node_schema()? Is it actually true?

+++ b/core/modules/block/custom_block/custom_block.install
@@ -33,17 +32,80 @@ function custom_block_schema() {
+    'indexes' => array(
+      'block_custom_type'   => array(array('type', 4)),

Limited to 4 chars - are you sure? :)

+++ b/core/modules/block/custom_block/custom_block.module
@@ -5,29 +5,129 @@
+function custom_block_menu_local_tasks_alter(&$data, $router_item, $root_path) {
...
+  if (substr($root_path, 0, 26) == 'admin/structure/block/list') {
...
+    $path_map = explode('/', $root_path);

I just left some remarks in IRC that this implementation of hook_menu_local_tasks_alter() is one of the most complex I've ever seen and that I'm absolutely confident that it can be vastly simplified.

But that's straight refactoring/simplification, so can happen later. :)

That said, in case #1864066: Simplify hook_menu_local_tasks() and MENU_DEFAULT_LOCAL_TASK usage will land first, this hook needs to be adjusted to be not an alter hook, since it adds a local task (instead of altering an existing).

+++ b/core/modules/block/custom_block/custom_block.module
@@ -53,3 +157,161 @@ function theme_custom_block_block($variables) {
+function custom_block_add_body_field($block_type_id, $label = 'Block body') {

Minor: Should default to just "Body", I think.

+++ b/core/modules/block/custom_block/custom_block.module
@@ -53,3 +157,161 @@ function theme_custom_block_block($variables) {
+  $field = field_info_field('block_body');
+  $instance = field_info_instance('custom_block', 'block_body', $block_type_id);
...
+      'field_name' => 'block_body',

I'm concerned about module namespace issues here; technically, this module-specific field should at least start with "custom_block_" as it is owned by custom_block.module.

Core can make exceptions, of course (just like the "body" field on nodes and comments), but with D8+, we're nearing a phase of maturity in which such "hacks" are less easy to explain.

+++ b/core/modules/block/custom_block/custom_block.module
@@ -53,3 +157,161 @@ function theme_custom_block_block($variables) {
+function custom_block_block_view_alter(array &$build, Block $block) {
+  // Add contextual links for custom blocks.

For adding stuff to block views, we should have a _view hook that is not a _view_alter hook. I don't know whether that exists or not, but in case it does not, we need one (and this one should be converted). Potential follow-up issue.

+++ b/core/modules/block/custom_block/custom_block.pages.inc
@@ -0,0 +1,153 @@
+function custom_block_add_page() {
...
+  $request = drupal_container()->get('request');
+  if (($theme = $request->attributes->get('theme')) && in_array($theme, array_keys(list_themes()))) {
+    // We have navigated to this page from the block library and will keep track
+    // of the theme for redirecting the user to the configuration page for the
+    // newly created block in the given theme.
+    $options = array(
+      'query' => array('theme' => $theme)
+    );
+  }
...
+function custom_block_add(CustomBlockType $block_type) {
...
+  $request = drupal_container()->get('request');
+  if (($theme = $request->attributes->get('theme')) && in_array($theme, array_keys(list_themes()))) {
+    // We have navigated to this page from the block library and will keep track
+    // of the theme for redirecting the user to the configuration page for the
+    // newly created block in the given theme.
+    $block->setTheme($theme);
+  }
+  return entity_get_form($block);

This definitely needs a follow-up issue to improve the DX/API situation.

My prediction is that there will be many more custom_block-alike modules in contrib in D8, and we really do not want them to have to repeat this request-futzing weirdness. :)

+++ b/core/modules/block/custom_block/custom_block.pages.inc
@@ -0,0 +1,153 @@
+  return array('#theme' => 'custom_block_add_list', '#content' => $types);
...
+function theme_custom_block_add_list($variables) {
...
+    $output = '<dl class="node-type-list">';

I... hrm. We have (minor) wrong "node" class name in there, but I don't really get why the /add callback optionally renders a list of (existing?) custom blocks that may be added - shouldn't that be a different controller/callback?

In any case, do we really need a theme function for that? Can't we make it a simple table or something? :)

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockAccessController.php
@@ -0,0 +1,47 @@
+  public function viewAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {
+    return user_access('view content', $account);

Hm. That user permission belongs to Node module.

We definitely need a (major) follow-up issue for that.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
@@ -0,0 +1,216 @@
+    // Override the default CSS class name, since the user-defined custom block
+    // type name in 'TYPE-block-form' potentially clashes with third-party class
+    // names.
+    $form['#attributes']['class'][0] = drupal_html_class('block-' . $block->type->value . '-form');

The BUNDLE_TYPE_form pattern is a massive anti-pattern from Node module in the first place.

Let's create a (major) follow-up issue to change that form ID pattern to prevent such namespace conflicts (for all entity types).

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
@@ -0,0 +1,216 @@
+    $language_configuration = module_invoke('language', 'get_default_configuration', 'custom_block', $block->type->value);

Hm. Not sure whether this is the correct Language API we're supposed to use?

Off-hand, I can't tell how this works for other entity types though, so perhaps this is actually right...

In any case, I think a follow-up issue to clean up the "API" would be helpful for D8.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
@@ -0,0 +1,216 @@
+    $form['additional_settings'] = array(
+      '#type' => 'vertical_tabs',

Our new standard is to call the vertical tabs container 'advanced' instead of additional_settings. Let's adjust that in a follow-up.

Dries’s picture

I just committed #1864066: Simplify hook_menu_local_tasks() and MENU_DEFAULT_LOCAL_TASK usage. This may require this patch to be re-rolled per sun's review in #108.

I like the follow-up issues in #109 but I don't see any for the UX issues that have been pointed out in the comments above. I saw a couple of issues related to the UX in the issue summary but maybe there are more.

Can someone update the issue summary with all the follow-ups from #109?

Dries’s picture

I tried this patch. Here is what I found.

custom-block-types-or-block-types.jpg

adding-a-new-block.jpg

So I created a new block type called 'Quote', using the newly added datetime module (yay!). Here is what it looks likes (doesn't really matter for the rest of this comment):

quote-block-type.jpg

But when I want to create a new instance of my quote block, I couldn't figure out where to go. Eventually I figured it out, but it wasn't intuitive at all.

add-custom-block.jpg

add-custom-block-2.jpg

(I was going to review more of this but simplytest.me expired my environment.)

Dries’s picture

I haven't looked at the code yet, but I like where this is going from a feature perspective. As pointed out by many (including myself in #111), this patch has significant UX issues. We've held up, and are actively holding up, other patches for much smaller UX issues. I've also seen very little feedback on architecture, upgrade path or even performance. That said, I plan to review this patch's code tonight with the aim to commit it. It would be great if we could fix some of the UX issues today but I understand that may have to happen after feature freeze.

hass’s picture

We also need to followup with the buttons implemented in #1751606: Move published status checkbox next to "Save"

xjm’s picture

We have #1875252: [META] Make the block plugin UI shippable for the UX regressions in the Block UI, so let's make sure anything from Dries' feedback that doesn't get fixed here has a followup under that meta.

Dries’s picture

Re @xjm in #114: I just cross-posted my review to #1875252: [META] Make the block plugin UI shippable. Given that #1875252 is marked as "critical", I'm comfortable proceeding with this patch. I'll look at the code now.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Committed to 8.x. Moving to 'needs work', as it needs work based on recent reviews.

Dries’s picture

Issue summary: View changes

updated remaining tasks

EclipseGc’s picture

Status: Needs work » Fixed

I think we can probably do the necessary work in the related issues in the issue summary as follow up to this one. Most of the problems specced here are issues with the interface as a whole and not specific to custom blocks.

Eclipse

sun’s picture

Before we're going to add a issue component to the d.o Drupal project:
#1920862: Rename custom_block.module to block_content.module

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

It is surprising a change notice was not demanded for this issue. I've added one at http://drupal.org/node/1933768.

Gábor Hojtsy’s picture

Issue summary: View changes

adding reference to #1875252

larowlan’s picture

Issue summary: View changes

Updated issue summary.