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:

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

/**
 * 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:

/**
 * Block settings callback; Generates settings form for the Who's new 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 Who's new 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 $op to 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 $op switch 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:

  1. Modification of _block_render_blocks to render blocks using the dedicated callback function instead of the module_invoke statement:
      $function = "{$block->module}_block_{$block->delta}"; 
        if (drupal_function_exists($function)) {
          $array = $function();
      }
    
  2. Refactor _block_rehash to update the 'blocks' DB table with the blocks currently exported by modules using the new hook_blocks
  3. Migrate all the hook_block in core modules to the new hook_blocks implementation, starting with user.module to have the Navigation menu block available as soon as possible.
  4. Deprecate hook_block and write documentation for hook_blocks
  5. Extend this idea to implement related feature request in the block system
Files: 
CommentFileSizeAuthor
#88 257032-88-block_system_refactoring.patch30.86 KBPasqualle
Unable to apply patch 257032-88-block_system_refactoring.patch View
#72 block_system_refactoring-257032-72.patch35.18 KBdropcube
Invalid patch format in block_system_refactoring-257032-72.patch. View
#31 block.install.txt8.8 KBdropcube
#31 block.install-new-db-schema7-257032-31.patch12.43 KBdropcube
Invalid patch format in block.install-new-db-schema7-257032-31.patch. View
#22 blocks-db-schema-7.png29.62 KBdropcube
#22 block.install.txt8.46 KBdropcube
#22 block.install-new-db-schema7.patch10.92 KBdropcube
Invalid patch format in block.install-new-db-schema7.patch. View
#1 blocks_system_refactoring-257032.patch11.72 KBdropcube
Invalid patch format in blocks_system_refactoring-257032.patch. View

Comments

dropcube’s picture

FileSize
11.72 KB
Invalid patch format in blocks_system_refactoring-257032.patch. View

Here is an initial patch that implements the first required modifications in block.module and the proposed hook_blocks in user.module for 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.

kbahey’s picture

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.

catch’s picture

Category: feature » task

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_CACHE rather than just switching it off.

moshe weitzman’s picture

This looks terrific in just about every way. I'll try to give it a review soon.

Gábor Hojtsy’s picture

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.

Crell’s picture

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:

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.

Crell’s picture

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.

merlinofchaos’s picture

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.

fgm’s picture

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.

Pasqualle’s picture

subscribe

recidive’s picture

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() and module_block_delta_settings() functions.

I think 'info' should be renamed to 'description' and 'subject' to 'title'.

catch’s picture

+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?))?

Crell’s picture

@catch: It should be. Block module would have an implementation something vaguely like:

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.

pwolanin’s picture

@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?

recidive’s picture

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?

merlinofchaos’s picture

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?

recidive’s picture

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.

merlinofchaos’s picture

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.

dropcube’s picture

@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.

fgm’s picture

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.

dmitrig01’s picture

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

dropcube’s picture

FileSize
10.92 KB
Invalid patch format in block.install-new-db-schema7.patch. View
8.46 KB
29.62 KB

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.

dropcube’s picture

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_blocks implementation in the block.module.

beejeebus’s picture

cool stuff. is there an up to date patch somewhere?

dropcube’s picture

@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.

Crell’s picture

- 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: Ensure block delta is class safe.

- 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?

dropcube’s picture

@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_blocks implementation. 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.

-

How important is it to have the module name listed in {blocks}

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.

pwolanin’s picture

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.

Crell’s picture

@pwolanin: I believe the proposal is to drop $module = user, $delta = navigation in 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.

fgm’s picture

@Crell: probably better for themeing/maintenance: block_<title>, aggregator_<title> ...

dropcube’s picture

FileSize
12.43 KB
Invalid patch format in block.install-new-db-schema7-257032-31.patch. View
8.8 KB

@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

merlinofchaos’s picture

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.

Crell’s picture

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.

dropcube’s picture

Well, block_name for the block unique ID, which in fact would be mostly $module_$delta and display_name for the human readable name.

eaton’s picture

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...

kbahey’s picture

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.

RobLoach’s picture

Great topic.

moshe weitzman’s picture

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.

eaton’s picture

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.

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.

Crell’s picture

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?

merlinofchaos’s picture

I have blocks that need to set their CSS dynamically.

Crell’s picture

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

merlinofchaos’s picture

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.

merlinofchaos’s picture

Note, though, that due to the 'setting' feature of drupal_add_js, javascript is far, far more likely to be dynamic than CSS.

gpk’s picture

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

gpk’s picture

Todd Nienkerk’s picture

+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.

treksler’s picture

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

dmitrig01’s picture

Assigned: dropcube » dmitrig01
Priority: Normal » Critical

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.

swentel’s picture

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)

dmitrig01’s picture

This would require some re-thinking of the user interface. Let's do that in another issue.

swentel’s picture

fair enough, seems like there's an issue about that topic allready, cf http://drupal.org/node/79571

Alan D.’s picture

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'),
Crell’s picture

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.

dmitrig01’s picture

Assigned: dmitrig01 » Unassigned

I have no time because of school. I might pick this up next weekend at the code sprint @ BADCamp.

dropcube’s picture

Assigned: Unassigned » dropcube

Ok, I am going to have some time to continue with my patch.

beejeebus’s picture

@dropcube: now the remove $op from hook_user and hook_nodeapi patches are in, i'll pitch in here as well.

beejeebus’s picture

@dropcube: how is this going? what's the best way to help move this forward?

dropcube’s picture

@justinrandell: I started working on the patch again. There are several things to refactor, including the database schema. I will post a patch soon, so that others can help to move this forward.

BioALIEN’s picture

Subscribing.

beejeebus’s picture

@dropcube: thanks for the update :-)

jsaints’s picture

subscribing

agentrickard’s picture

[edit] Disregard this post. Not relevant. Sorry. [/edit]

Is it worth filing a hook_block_alter() patch that is much smaller than these revisions?

drewish’s picture

subscribing

JohnAlbin’s picture

An updated patch would be nice so others can help out (even if its still CNW). Otherwise, we'll need to start with the patch in #31.

drewish’s picture

Status: Needs work » Closed (duplicate)
beejeebus’s picture

Status: Closed (duplicate) » Needs work

nope, not a dupe. this issue aims for much bigger changes to the block system.

just needs to be brought back up to date.

drewish’s picture

If you're serious about getting this committed I think it'd be worth restarting this issue with a clean slate. This is a long enough and just reading the first page or two it's not clear what's left to od now that #345866: remove $op from hook_block(): Documentation has been committed. I'd say write up a new summary and post it as a new issue.

beejeebus’s picture

@drewish: i think you're right, will create an issue over the weekend.

moshe weitzman’s picture

This is a good proposal. I'll just add that the $content part of the page actually should be a block as well, and should be moveable across regions like blocks are. This line of reasoning leads to some confusion in my mind whether our current 'page callback' in hook_menu() really belongs as a block callback. We discussed hook_menu() earlier in this issue but I think my suggestion is a bit different.

One key advantage of this proposal is that the $content of the page can now take advantage of block caching. Thats a key performance improvement for d6 and it is sad that the main part of the page can't use it.

My musings are probably off topic here, but I might as well muse while everyone who cares about blocks is listening.

drewish’s picture

moshe, you're sounding like chx now ;) he had a proposal he was shopping around at drupalcon that sounded quite a bit like that. i can't remember what he'd called it but you might want to touch base with him.

dropcube’s picture

FileSize
35.18 KB
Invalid patch format in block_system_refactoring-257032-72.patch. View

Ok, here is an updated patch against HEAD. The patch includes a new schema required for the new block system, implementation of the concept in the user module and changes in the logic required to build (rehash) and render the blocks. Currently, the blocks administration pages is broken.

I agree this issue is becoming long enough, and there is a lot of stuff to refactor in the block system, so +1 to the idea of split it up into a number of separate issues. I think a new DB schema should be the first step, a DB schema that allows us to implement the features requested around the blocks system.

catch’s picture

While we're pondering we should also look at how blocks are displayed on pages too.

Currently we load every enabled block on every page, then selectively remove them based on visibility rules and whether they've got content. Would be great to be able to create certain collections of blocks for certain paths (i.e. node/n, taxonomy/term/n user/n and specific paths as well) - remove the path matching from individual blocks - then we'd only load blocks on the pages they're specified to load on, and it would enable very nice UI improvements too.

Panels does this (although outside of the block system of course), and Jeff Noyes also did a mockup for a core UI here - http://groups.drupal.org/node/17590

Dries’s picture

Issue tags: +Favorite-of-Dries
RobLoach’s picture

Status: Needs work » Postponed
Issue tags: +Patch Spotlight
Frando’s picture

Status: Postponed » Needs work

There's no reason this should be postponed for #370172: refactor block visibility, they're quite seperate (#370172 adds new functionality to the existing block system while this one radically improves the existing parts).

catch’s picture

Also I'm fairly sure the visibility refactoring will play well with block_instances and might mean there's no need for a TODO there, so it's more a matter of which gets in first rather than dependencies.

quicksketch’s picture

Also subscribing. I've got intentions to separate hook_block_view() into hook_block_load() (to do the variable_get()s and settings loading) and hook_block_view() (which will use the properties of the $block object). It doesn't look like that functionality is included here, so I'll put this into a separate patch. Just something to keep in mind as we move forward with this.

sun’s picture

Since we are moving more and more stuff into blocks currently, we should make sure that each block stores its individual configuration. variable_get/variable_set simply doesn't work out. Especially when using Panels to re-use or relocate blocks individually on certain pages. This also applies to multi-site/-domain and multi-theme scenarios.

Effectively, we could just remove $op 'save' and let Block API save the block configuration serialized (or not; debatable) in {block}.configuration.

Earl might have further input here.

merlinofchaos’s picture

sun: In order for this to work you either need the block data stored externally (Hi here's a packet of config data, please save this somewhere and give it to me when you need me) or provide a unique instance ID for every variant of a block, which is then passed into the block when it is used.

Panels currently does the former.

Xano’s picture

Subscribing.

Very nice work so far :)

Xano’s picture

What if we create a hook_callback() to declare callback items used by hook_menu() and hook_block()? Something like this:

function hook_callback() {
  return array(
    'node_view' => array(
      'title callback' => 'node_page_title',
      'title arguments' => array(1),
      'page callback' => 'node_page_view',
      'page arguments' => array(1),
      'access callback' => 'node_access',
      'access arguments' => array('view', 1),
    ),
  );
}

function hook_menu() {
  return array(
    'node/%node' => array(
      'callback item' => 'node_view',
      'type' => MENU_CALLBACK,
    );
  );
}

function hook_block() {
  return array(
    'node' => array(
      'callback item' => 'node_view',
      'cache' => BLOCK_CACHE_PER_ROLE,
    );
  );
}

This is still a very rough idea of centralising callbacks that hasn't been worked out yet, but I hope you get the picture.

moshe weitzman’s picture

xano - that was discussed a year ago. see #19 for a reply.

Xano’s picture

#19 is irrelevant, since it responds to the idea of merging both hooks. I simply suggested we might move the callback info to a dedicated hook. Hierarchical data for menu items remains in hook_menu() and block-specific information remains in hook_block().

hass’s picture

#359546: DX: Implement hook_block_* (insert/update/prepare/delete) functionality for modules may be a duplicate and need to be renamed... not sure if you planed to implement hook_block_update(), hook_block_insert(), hook_block_prepare() and hook_block_delete() together with this issue here, but it would be important to have this hooks for modules doing something with a block.

seutje’s picture

subscribe

Gábor Hojtsy’s picture

The #544360: D7UX dashboard module patch was just interconnected with this one given that the block instances introduced here would be highly useful for the dashboard too.

Pasqualle’s picture

FileSize
30.86 KB
Unable to apply patch 257032-88-block_system_refactoring.patch View

first step.
No API, nor functional change in this patch. This is a cleanup of hook_block_view() implementations. The $block['content'] moved into separate functions in *.block.inc files. (These functions will be the 'block callback'-s)

It would be nice if this could be committed before the next steps..

Pasqualle’s picture

Status: Needs work » Needs review
sun’s picture

Status: Needs review » Needs work

We no longer split module files into more includes.

Pasqualle’s picture

why not?

Xano’s picture

We no longer split module files into more includes.

I think sun refers to the registry that we no longer have. However this doesn't mean we shouldn't split up modules in include files. Files of thousands of lines still have performance issues and are not very overseeable. On the other hand I must say that include files with just a few dozen lines hardly improve performance or reduce complexity.

moshe weitzman’s picture

Basically, you picked a bad piece of the patch to start with. We just had an epic battle over file split. Lets not do anymore splitting in D7, OK. Fatigued. There are other parts of this patch that would be great to get in.

catch’s picture

Also please review #430886: Make all blocks fieldable entities - it introduces a block instance table amongst other things (block module too broken to add fields otherwise), so would be a more up-to-date starting point.

chx’s picture

Version: 7.x-dev » 8.x-dev
KingMoore’s picture

It would be great if drupal was self aware of blocks somehow without needing to declare everything in hook_block. Like if I could just drop ".block" files into a directory and each one was a self contained block that knew how to list, view, save, whatever itself. The process of defining all my blocks in hook_block always seems a bit redundant.

catch’s picture

Priority: Critical » Major

Downgrading all D8 criticals to major per http://drupal.org/node/45111

treksler’s picture

http://drupal.org/node/430886 is a nice idea, but it's just more cruft on top of cruft

For D8,

1) The concept of 'blocks' and 'panels' might need to be merged. Let us step away from Drupal land and the historical cruft that has piled up and let us think for a second. Looking at a web page, what do you see? You see regions that contain content. Some of that content is news, some of it is a banner, some of it is a menu. The distinction between a block and a page is useless. Drupal needs one single abstract concept for a region on a webpage.

2) Drupal could use a layout editor that could let the user graphically create layouts of these content regions and assign them by URL. Themes would just be a a collection of these layouts, created via a layout API (Think how features relate to modules)

3) Built in content regions should be indexed so that switching themes does not destroy all block assignments.

Xano’s picture

quicksketch’s picture

Title: Blocks system refactoring » Split block $ops into separate functions
Status: Needs work » Closed (duplicate)

The original issue:

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.

Per #66 - #68. The original goal of this issue has been solved. #430886: Make all blocks fieldable entities is the next natural step here.