DROP Task: Remove hardcoded numeric deltas from all hook_block() implementations in core

Awesome idea (aka subscribe).

Subscribing.

Changing title to reflect that it's a DROP task now. See http://drop.cwgordon.com/node/18.

sweet.. i just re-themed by block theme function as this - Dr 5

function umg_wide_block($block) {
  $idname = strtolower(str_replace(" ", "-", $block->subject));
  $output  = "<div class=\"block block-$block->module\" id=\"block-$block->module-$idname\">\n";
  if ($block->subject != "") $output .= " <h2 class=\"title\">$block->subject</h2>\n";
  $output .= " <div class=\"content\">$block->content</div>\n";
  $output .= "</div>\n";
  return $output;
}

to fix this very issue... and i think there are more places in Drupal besides block ids that do this.. and it's bad.. very bad.. :)

just for completeness.. this likely works better (since some module defined blocks do not have subjetc's - like nice_menus)

function theme_block($block) {
  if (!$block->subject) {
    $idname = strtolower(str_replace(" ", "-", $block->subject));
    $output  = "<div class=\"block block-$block->module\" id=\"block-$block->module-$idname\">\n";
  }
  else $output  = "<div class=\"block block-$block->module\" id=\"block-$block->module-$block->delta\">\n"; 
  if ($block->subject != "") $output .= " <h2 class=\"title\">$block->subject</h2>\n";
  $output .= " <div class=\"content\">$block->content</div>\n";
  $output .= "</div>\n";
  return $output;
}

This will need some performance testing, but I'm pretty sure there won't be much difference between joining on a varchar with one character in it vs. 30 or whatever.

@webchick - this requires no changes to core. i think there is a full index on delta, so I don't think the join performance will change?

Note that the 6.x book module and menu module already use meaningful strings for deltas (thanks to me and chx).

I am plotting some not-yet-hatched evil things for the block system later in the D7 cycle that this would greatly help. Even if not, this is still a good idea. :-) Even when there are a variable number of blocks from a module, they should have a string prefix, like aggregator-feed-1, aggregator-feed-2, etc. (aggregator being the module name, not part of the delta itself, of course).

So, who's going to do it? :-)

I just claimed this at DROP. I hope to get it done in the next couple of days.

Ouch, it turns out that there are numeric deltas hiding all over the place, sometimes in unexpected locations ;) But I think this patch catches them all, except for three things I haven't done yet:

• forum module
• aggregator module
• a D6 -> D7 database update (which seems like it will be a bit of a pain because numeric deltas can theoretically exist in any row in the user table, thanks to user-specific block visibility settings)

I'll work more on it later, but feel free to check over what I have so far... thanks.

Here is the patch. All hardcoded numeric deltas are replaced, and there is also a batch update function.

I didn't implement the 'aggregator-feed-planet' idea. It's a great idea, but I think it needs more discussion for how best to go about it (translation = I can think of a few ways to do it but don't know which, if any, are good ways). It really should be a separate patch anyway and shouldn't hold the current patch up.

As things stand, the block IDs look like 'aggregator-feed-1', 'aggregator-feed-2', etc. (that was already the case in Drupal 6, actually). So I think that means this patch is sufficient for whatever @Crell's evil plans are ;)

I created a new issue for the 'aggregator-feed-planet' idea: http://drupal.org/node/234061

terrific work. thanks david. looking forward to this in core.

for bonus points, change the parameter name from $delta to $id. that removes the numeric connotation.

Thank you. Changing $delta to $id certainly seems to make sense and isn't hard to do. Do you mean just inside hook_block? Or everywhere else in the code too? (Changing it everywhere might be a pain. And there is already a 'bid' column in the blocks table, so having another 'id' too might be confusing.)

The alternative would be to just pretend you don't know algebra, and therefore "delta", being a Greek letter, should make you think of a string ;)

Better yet (or in addition to), what about just changing all the function signatures from

function hook_block($op = 'list', $delta = 0, $edit = array()) {
}

to

function hook_block($op = 'list', $delta = '', $edit = array()) {
}

That shouldn't cause any problems, right? And it would be a hint to module developers that $delta is supposed to be a string. This could be instead of or in addition to changing $delta to $id.

The suggestion in #16 should be fine as long as no modules test $delta for 0 specifically rather than simply for truthfulness. I highly doubt any are doing so, but it's worth checking. Of course, at that point perhaps $delta should default to NULL instead?

Personally I'd prefer $delta over yet-another-$id, as long as it's clear that it's a string.

OK, well one way or another, it definitely shouldn't be left at "$delta = 0", so I'm putting this back to code needs work..... I'm very busy at the moment, but I expect to be able to get back to it in the next couple of days unless someone else wants to do it before then...

OK, I had a little time to work on this. Here is a new version, using $delta = '' as the default; I decided to go with that since it seemed like the simplest change. Otherwise the patch is the same as before.

I checked the code, and actually it doesn't look like the default value of $delta ever gets used at all, so this really doesn't matter either way... but for instructive purposes it is probably best if it defaults to a string since that is what we want to encourage people to use. After this patch gets in, it will also be a good idea to update the API docs for hook_block().... those currently use numeric deltas too.

Seems ready to be committed. Just re-rolled with system_update_7001() renamed to system_update_7002() so that the patch applies to the current HEAD.

This looks great!

YAY.
I've always said that $delta was a bad misnomer, and string labels are a better idea

Refreshing the patch, renaming the update function to system_update_7004().

Just one observation: it would be cleaner to write:

$blocks['online'] = array(
  'info' => t('Who\'s online'),
  'cache' => BLOCK_NO_CACHE,
);

Instead of:

$blocks['online']['info'] = t('Who\'s online');
$blocks['online']['cache'] = BLOCK_NO_CACHE;

This is a nice patch. Great work!

Thanks! I was planning to reroll this myself but you beat me to it.

About the code style issues, I agree. I'm pretty sure I didn't introduce any of those, though, just kept ones that were already in there -- at least I hope so ;) In retrospect I wish I had fixed them as part of this patch, but I'd rather not go back and do that now since the patch is already RTBC. Hopefully it will get considered soon and we won't need too many more rerolls....

