Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Derived from #1183042: Regression: Add WAI-ARIA roles to Core blocks, allow module maintainers to inject attributes and classes into blocks by adding "attributes" to hook_block_view(). system_preprocess_block() is a good example of why this is a good thing as it saves us having to write another preprocess function.
Comment | File | Size | Author |
---|---|---|---|
#9 | hook_block_view-attributes.patch | 7.24 KB | michaelfavia |
hook_block_view-attributes.patch | 7.16 KB | RobLoach | |
Comments
Comment #1
RobLoachRight now, if you have a module where you're creating a block where you need a specific block class, you need to use:
It makes infinitely more sense to use:
Comment #2
JacineThanks Rob!
So, my question is can this be done for nodes too? And anything else?
Comment #3
Jeff Burnz CreditAttribution: Jeff Burnz commentedSubscribe :)
Comment #4
RobLoachI was thinking about this last night as @Jacine does make a good point. The key difference between Blocks and Nodes, however, is that Blocks are not Entities. Blocks are containers for content on your site. They hold content, doesn't matter what it is, they just display it. This means we either:
...... or am I just crazy?
Comment #5
JacineNot crazy at all. I thought blocks were going to turn into entities though? Anyway, blocks should definitely become render arrays. I think the only reason they weren't switched over in D7 was because there wasn't time. It's totally weird that they are not right now.
The main thing here that I am concerned about is that ALL attributes are done the same way for consistency. I still worry about being able to manipulate them fully in preprocess functions. I have run into some funky situations in D7 where pre render functions destroy the theme's ability to make preprocess/preprocess changes. Though, if anyone screws around with attributes in that way, I guess that would be pretty dumb.
Comment #6
Everett Zufelt CreditAttribution: Everett Zufelt commentedIf we made blocks into renderable elements (which i am in support of), we could do:
#type = block
#attributes][class][] = my-block-custom-class
#title = My block title // This is currently 'subject'
#title_display = invisible // If you want to hide the title
... // the rest of the content of the block.
Comment #7
indytechcook CreditAttribution: indytechcook commentedCross post with #430886: Make all blocks fieldable entities
Blocks are going toward entities. The block rendering should be a render array. The other issue is now focused on making the custom block entities. If we end up with all blocks as entities then blocks will return render arrays.
@Rob Loach that statement isn't true. $block['content'] expects a render array. see http://api.drupal.org/api/drupal/modules--block--block.module/function/_...
Comment #8
xjmThanks for your work on this patch. Note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."
Tagging as novice for the task of rerolling the Drupal 8.x patch.
If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.
Comment #9
michaelfavia CreditAttribution: michaelfavia commentedRerolled post core move.
Comment #10
JohnAlbinI'd like to point out that block contents are already renderable arrays. If you have a module that is producing a block and its contents aren't a render array, that's arguably a bug.
That still doesn't solve the problem of wanting to add attributes to the block.tpl wrapper.
Still, I think converting blocks to entities sounds more interesting. And would likely mean any work here would need to be undone or re-done.
Comment #11
Jacine@JohnAlbin So why aren't we using render() for $content in block.tpl.php? Did this just happen in D8 or is it this way in D7 too?
Should we just postpone this for now?
Comment #12
moshe weitzman CreditAttribution: moshe weitzman commentedIt is correct that the preferred return from hook_block_view is a render array in D7. We didn't convert all blocks because it didn't seem that important for the simpler ones, and it takes time to push those patches in.
I think block.tpl.php does not use render() becasause it is a called as a theme_wrapper, not by theme(). So it gets handed already rendered $content and not a render array. All it does is "wrap" that content in the usual block markup.
So this issue seems 'works as designed' or needs to focus more on converting legacy blocks.
Comment #13
catchSo I haven't tested this - if a block returns a render array, can it add affect attributes in block.tpl.php or not currently?
Comment #14
catchI'm going to commit #1183042: Regression: Add WAI-ARIA roles to Core blocks. Using preprocess is not ideal at all, but hook_preprocess_node() shows that's the current standard. If everything works already, then we can use this issue to track converting older code that's relying on preprocess to use it.
Re-titling and bumping to major.
Comment #15
Eric_A CreditAttribution: Eric_A commentedif a block returns a render array, can it add affect attributes in block.tpl.php or not currently?
Not really.
A quick fix might be to change $build in _block_get_renderable_array() to basically stuff everything except the theme wrapper in a child element, and then have the function provide a build alter to add our #attributes or #attributes_array to the parent. (#attributes would need to be preprocessed.)
Comment #16
Eric_A CreditAttribution: Eric_A commentedOops, _block_get_renderable_array() is D7.
In D8 there already is a child element for content and thus a parent element to have its very own attributes. So why not use #theme now instead of #theme_wrappers and convert the block preprocessors?
Currently you would still have to rely on page altering to add attributes to a main block element.
Comment #17
Eric_A CreditAttribution: Eric_A commentedI guess it's alright to pass #attributes next to #children to a theme wrapper, so no urgent need to switch to #theme. (See template_preprocess_region() for the #children and #region variant.)
So if it were alright to have hook_block_view() return a renderable block with a 'content' child, we could add both "content attributes" and "wrapper attributes" there. Then block_get_renderable_region() would add the final theme wrapper and template_preprocess_block() would populate $attributes_array.
Such a patch would touch every implementation of hook_block_view() and hook_block_view_alter(), but would allow for early initialization of WAI-ARIA role attributes.
Comment #18
David_Rothstein CreditAttribution: David_Rothstein commentedI created #1402250: Allow modules to classify blocks when defining them, so WAI-ARIA roles can automatically be added in the theme layer to follow up on the WAI-ARIA roles.
It may or may not wind up relying on this issue (I personally prefer the implementation that doesn't), but either way I think this issue is a good one to solve :) Having to use a preprocess function to add an attribute is definitely annoying.
Comment #19
mgifford#9: hook_block_view-attributes.patch queued for re-testing.
Comment #21
tim.plunkettThis is almost exclusively about blocks, and #1535868: Convert all blocks into plugins will drastically change all of this, possibly even rendering it obsolete, so postponing.
Comment #22
effulgentsia CreditAttribution: effulgentsia commentedUnpostponing. I also re-opened #1183042: Regression: Add WAI-ARIA roles to Core blocks to specifically track the regression of the role attribute, though maybe this issue will fix that.
Comment #23
sunClosely related: #1880588: Regression: invalid HTML in menu blocks due to _block_get_renderable_block() clobbering #theme_wrappers of block contents
Comment #24
tim.plunkettThis should be more than possible now.
BlockPluginInterface::build() (effectively the replacement for hook_block_view) now requires returning a render array.
Comment #27
tim.plunkettNot actively part of the Blocks-Layouts work.