Port Block API to Drupal 7

Hi John, I have a D7 version but have had trouble with CVS and have been unable to get it on to d.o.

Due to name changes in D7 I have had to make quite a few changes to the function names which I duplicated to the D6 module, but again have had trouble with CVS.

Attached are Zip files containing the 2 different versions, and I really need to get my head around CVS but am afraid I will bugger things up.

Both latest D6 and D7 releases now on CVS.

A couple of issues:

1) The HEAD branch contains D6 code.

2) There is also a DRUPAL-7--1 branch, but it is empty. (also, HEAD should be used for code compatible with Drupal core HEAD; so this is more or less correct, but having an empty branch is kinda confusing)

3) The official 7.0-beta1 release contains D6 code. (!) It should be unpublished and/or marked as unsupported.

4) There is no DRUPAL-6--1 branch for the D6 code yet, so HEAD needs to be branched into DRUPAL-6--1 first, before replacing HEAD with D7 code.

I really need to get my head round CVS.

I'm available in IRC, if you need help. :)

Thanks for your help sun, think I am getting it.

Attached is a patch with the changes to hooks to use hook_block_api_info rather than hook_block_api_block_types and hook_block_api_view instead of hook_block_api_display.

Also converted files from DOS to Unix so it is bigger than usual.

Attempt #2 at uploading patch

Patch again without the dos2unix bits

Patch looks good. However, I have some remarks we should tackle in a follow-up:

