Blocks system refactoring
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | block.module |
| Category: | task |
| Priority: | critical |
| Assigned: | dropcube |
| Status: | patch (code needs work) |
Description
Currently, the way blocks are handled using the hook_block and the different $op values has major weaknesses and writing a switch statement to parse out which $op is everytime causes hard to maintain and inconsistent code.
I propose a refactoring of the block system and block module to allow a more consistent API for blocks that give us more performance, usability improvements and consistency.
Proposition
hook_blocks
Introduce a new hook_blocks, similar to hook_menu that allows modules to declare the blocks they provide through an associative array. They keys are the block deltas and the values are block definitions. Each block definition is again an associative array that may contains the following key-value pairs:
'info' (required): The human-readable name of the block
'description' (required): description of what the block display. It will allow us to provide more descriptive info about the block and avoid that users have to enable the block to see what it does. Currently there is not a consistent way of provide such additional description about each block.
'cache': A bitmask of flags describing how the block should behave with respect to block caching. Just like it is being used now.
'weight', 'status', 'region', 'visibility', 'pages': others key that may be declared for each block.
The intention of the hook_blocks is only define the blocks provided by the module, like hook_menu is.
For example, for the user module we may have a hook_blocks like this:
<?php
**
* Implementation of hook_block().
*/
function user_blocks() {
$blocks['login'] = array(
'info' => t('User login'),
'description' => t('Provides the user login box'),
'cache' => BLOCK_NO_CACHE // Not worth caching.
);
$blocks['navigation'] = array(
'info' => t('Navigation'),
'description' => t('Provides the default navigation menu') ,
// Menu blocks can't be cached because each menu item can have
// a custom access callback. menu.inc manages its own caching.
'cache' => BLOCK_NO_CACHE
);
$blocks['new'] = array(
'info' => t('Who\'s new'),
'description' => t('Shows users recently registered'),
);
$blocks['online'] = array(
'info' => t('Who\'s online'),
'description' => t('Shows online users'),
'cache' => BLOCK_NO_CACHE // Too dynamic to cache.
);
return $blocks;
}
?>Block rendering:
The modules may define block rendering function in the form MODULE_block_delta where delta is the delta string of the block.
For example, for the user.module blocks we may have the function user_block_navigation which renders the Navigation block:
<?php
/**
* Block callback; generates the navigation menu
*/
function user_block_navigation() {
global $user;
if ($menu = menu_tree()) {
$block['subject'] = $user->uid ? check_plain($user->name) : t('Navigation');
$block['content'] = $menu;
return $block;
}
}
?>In the same way we may have the functions
user_block_login(), <code>user_block_new() and user_block_online()
Blocks configuration callbacks:
Having blocks configuration callbacks in a .inc file that only will be loaded in the block admin pages, let’s say MODULE.blocks.inc will be a performance improvement. Block settings callbacks and block settings submit callbacks may be of the form:
MODULE_block_delta_settings() and MODULE_block_delta_settings_submit($edit) respectively.
For example, user.blocks.inc will have the settings callbacks and the settings submit callbacks for the above blocks:
<?php
/**
* Block settings callback; Generates settings form for the <i>Who's new</i> block
*
* @return
* the form array
*/
function user_block_new_settings() {
$form['user_block_whois_new_count'] = array(
'#type' => 'select',
'#title' => t('Number of users to display'),
'#default_value' => variable_get('user_block_whois_new_count', 5),
'#options' => drupal_map_assoc(array(1, 2, 3, 4, 5, 6, 7, 8, 9, 10)),
);
return $form;
}
/**
* Block settings submit callback; Process the settings form submission for the <i>Who's new</i> block
*/
function user_block_new_settings_submit($edit) {
variable_set('user_block_whois_new_count', $edit['user_block_whois_new_count']);
}
...
?>Advantages of refactoring the block system and block module:
- More consistent API for block handling: move away from
$opto dedicated functions callbacks in separate files. - Extensible blocks declarations: Will allow us to declare other properties of the blocks, for access control checking, visibility, etc… Ex. http://drupal.org/node/147843
- More maintainable and organized code: move away from the
$opswitch to dedicated rendering functions and settings callbacks - Performance improvements: reduce the code loaded per request
- Usability improvements: provide better description about the blocks declared
Implementation:
I'd like to propose a patch I have been working to get these changes into core. Phases:
- Modification of
_block_render_blocksto render blocks using the dedicated callback function instead of themodule_invokestatement:
<?php
$function = "{$block->module}_block_{$block->delta}";
if (drupal_function_exists($function)) {
$array = $function();
}
?> - Refactor
_block_rehashto update the 'blocks' DB table with the blocks currently exported by modules using the newhook_blocks - Migrate all the
hook_blockin core modules to the newhook_blocksimplementation, starting withuser.moduleto have the Navigation menu block available as soon as possible. - Deprecate
hook_blockand write documentation forhook_blocks - Extend this idea to implement related feature request in the block system
#1
Here is an initial patch that implements the first required modifications in
block.moduleand the proposedhook_blocksinuser.modulefor demonstration. Tests and feedback appreciated.After applying the patch the blocks are functional, but only the blocks provided by the user.module will be displayed.
#2
I think this is a proposal worth considering.
I am not sure I like that every settings will now be a bunch of 2 (or more) functions implementations of Forms API. Too many functions per module, but seems like we have moved there already with other things (e.g. hook_settings() ...etc.).
Also, it would be better to have a 'block callback' element that would allow the function to be called back to be anything and not force the modulename_block_delta() as the only option.
Same for the _settings(), no need to have "hidden" or magical function names, we can have 'settings callback' => 'somefunction', and blocks who need it, define it.
What I do like is that you can now have a proper hook_block_alter(), which was proposed back in #76666, but not taken to completion.
#3
I like the way this is going and agree with kbahey's suggestions/reservations about the callbacks.
A big win here would be hook_block_alter() (could we have hook_block_$delta_alter like the new FAPI ones or would that be overkill?). This would allow for a contrib UI to the block caching system which was mooted when the patch originally went in but didn't surface. Also I guess when node_access modules are enabled core could alter every block to
BLOCK_NO_CACHErather than just switching it off.#4
This looks terrific in just about every way. I'll try to give it a review soon.
#5
Please rename 'info' and 'subject'! I think a refactoring of the block API should at least rename these, since these are the highest confusing names within the block API today IMHO. 'info' is such a bad name, it could be 'name' or something like that. 'subject' is also very vague IMHO. Unfortunately 'info' is used on the web interface as the name of the block, while 'subject' is the default title for the block, which is modifiable on the web interface. It might also make sense to try and get rid of one of these to remove some confusion.
#6
chx and I had been discussing something very similar to this back in February. Great to see someone else agrees that hook_block needs to grow up! :-)
I would actually recommend moving much closer to hook_menu's structure. Vis, hook_block would look something like:
<?php
function user_blocks() {
$blocks['login'] = array(
'title' => t('User login'),
'title callback' => 'function_to_generate_title',
'title arguments' => array('args', 'to', 'function'),
'block callback' => 'function_to_generate_body',
'callback arguments' => array('maybe', 'even', 'allow', 'dynamic', 'args?'),
'access callback' => 'user_access',
'access arguments' => array('who', 'says', 'we', 'can\t', 'put', 'this', 'in', 'code?'),
'description' => t('Provides the user login box'),
'cache' => BLOCK_NO_CACHE // Not worth caching.
);
// ...
return $blocks;
}
?>
That way, we can easily have multiple blocks that use the same callback function, make the title dynamic, have fancy access rules beyond just roles (such as "only show on node pages"), have smaller and more targeted functions (that are therefore easier to debug and unit test), and other fun stuff. I'd love to figure out a way to make blocks contextual here as well, via callback arguments. Not sure how easily we can pull in arg()s here. It's probably doable.
hook_block_alter() is then am absolute no-brainer. hook_block_$module_$delta_alter() is a maybe, maybe not. It depends how much block altering is actually going to happen. With the registry now, the cost of calling a hook should be far lower since we don't have to iterate m modules * n hook calls.
#7
Forgot to mention: As for settings, is that even needed now with form_alter? Can't a module just form-alter itself into the block config page and be done with it?
I don't mind lots of little functions, as that's easier to debug and to factor out to separate files, as long as they're sensible little functions.
#8
I just want to throw in: It is very very very important for Views that I am not required to have a separate function for each block. However, this is also true for block.module itself as well as aggregator.module -- both of which have blocks whose deltas correlate directly to database fields, not function names.
#9
It looks like we could even push the idea to its logical conclusion and drop hook_blocks altogether, by making it a subcase of hook_menu.
Thinks of it: basically, hook_block implementations map an internal call path like somemodule/somedelta (as opposed to a URL path for menus) to a set of node-like operations including loading, viewing (for display), editing (for configuration edit), updating (for configuration save). All of this subject to access, description, title, and caching considerations, just like menus.
It's so much like the intrinsic behaviour of other elements that it looks like it should be possible to leverage the existing menu code to unify the description and coding mechanisms, and simplify both the API and the underlying code.
#10
subscribe
#11
I'm with kbahey on the callback functions. Think modules that generates dynamic blocks (e.g. menu, views, etc). They often need only one callback for rendering all the blocks and other one for their settings form. I'm not sure it is actually doable with
module_block_delta()andmodule_block_delta_settings()functions.I think 'info' should be renamed to 'description' and 'subject' to 'title'.
#12
+1 to 'description' and 'title'.
Crell's #6 looks like it might be getting some way towards allowing for single callback functions - is that likely to be enough for views (and menu, aggregator, (boxes?))?
#13
@catch: It should be. Block module would have an implementation something vaguely like:
<?php
function block_blocks() {
$blocks = array();
$result = db_query("SELECT delta, title, description FROM {boxes}");
while ($record = db_fetch_object($result)) {
$blocks[$record->delta] = array(
'title' => $record->title,
'block callback' => 'block_box_body',
'callback arguments' => array($record->delta),
'description' => $record->description,
);
}
return $blocks;
}
function block_box_body($delta) {
return db_result(db_query("SELECT body FROM {boxes} WHERE delta='%s'", $delta));
}
?>
As long as the deltas are strings, no problem. And since they now are already, no problem. :-) Views/menu/etc. can do whatever their equivalent logic is. And just like the new menu system, hook_blocks() data can all be stored in the database and hook_blocks() never called again. That's another registry function that can be moved off to $module.registry.inc and never parsed again.
#14
@fgm - that's an interesting idea (basically define each block's content as a page callback?). But how do you get the settings and other features that are quite different?
#15
I don't see an user case for
'* arguments'properties nor callbacks for 'title' and 'access'. Maybe even and 'access' property is not necessary, as IMO the current implementation works well (if an user can't view a block the block callback should return NULL). Is that something to do in order to help blocks caching? Am I missing some points?#16
You don't see a use case for a title callback?!
What good is having a single callback for all the blocks and a fixed title?
#17
Well, see the #13. It uses a single callback, but no title callback. The point I can imagine is caching, e.g. you cache blocks metadata for a module e.g. (module_invoke_all('block')) but still allow dynamic titles and content through callbacks.
#18
That still doesn't work if the title is dynamic. Heck, the Navigation block title is dynamic. It changes based on who's logged in. Views titles are just as dynamic. They can change on pure whim.
#19
@fgm and @pwolanin: yes, that's may be interesting an idea, but the current menu system code is implemented to handle complex and hierarchical data as is the Drupal menu data structure, and it is optimized for that. Blocks are simpler concepts, have other configuration options, other cache mechanisms, are related with theme regions, etc... At the end, probably we will be only reusing the menu_router table (or similar) and the function callbacks, etc... I suggest to go with a clean blocks API instead.
#20
I'd say just like you get to a module settings using the menu system.
Remember 4.x: settings were a special case, with their predefined hook. In 5, it became just like any other path, with just system_settings_form() available to build the form. We could trod the same path here : after all, there are not so many module-define blocks that need configuration ; although I have some in one of my modules, for instance, I often think that having configuration in two places for a module (normal settings + block configuration) might actually be a usability problem. And as to custom blocks, they are just a specific case of block module defining its own module blocks like any other module would.
So remove the block configuration specificity, and add a block_settings_form() function so any block-defining module can easily define its own block configuration form, which will be added to the core block form.
#21
@merlin – for aggregator and views, you can define many blocks that point to one callback. Then, you can use the dynamic arguments crell was talking about to pass in (view name/display|aggregator stuff)
#22
To implement this refactoring we will require a new DB schema. The current DB schema is confusing and the blocks table contains redundant data (eg. there are blocks settings that are applicable to all enabled themes, and all the settings are replicated for each module, delta, theme tuple in the table).
Other issue that is worth considering is get rid of the blocks' delta and stick with a unique block ID. Each module should provide blocks with unique names, in accordance with Drupal namespace rules (i.e. MODULE_block_id).
Here is a diagram of a new database schema for block.module (see image)
blocks: Stores block's info and callbacks defined by enabled modules. This table will populated with the information extracted from hook_blocks, and can be altered by other modules using hook_alter_blocks ?
bid (primary key) is the block unique identifier
blocks_regions Maps blocks to themes and regions. Block, theme, region relations are stored here.
blocks_roles Sets up access permissions for blocks based on user roles (this table already exists)
blocks_custom Stores contents of custom-made (user defined) blocks. This table will replace the boxes table.
Check this diagram and find attached a patch to block.install and a complete block.install file with the proposed database schema defined.
Feedback and suggestions appreciated.
#23
Note that the proposed blocks_custom table is only for storing admin defined blocks through the blocks administration pages. Those blocks will be exported to the blocks system using the
hook_blocksimplementation in the block.module.#24
cool stuff. is there an up to date patch somewhere?
#25
@justinrandell: I am working on the patch, but would be good to receive more feedback about the proposed database schema. The patch in fact is based on it.
#26
- For consistency, IF we drop the module/delta split (which I'm not convinced of, but I am open to being convinced) then we should call it block_name or just name. All other qid-style fields in Drupal are auto-increment ints. bcid can then be just bid, which is an auto_increment field, and that value can be passed in to the block callback in the block definition array as an argument, which in turn neatly solves #220064: Remove numeric deltas from the boxes table (also fixes broken JOIN in pgsql 8.3).
- How important is it to have the module name listed in {blocks} if we're dropping the module/delta split? Is that necessary, or is it there "in case it's needed later"? (I am actually OK with "in case we need it later" if there's some idea of when it might be needed.)
- We want to make sure that we can still have CSS classes of block-$module and block-$unique_thingie ($module-$delta or $block_name).
- Is 64 characters long enough for a module name? {system} supports 255. We should do the same here.
- Why is "description" a TEXT field? Shouldn't it be a long varchar? Is it used anywhere other than on the blocks admin page (where even 255 characters is table-breaking long)?
- As long as we're breaking the schema, let's come up with a better field name than "custom". Without the description in the schema hook, it looks like a flag to indicate that the block is admin-created, which makes no sense. Maybe "user_visibility"?
- The title field in the database is ambiguous. Does it save the title key from the hook_block entry, or does it save the admin-entered title? We need both if we are going to avoid touching hook_blocks again. I'd suggest title and custom_title.
- We should also figure out the interaction between {blocks_roles} and access callback. My thinking is that if nothing is specified for either, that access property is allow-all. If it specifies anything at all, it becomes deny-default. To have access to a block, you must pass both tests. Thoughts?
#27
@Crell:
- Modules can provides content types, and they are not identified by module/delta (or module/type), they just have a unique name. Similarly blocks can have unique 'namespaced' names and be identified by them. I don't see real need of having a module/delta pair to identify a block. We can use the block name, or block id as the primary key for the {blocks} table and for blocks related tables. I agree that bid is not a good name for the block unique ID, may be block_id fits better and use block_name for the human readable name of the block (which is different to the block title). Blocks' names and descriptions are intended to be used in the blocks administration pages.
In the case of custom or user defined blocks, it's a different table {blocks_custom} (or just {boxes} ?), intended for internal use of the block.module to store user defined blocks. Won't be required to join to this database, the custom created blocks will be provided to the system through the
block_blocksimplementation. The primary key of this table can then be an auto-increment integer bid.block.module should be able to provide unique names for its blocks, maybe block_$bid, where $bid is the integer ID of the custom block.
-
As you say, it is there "in case it's needed later"... It maybe used for theming purposes, CSS classes and template suggestions in the form of block-$module, $block-module-$block_id, ..., etc...
- Having title and custom_title is Ok.
#28
I'm opposed to dropping module+delta. It gives a unique key that is not a random autoincrement but is rather predictable between sites, is unlikely to be the subvject of conflicts between modules, and can be used for nice CSS classes.
If there is not already, t some point we'll have a patch for dropping the existing autoincrement PRIMARY key, since it's pointless and never used.
#29
@pwolanin: I believe the proposal is to drop
$module = user, $delta = navigationin favor of$block_name = user_navigation. As I said, I'm still not convinced that's a good idea, but I can be convinced. For dynamic blocks, like custom blocks or aggregator, block_1, block_2, aggregator_1, aggregator_2 is what we'd end up with in either case, I suspect.#30
@Crell: probably better for themeing/maintenance:
block_<title>,aggregator_<title>...#31
@fgm: for block template suggestions we can use :
'block-' . $variables['block']->region;'block-' . $variables['block']->module;
'block-' . $variables['block']->block_id;
where $block_id is a unique module-provided block ID (block_name = human readable name of the block for displaying in the blocks admin page) .
IMO, having a block unique ID and not a module/delta pair, is easier to understand for themers: the system has blocks with unique IDs, no matter what module provides them. For theming individual blocks, is easier to use the block ID. If the themer knows about the module and is required to theme all blocks provided by a specific module, then we have the $block->module stored and the template
'block-' . $variables['block']->module;is suggested.However, the main reason I am proposing a block unique ID is with the intention to have a proper DB schema, where blocks related tables use the block_id as the primary or foreign key, and for simplicity and consistency. Other 'objects' provided by modules are not in the form module/delta, they just provide a unique name, e.g. content types, perms, elements, node operations, etc...
Attached is a proposition for the blocks database schema, a complete file and the patch
#32
I tend to agree about the unique block IDs. Drupal standards would suggest that block id should always be module name + _ + id anyway, so as to prevent namespace collisions. So that would effectively be module + delta anyhow.
#33
If we merge them into a single field, I'd rather call it block_name or name. "id" to me implies an integer and surrogate key, which this is not. It's more akin to the view_name of a view, or the field_* name of a CCK field.
#34
Well, block_name for the block unique ID, which in fact would be mostly
$module_$deltaand display_name for the human readable name.#35
As mentioned in http://drupal.org/node/214856, it would be great if we could figure out some way to pull in CSS or JS that is specific to a block in the various bits of metadata. Right now, it's very easy to get into a situation where a cached block only gets its JS and CSS the first time it's displayed...
#36
I would say there is a bit of scope creep here. We already have this problem now, so we are no worse off if we refactor the block system to make it behave the same it does today. So, if we can fix this, great! If we can't, let us move on with the other good things this buys us.
#37
Great topic.
#38
if we keep module as a column, then block system can do automatic uninstall and modules need not bother with it. seems pretty useful to me. we should do same for variables.
#39
Except for the fact that the inability to keep track of CSS and JS is a serious flaw in the current system. It's a bit silly to redesign the blocks system entirely -- so that it 'feels nicer' -- while ignoring the parts that are actually broken.
#40
OK, I'm convinced on including the module column. Let's do.
I see no reason to not fix the JS/CSS bug at the same time here. I see this issue as "modernize the block API and make it slick", and fixing a bug makes it more slick. :-)
Actually, correction, I do see a reason to not fix that bug. With the CSS and JS compressors, you get better performance if you include the CSS and (arguably) JS globally and let it get cached, sent once, and be done with it rather than including it only sometimes and then triggering another CSS/JS cache rebuild, which means *all* of that code gets resent. (Do you really want to have to send jquery.js and drupal.js multiple times?) So wouldn't it be a bug in modules that add CSS/JS in their block that they're doing it in the block in the first place?
#41
I have blocks that need to set their CSS dynamically.
#42
You would, wouldn't you... :-P Do you have an example of where it needs to be dynamic and not just setup for the site so that the compressor can do its thing?
(Note: This is now a side discussion and should not distract from the main question of the format of the hook itself. Sorry for the noise.)
#43
Sure, see flexible.inc in panels.
It's possible that could be retooled to create a .css file in the files directory in advance, but right now it uses drupal_set_html_head.
#44
Note, though, that due to the 'setting' feature of drupal_add_js, javascript is far, far more likely to be dynamic than CSS.
#45
Can I suggest some minor themeing enhancements?
Following on from drupcube @ #31, it would be really useful (for admin-created blocks at least) to be able to set an additional parameter per-block, say "block type" or "block class" that is available to the block template (in the $block object, as $block->class say). This means you can add $block->class as a CSS class and hook into pre-defined CSS definitions without having to leave the admin interface. (Something like http://drupal.org/project/block_class.)
Even more useful,
'block-' . $variables['block']->block_class;could be available as another template suggestion, so you can control (from the admin interface) the template that your block is going to use. (Something like http://drupal.org/project/blocktheme.) This means blocks would be catching up with nodes a bit in terms of the themeing possibilities. I mean, we can use the node type to select alternative page or node templates, and we can also do things like select a different page/node template based on taxonomy terms fairly easily.However for admin-created blocks the only "out-of-the-box" option at the moment is to create the block, check its ID and then create the template block-block-nn.tpl.php; if I want another block to use the same template then I'll have to create that, check its ID, and copy block-block-nn.tpl.php to block-block-mm.tpl.php or symlink it or something ... yuk!
IMO these simple enhancements should be in core and would make themeing custom blocks much neater. I'd post some code but this patch is already way over my head :P
#46
Related to #45: http://drupal.org/node/80227.
#47
+1 for #45. Being able to supply a top-level class for each block would extend and simplify block theming for both casual and expert users. Among the many benefits of including classes, fewer block.tpl.php files would be need, and styling would require less and more robust CSS (thus improving page load times). Theming blocks by their delta values -- a very non-robust solution -- would nearly be a thing of the past.
#48
do we even need 'blocks_custom' or can we get away with storing nodes of type 'block'??
that way you get revisions and comments, 'read more' etc for a block
#49
I am picking this one up.
I set this to critical as it would speed up drupal quite a bit, especially when combined with module registry.
#50
An important thing that I'm missing is the ability to have multiple instances of one block. We have the multiblock project in contrib now, but it would be *great* (not to say fantastic) to have this in core too. (correct me if I overlooked the proposal or any comments in this issue)
#51
This would require some re-thinking of the user interface. Let's do that in another issue.
#52
fair enough, seems like there's an issue about that topic allready, cf http://drupal.org/node/79571
#53
With the refactoring, is it worth adding a 'blocks_callback_file' to the schema to allow code migration to outside of the calling module. Similar to the file parameter in the menu system.
'blocks_callback_file' => array('description' => t('The file to include for this block, usually the block callback function lives in this file.'),
'type' => 'text',
'size' => 'medium'),
#54
No, because the D7 registry replaces the file and file path keys in menu hooks already. All lazy code loading should be automatic or nearly so at this point.
#55
I have no time because of school. I might pick this up next weekend at the code sprint @ BADCamp.
#56
Ok, I am going to have some time to continue with my patch.
#57
@dropcube: now the remove $op from hook_user and hook_nodeapi patches are in, i'll pitch in here as well.