Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#17 | interdiff.txt | 728 bytes | sdboyer |
#17 | re-empower-the-brc-2014215-17.patch | 21.18 KB | sdboyer |
#15 | re-empower-the-brc-2014215-15.patch | 21.12 KB | sdboyer |
#15 | interdiff.txt | 758 bytes | sdboyer |
#13 | re-empower-the-brc-2014215-13.patch | 21.11 KB | sdboyer |
Comments
Comment #1
sdboyer CreditAttribution: sdboyer commentedand here's the patch.
Comment #3
tim.plunkettThe
$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.
Comment #4
sdboyer CreditAttribution: sdboyer commentedoh 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.
Comment #8
tim.plunkettSorry, 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 :)
Comment #9
sdboyer CreditAttribution: sdboyer commentedmaybe the bot will hate this less.
Comment #11
sdboyer CreditAttribution: sdboyer commentedwell 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.
Comment #12
sdboyer CreditAttribution: sdboyer commentedblah, double-uploaded the main patch and not the interdiff. whoops.
Comment #13
sdboyer CreditAttribution: sdboyer commentedOH. i bet it's because things like views mutate the configuration during
build()
...so, have to get config after it's been called.Comment #15
sdboyer CreditAttribution: sdboyer commenteddurrr, dur dur dur DURRRRRR
Comment #17
sdboyer CreditAttribution: sdboyer commentedwow, 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.
Comment #18
EclipseGc CreditAttribution: EclipseGc commentedDiscussed at length with sdboyer, I'm on board.
RTBC
Eclipse
Comment #19
alexpottCommitted 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...
Comment #20
catch#2083415: [META] Write up all outstanding change notices before release
Comment #21
xjmTagging "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.
Comment #22
chx CreditAttribution: chx commentedI 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.
Comment #23
chx CreditAttribution: chx commentedComment #24
Wim Leers