+++ block_api.module	8 Dec 2010 21:55:12 -0000
@@ -32,8 +32,8 @@ function block_api_get_config($delta) {
 function _block_api_get_types() {

@@ -48,8 +48,8 @@ function _block_api_get_types() {
 function _block_api_get_block_type_descriptions() {

I do not understand

1) Why these API functions are private?

2) Why there is a separate API function for retrieving the description?

Powered by Dreditor.

I have no idea, it probably made sense to me at the time.

I don't see any reason why the following 3 functions can't be combined:

_block_api_get_type(), _block_api_get_block_type_descriptions(), _block_api_get_types()

To have something like:

function block_api_get_types($module = NULL, $type = NULL) {
  if ($module && $type) {
    $types = module_invoke($module, 'block_api_info');
    return $types[$type];
  }
  else {
    $types = array();
    foreach (module_implements('block_api_info') as $module) {
      $module_types = module_invoke($module, 'block_api_info');
      foreach ($module_types as $type => $data) {
        $types[$module.':'. $type] = array(
          'title' => $data['title'],
          'description' => $data['description'],
        );
      }
    }
    return $types;
  }
}

Let's commit the current patch to HEAD first? Afterwards, I'll try to review the entire module's code + perhaps come back with a patch ;)

HEAD updated

We need #994870: Custom #type machine_name 'exists' callbacks cannot access other form elements/values for Block API, so please help to get that in.

Not sure I completely understand the need for it, although by the looks of it, it wouldn't do any harm anyway and would likely help some people with their validation of machine names in some cases.

Although it has brought up a point where you mentioned the "joined" machine name. I don't think this is necessary any more as the blocks are defined in the database as belonging to block_api and not the other module (this is handled in the block_api table) where as originally the block belonged to the original module like menu_block works.

So the "block_api_" prefix on the machine name/delta could be removed with no ill effects.

First pass.

Some considerations and topics we should discuss:

1. We need a clear and always obvious differentiation between block types and block instances.

   A block type is what modules register via hook_block_api_info(). A block instance is a block of a certain type.

2. The previous bullet already allows us to think big. Blocks should be entities, block types may be entity bundles.

   By implementing and integrating with D7's Entity API and Field API, there's *lots of* stuff we will get *for free*. (Such as: fieldable blocks)

3. The database schema of the module as well as its usage of delta, machine_name, module, and block_type is really really confusing right now. As of now (without really testing the functionality in the UI and merely looking at the code), it is entirely not clear to me what exactly the $delta of a block instance is with regard to the core Block module storage and handling.

Attached is an updated patch of your patch with a few bug fixes.

• Delete link on block admin page weren't displayed
• Error on creating new module block
• Corrected the redirect on block deletion

I'm just thinking if #2 is in the scope of the project i.e. is it needed in this module or better placed in a sub module to allow the user to create block types to appear in the module block creation list in a UI like they create content types at the moment but are still able to define block types within their modules as they currently can do with block api.

Regarding #3, block_type is needed as modules can define more than 1 block type and works in the same fashion as the core node module. machine_name and delta are the same thing (although delta prepends the entered machine_name with 'block_api_' (although I don't think this is necessary)

Coolio, thanks!

+++ block_api.module	12 Dec 2010 10:52:59 -0000
@@ -170,40 +182,45 @@ function block_api_get_blocks() {
 function block_api_form_block_admin_display_form_alter(&$form, $form_state) {
...
+  foreach ($blocks as $delta => $block) {
+    $form['blocks']['block_api_'. $delta]['delete'] = array(

@@ -267,28 +281,30 @@ function block_api_create_form($form_sta
+function block_api_validate_machine_name($machine_name) {
+  $delta = 'block_api_' . $machine_name;

+++ html_block/html_block.module	12 Dec 2010 10:52:59 -0000
@@ -1,15 +1,21 @@
 function html_block_block_api_info() {
   $types['html'] = array(

So as of now, I understand that

1. The effective $delta stored by Block module currently is block_api_MACHINENAME.
2. machine_name is stored as a reference in {block_api}, but without block_api_ prefix.
3. The internally registered block type, resp. $block_type, is only stored as a passive reference in {block_api}.
4. Modules integrating with block_api are registering block types with arbitrary, custom names; e.g., html. There is no module name prefix (e.g., html_block_html) that would ensure that block types of multiple modules are unique. Only the user-entered machine name of block type instances is unique.
5. Technically, the machine name would be sufficient to ensure uniqueness and identify blocks in Block module's administrative UI and menu paths.
6. Only relying on a unique machine name of block type instances means that there can only be one foo_bar block, regardless of block type. It sounds acceptable to have this "limitation".

Thus, it looks like we should

1. Remove the block_api_ prefix from machine names, so {block_api}.machine_name == {block}.delta.
2. Investigate whether we need to do something and what we can do to ensure uniqueness of internally registered block type names (i.e., html).

I also badly want to clean up, resp. rename, the {block_api} table column names in a follow-up patch.

Powered by Dreditor.

Implements most of #18.

A couple of fixes after manual testing.

Fixed block deletion (and also updated code and strings to latest Block module code in Drupal HEAD).

Further minor adjustments. I think this second follow-up is in itself self-contained and ready to fly (commit) now. We should continue with remaining tasks in this issue though.

Also, marking this RTBC reminds me... I'm going to commit the usual baseline of module and testing files (for automated unit tests), if you're OK with that?

Happy to remove block_api from delta and that makes sense.

The type has to be stored so that the module that defines the block type can save/delete/show form/view the block. As far as I can see, it doesn't matter if the block type is unique, as long as it is unique per module which it has to be with associative arrays.

Just found a small bug in D7 machine_name which I will submit, but everything else with the latest patch in #21 works fine.

Yeah that is fine.

Also changing status (was editing while you posted).

heya :) can you hop into IRC? :)

Thanks! Committed to HEAD: http://drupal.org/cvs?commit=463276

Still need to complete the db updates, but attached patch reworks the entire remaining CRUD functionality.

Completed the db schema changes. Renamed 'block_type' to just 'type'. Also considered to rename 'admin_title' to 'info' to be consistent with Block module's {block_custom} schema/terminology, but then re-considered, because 'admin_title' is way more descriptive than 'info'.

Now with tests...

42 passes, 0 fails, 0 exceptions, and 12 debug messages

YAY! :)

+++ block_api.api.php	12 Dec 2010 15:39:49 -0000
@@ -6,3 +6,27 @@
+ * @todo ¶
...
+ * @todo ¶
...
+ * @todo ¶
...
+ * @todo ¶

Oopsie, still need to document these.

+++ block_api.module	12 Dec 2010 15:54:03 -0000
@@ -9,8 +9,14 @@
+/**
+ * @todo ¶
+ */
 define('BLOCK_API_STORAGE_DATABASE', 0);
 
+/**
+ * @todo ¶
+ */
 define('BLOCK_API_STORAGE_OWN', 1);

However, leaving those two @todos for later, as I'm not sure how those constants are supposed to work currently.

Powered by Dreditor.

Added those CRUD API docs. (We're still missing Block API docs => leaving that to the follow-up patch that will change the hook names).

It's possible that I won't be able to come back to block_api module very soon, so I went ahead and committed those CRUD API improvements + tests now, in order to try to continue with as many improvements as possible today.

Full core entity system/API integration.

As tests still pass, committed that one, too.

Resolving some of the last remaining @todos.

Changed the hook names (more or less) as discussed. Also added documentation for the hooks.

Didn't go with the TYPE as suffix, because I figured that we might want to provide a "full" Form API integration at some point in the future; i.e., hook_form(), hook_form_validate(), hook_form_submit().

Same as 36 with a few typos amended in the html_block module file.

Thanks for reviewing, and testing! Committed to HEAD.

Attached patch is most likely going to be the last from me today. However, I really have a positive feeling about the current (revamped) API and state of the module.

JohnAlbin should be easily able to integrate with Block API now.

Also fixed missing phpDocs in block_api.install.

Added one small change to block_api_block_info() to additionally output the title of block type of a block instance. While doing so, I realized that hook_block_info() allows modules to set additional Block module properties and we don't support those currently.

Update to test when module defining block type is disabled.

Merged the patches and improved the added test.

This one fixes the fatal error revealed via tests when a block type module is disabled.

Separated the module system integration test into a new test case class. Note: You need to flush caches (class registry) in order to see and run the new test case in SimpleTest.

And #44 passed tests. :)

So my last worry was the html_block sub-module, which is technically outside of the project's namespace. Attached patch moves that simple example into the API docs and removes the sub-module.

The last remaining problem to resolve is the internal module/type clash in block_api_get_info():

        // @todo Block types of one module are potentially overwriting block
        //   types of another module. Namespace info in $types somehow.
        $types[$type] = $info;

As of now, the block type info is used internally in two locations only. It might even be possible to key the block info by module AND type; i.e., $types[$module][$type] - and perhaps that would also be most sane way to resolve the issue.

@jamestamr: Can you take this on?

Yep, will look in to it.

@jamestamr: Any updates?

@sun, no, I have been super busy on some other project work. Hope to get on to this over the holiday break.

We should try to resolve that tiny last bit in #48 as soon as possible (this week[end]?), so we can create an official release for D7.

I unfortunately haven't had a chance to look at it with my work load, but will try to get on IRC tomorrow and go through it.

Just had a quick play.

$types[$module][$type] = $info;

Won't work, as block_api_create_form() will still get the type by itself so name clashes won't will still clash on the add module block form (once you have added another foreach to take in to account the new hierarchy).

$types[$module .'_'. $type] = $info;

Breaks the hook_block_api_TYPE_xxxx() hooks, they would need to be hook_block_api_hook_TYPE_xxxx().

If we did this, we could change the hooks to hook_TYPE_block_api_xxxx(), but there is a chance of a name clash such as:

Module name: xxx
Block type name: yyy_zzz
Resulting view hook: xxx_yyy_zzz_block_api_view()

Module name: xxx_yyy
Block type name: zzz
Resulting view hook: xxx_yyy_zzz_block_api_view()