One of the things we did in #1927608: Remove the tight coupling between Block Plugins and Block Entities was shift the creation of a default render array (and the invocation of an alter on that render array) onto BlockBase::build(). This was not the greatest thing.

  • It places responsibility for dictating how the presentation layer should receive the block into the hands of the block itself. Block plugins should be concerned only with producing a) a title, b) content, and hopefully soon c) assets. They should not concern themselves with how that information is glued together and provided to the frontend. We have a BlockPluginInterface specifically so that external systems can make such decisions.
  • By placing the alter inside the build method, we make it possible for blocks to individually decide whether they are alterable or not. Much though I hate that particular alter, if we're going to have it, then it needs to be invoked reliably. That means pushing the decision up to a higher layer where it can be swapped out wholesale, and is either on or off for all blocks, equally.
  • If someone wants to, say, create an administrative interface in which one "renders" blocks for some alternate purpose - say, putting in dummy content in order to populate a demonstration interface - then it's easy to do by simply swapping in a different rendering controller and creating a different default render array. If the logic remains baked on the block itself, though, then we either have to ignore that method entirely, or throw out and rewrite most the render array each time...and it's hard to do that really reliably, since a particular block plugin might override BlockBase::build() and do a whole buncha wacky crap with the render array it returns.

So, this patch shifts that responsibility back out onto the BlockRenderController. It also gets rid of the blockBuild() method, and all block plugins now just directly implement build().

Note: this also makes princess happy.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sdboyer’s picture

Status: Active » Needs review
FileSize
19.9 KB

and here's the patch.

Status: Needs review » Needs work

The last submitted patch, re-empower-the-brc.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -224,33 +224,4 @@ public function submit($form, &$form_state) {
-    drupal_alter(array('block_view', "block_view_$base_id"), $build, $this);

+++ b/core/modules/block/lib/Drupal/block/BlockRenderController.phpundefined
@@ -36,7 +36,20 @@ public function view(EntityInterface $entity, $view_mode = 'full', $langcode = N
+      drupal_alter(array('block_view', "block_view_$base_id"), $build, $this);

The $this is going to be different. So the API docs need to be updated, and I'm going to guess menu_block_view_system_menu_block_alter() will cause errors here.

I'm pretty tired of moving this back and forth, but I did like it here most, and if it helps SCOTCH I'm +1.

sdboyer’s picture

oh whoops, forgot to switch that reference to $this...think i caught all the others. i don't see why menu_block_view_system_menu_block_alter() would be affected - it should look exactly the same to that code.

which API docs? anything referencing blockBuild()?

yeah, we've yanked this stuff around a lot...sorry to do it again. i'm really hoping we can keep this one contained to this issue, uncontroversial, and just get it through.

Status: Needs review » Needs work

The last submitted patch, re-empower-the-brc-2014215-4.patch, failed testing.

tim.plunkett’s picture

Sorry, I meant if you WANTED to keep $this, to make it pass the entity, then you'd need to change the typehints and docs. Just switching the variable is easier :)

sdboyer’s picture

Status: Needs work » Needs review
FileSize
1.13 KB
19.93 KB

maybe the bot will hate this less.

Status: Needs review » Needs work

The last submitted patch, re-empower-the-brc-2014215-9.patch, failed testing.

sdboyer’s picture

well i got one failure, but the other six are baffling me. they seem to all be symptomatic of the same basic problem, that an id/title something isn't being set in quite the same way. i'll keep trying if i don't see movement here in a couple days, but someone with a bit more insight into this system would maybe have more of a clue.

sdboyer’s picture

Status: Needs work » Needs review
FileSize
1.18 KB

blah, double-uploaded the main patch and not the interdiff. whoops.

sdboyer’s picture

OH. i bet it's because things like views mutate the configuration during build()...so, have to get config after it's been called.

Status: Needs review » Needs work

The last submitted patch, re-empower-the-brc-2014215-13.patch, failed testing.

sdboyer’s picture

Status: Needs work » Needs review
FileSize
758 bytes
21.12 KB

durrr, dur dur dur DURRRRRR

Status: Needs review » Needs work

The last submitted patch, re-empower-the-brc-2014215-15.patch, failed testing.

sdboyer’s picture

Status: Needs work » Needs review
FileSize
21.18 KB
728 bytes

wow, so, there's an *astounding* number of places where we alter an empty array, it seems. i'm gonna open a separate issue about not altering an empty array...where can can argue about it, as i already know timplunkett and i disagree.

regardless, this should fix it for now.

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

Discussed at length with sdboyer, I'm on board.

RTBC

Eclipse

alexpott’s picture

Title: Shift render array defaults back out onto BlockRenderController » Change notice: Shift render array defaults back out onto BlockRenderController
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed fd9804d and pushed to 8.x. Thanks!

Need to ensure that the change record https://drupal.org/node/1880620 is correct...

Also this needing fixing on commit...

/**
 * Render API callback: Lists nodes based on the element's #query property.
 *
 * This function can be used as a #pre_render callback.
 *
 * @see \Drupal\forum\Plugin\block\block\NewTopicsBlock::blockBuild()
 * @see \Drupal\forum\Plugin\block\block\ActiveTopicsBlock::blockBuild()
 */
function forum_block_view_pre_render($elements) {
catch’s picture

xjm’s picture

Issue tags: +Missing change record

Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release.

The patch for this issue was committed on June 13, 2013, meaning the change record for this issue has been missing for more than seven months.

chx’s picture

Status: Active » Fixed
Issue tags: -Needs change record, -Missing change record

I am not even sure originally this needed a change notice but now it surely doesn't need one; the build method is explained in the BAP change notice.

chx’s picture

Title: Change notice: Shift render array defaults back out onto BlockRenderController » Shift render array defaults back out onto BlockRenderController
Priority: Major » Normal
Status: Fixed » Active
Wim Leers’s picture

Status: Active » Closed (fixed)