Committed to CVS HEAD. Great patch.

A couple of things:

* Before this patch is marked 'fixed', we should update the "upgrading your theme from D6 to D7" documentation.

* Now all deltas are strings, it might be a good idea to rename $delta to $id. It might be my CS background, but $delta sounds extremely numeric to me. This might be a good follow-up patch if this is considered to be a good, feasible idea.

Good work guys.

Delta is, and always has been, a misnomer.
Almost anything else is better, because delta has a maths/code/statistics meaning.
I say $block_id

This is a most welcome change for themers.

For block ID why not "$bid"?

For modules that create blocks on the fly, how would it possible to let the site administrator change the block id string from the block admin screens? Or let him add a class name?

@xmacinfo: In Drupal, variables in the form of $xid usually contain numeric IDs instead of identifier strings.

w00t!

If we're going all-strings, let's go all out and just call it $name or $block_name. (As kkaefer said, $bid would imply a numeric ID and is already used in at least 2 places, probably. )

I think module and name are good for emitting in CSS but we might want to query by bid now that this table has a primary key.

@moshe - that bid isn't a meaningful key - you can't really use it in a generic way. E.g. the same block on different sites may have a different value, right?

@pwolanin - consider when you are configuring a single block. we could load and save based on bid like we do for nodes, users, etc. but instead we use module/delta because of legacy code. there are probably more examples.

bid vs. module/delta is at #165785. This one should probably just be left for the upgrade documentation.

edit to remove old issue that should've been left alone back in November when I reopened it.

For renaming $delta, I like @Crell's suggestion of $name or $block_name. The problem with any of the "id" names is that there's already a 'bid' column in the blocks table itself. Having $bid and $(anything_else)_id in the same code would be confusing, since the difference between the two is very subtle (the first is a unique ID overall, the second is a unique ID within the module that defined the block).

This might not be a very simple change, though (I think it's more than just a "find and replace"...)

$block_name is as good as it's going to get.
As for search & replace, it doesn't matter. The var is local scope, and only really counts there and in the docs. Any module that's doing block stuff can continue to think of it as $delta until the next upgrade.
A note in the hook_block docs mentioning that it was renamed is quite enough.

Well, what I meant was that "delta" is used in various contexts all over core, not just the blocks system. (Plus it's embedded in database tables and stuff.) So we'd have to be careful to only change the ones we actually wanted to change. It certainly wouldn't be that hard to do, but I'm not sure it's something that I personally would be very excited about spending my spare time on either ;)

On the other hand, maybe I should repeat my question from #15: Are we talking about renaming $delta everywhere in the blocks system, or just inside the hook_block implementations? As you point out, the variable is local in scope, so any module that wants to can rename $delta in hook_block() to whatever it wants... But using $block_name in some cases while leaving $delta in other places seems like it could be a recipe for extreme confusion.

Anyway, renaming $delta isn't critical and could be split off to a separate issue. The two remaining critical things to do here are:

• Update the "converting your themes to 7.x" documentation
• Update the API docs for hook_block() to not use numeric deltas.

I'll try to get around to the above soon...

Note that this patch causes issues for new versions of pgSQL. See http://drupal.org/node/220064 for the conversation -- particularly comments > #30.

From a code standpoint, using numeric deltas is easier to support.

Block deltas show up for block-foo.tpl.php files, and also provide IDs for CSS, I think having slightly more code maintenance in core is worth the payoff for easier to maintain tpl.php and stylesheets everywhere else.

I don't see how this patch caused any issues. $delta was already a string in the database and was used that way in many cases (e.g., menu.module); it's just that parts of core were still using not-very-useful strings like '1', '2', '3', etc, and this patch made them more readable.

More specifically, this patch could have fixed the problem at the other issue, but didn't ;) One thing I did forget to mention above is that I totally ignored the administrator-defined blocks (the ones that match the {boxes} table) in this patch... those are still integers -- er, "strings that look like integers". I probably should have mentioned that somewhere, sorry. (I didn't see the reason for even trying to delve into that one since it's something that themers have to deal with only when customizing an individual site, and the title/entire purpose of the block can change at any time. In that case, a numeric ID might actually be the most friendly.)

Anyway, this reminds me that I still need to do the documentation updates for this issue. I'll make it a priority for this week.

Right, but that readability does have a cost. I think we are now agreed that it is worth the cost, but wanted everyone to be aware of the issues.

@Daivid_Rothstein: You're right, this patch didn't cause the bug. It's always been there, we just didn't get bitten by it. It's just that one of the two possibilities for fixing the bug is to drop support for string deltas, which would effectively roll-back this patch. I think most people are -1 on that, despite the added complexity it brings. #220064 is where we're discussing how to fix the bug.

And, yes, please, docs! :-)

Crell is right. Sorry, this patch does not _cause_ issues, it merely reinforces existing problems that need fixing.

OK, "upgrading your theme" documentation is now posted here:
http://drupal.org/node/254940

A patch (currently at CNR) for updating the hook_block() API docs is here:
#255098: Update hook_block() API docs for Drupal 7

And a new issue for renaming $delta to $block_name is here:
#255100: Rename $delta to $block_name in the blocks system

I think we're finished here. ;)