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.

Files: 
CommentFileSizeAuthor
#17 interdiff.txt728 bytessdboyer
#17 re-empower-the-brc-2014215-17.patch21.18 KBsdboyer
PASSED: [[SimpleTest]]: [MySQL] 55,044 pass(es).
[ View ]
#15 re-empower-the-brc-2014215-15.patch21.12 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] 55,431 pass(es), 9 fail(s), and 177 exception(s).
[ View ]
#15 interdiff.txt758 bytessdboyer
#13 re-empower-the-brc-2014215-13.patch21.11 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] 55,050 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#13 interdiff.txt780 bytessdboyer
#12 interdiff.txt1.18 KBsdboyer
#11 re-empower-the-brc-2014215-11.patch21.11 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#11 re-empower-the-brc-2014215-11.patch21.11 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] 55,613 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#9 re-empower-the-brc-2014215-9.patch19.93 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] 56,001 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#9 interdiff.txt1.13 KBsdboyer
#4 re-empower-the-brc-2014215-4.patch19.9 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#4 interdiff.txt744 bytessdboyer
#1 re-empower-the-brc.patch19.9 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new19.9 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

and here's the patch.

Status:Needs review» Needs work

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

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.

StatusFileSize
new744 bytes
new19.9 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

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.

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 :)

Status:Needs work» Needs review
StatusFileSize
new1.13 KB
new19.93 KB
FAILED: [[SimpleTest]]: [MySQL] 56,001 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

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.

StatusFileSize
new21.11 KB
FAILED: [[SimpleTest]]: [MySQL] 55,613 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
new21.11 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new1.18 KB

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

StatusFileSize
new780 bytes
new21.11 KB
FAILED: [[SimpleTest]]: [MySQL] 55,050 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new758 bytes
new21.12 KB
FAILED: [[SimpleTest]]: [MySQL] 55,431 pass(es), 9 fail(s), and 177 exception(s).
[ View ]

durrr, dur dur dur DURRRRRR

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new21.18 KB
PASSED: [[SimpleTest]]: [MySQL] 55,044 pass(es).
[ View ]
new728 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.

Status:Needs review» Reviewed & tested by the community

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

RTBC

Eclipse

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) {

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.

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.

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