Extracted from #375397: Make Node module optional

Block visibility settings are now vertical tabs, but Block module doesn't assign any #weight to each tab, so contributed modules cannot easily plug in there.

In addition, the current code does not really ensure that this form can properly be enhanced. However, we have a precedent in Node's visibility settings for blocks.

Hence, by moving Node module's integration code into node.module, we can ensure that Block visibility settings can be properly enhanced by other modules.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Status: Reviewed & tested by the community » Needs review

According to Firefox, that other issue never made it to RTBC. Therefore, this one needs review.

Status: Needs review » Needs work

The last submitted patch, drupal.block-node-visibility.0.patch, failed testing.

Status: Needs work » Needs review

Re-test of drupal.block-node-visibility.0.patch from comment @comment was requested by sun.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

+1 seems reasonable, less interlinking between "optional" modules in future

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

sun.core’s picture

Status: Closed (fixed) » Active
+++ modules/node/node.module	14 Jan 2010 00:39:02 -0000
@@ -2176,6 +2176,64 @@ function theme_node_recent_content($vari
+function node_block_admin_configure_submit($form, &$form_state) {
+  if (isset($form_state['values']['delta'])) {
...
+  }
...
+    $query->values(array(
...
+      'delta' => $form_state['values']['delta'],

yo, @sun, WTF?

Lucky life changing functionality that has no tests yet, eh?

--

Either this isset() is totally superfluous because 'delta' is always set - OR - 'delta' is always updated in $form_state by the primary form submit handler before this handler runs - OR - this code cannot work for newly added blocks.

140 critical left. Go review some!

sun’s picture

Status: Active » Needs review
FileSize
1.08 KB

hm, yeah, looks stupid. ;)

Since node + block modules are enabled by default, and I'm sure we have tests that add and edit custom blocks as well as module-defined blocks, attached patch should expose an error, if there is one.

If no error appears, then it's just a superfluous isset(), like you mentioned.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Looks like that condition was simply superfluous.

andypost’s picture

I really see no reason in the isset(), do we have a cases when it's could be empty?

+1 to commit. Anyway if this case could happen better to see error message!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD.

andypost’s picture

andypost’s picture

Priority: Normal » Critical
Status: Fixed » Needs work

Back to NW because I find no ability to pass $form_state['values']['delta'] to node_block_admin_configure_submit() when the block in creation stage.

Also marked as duplicate #735636: Restricting a custom block to certain content types while adding it leads to a fatal error

@sun should we rollback this or is there ability to reorder submit handlers to pass $delta ?

andypost’s picture

Status: Needs work » Needs review
FileSize
728 bytes

Quick fix, now working on tests - I think to merge tests for visibility and custom block functionality

andypost’s picture

FileSize
1.73 KB

Also node module should cleanup {block_node_type} when block deleted, but what to do with module-defined blocks?

andypost’s picture

FileSize
3.8 KB

Patch adds cleanup of {block_node_type} table with node_modules_uninstalled() and altering delete block form.

Also here is extended test.

Now both block.module and node.module are optional so I think that schema should be changed - {block_node_type} should be defined in node.install

andypost’s picture

FileSize
8.89 KB

Patch with changed schema

EDIT: update block_update_7001() could be used in #735900: Deleting module's blocks when module is uninstalled to cleanup orphan data

sun’s picture

Wow, awesome work! This clearly means that we missed quite a lot in the node type-specific visibility patch for blocks ;) (not sure which issue this was) I quickly double-checked and indeed, none of these settings are ever removed or cleaned up.

So technically, 99% of this patch is unrelated to this issue, but since we're already here, let's do it here :)

Likewise, this patch looks almost ready to fly, only minor points:

+++ modules/node/node.module	9 Mar 2010 10:30:47 -0000
@@ -2244,6 +2244,36 @@ function node_block_admin_configure_subm
+function node_form_block_custom_block_delete_alter(&$form, &$form_state) {
+  $form['#submit'][] = 'node_block_custom_block_delete_submit';
...
+function node_block_custom_block_delete_submit($form, &$form_state) {

Can we rename this form submit handler to

node_form_block_custom_block_delete_submit()

please? (i.e. _alter at the end replaced with _submit)

That would clarify that both functions belong together and "node_block_custom..." is not some sort of hook implementation.

+++ modules/node/node.install	9 Mar 2010 10:30:48 -0000
@@ -610,6 +638,41 @@ function node_update_7009() {
+ * Add the block_node_type table.
...
+function node_update_7010() {

1) Missing curly braces around {block_node_type}

2) Did you double-check that no other block or node update before 7010 needs this table?

Powered by Dreditor.

andypost’s picture

FileSize
10.55 KB

Fixed function names for all block-related hook_form_FORMID_alter() and added @see php-doc

And yes, I checked {block_node_type} is new table for D7 and not used in updates.

Also I found that it used in block_block_info_alter() so I think this part should be moved to node.module - if so, I could provide a patch. This hook should be good example of using this new hook.

PS: This issue is reasonable cleanup for #375397: Make Node module optional so webchick said to reopen this issue.

sun’s picture

Thanks, looks good!

I'd agree that moving the node type specific code from http://api.drupal.org/api/function/block_block_info_alter/7 into node.module as well would totally make sense.

andypost’s picture

FileSize
15 KB

Patch with node_block_info_alter() implementation

andypost’s picture

FileSize
15.03 KB

A bit optimized patch - calls to menu_get_object() and node_type_get_types() are moved out of foreach loop

andypost’s picture

FileSize
15.05 KB

global $theme_key was lost

andypost’s picture

FileSize
15.05 KB

global $theme_key was lost

andypost’s picture

sun’s picture

+++ modules/node/node.module	9 Mar 2010 16:53:34 -0000
@@ -2244,6 +2250,93 @@ function node_block_admin_configure_subm
+  $result = db_query('SELECT module, delta, type FROM {block_node_type}');
+  foreach ($result as $record) {
+    $block_node_types[$record->module][$record->delta][] = $record->type;
+  }
...
+        if (!in_array($node->type, $block_node_types[$block->module][$block->delta])) {
...
+        if (!in_array(arg(2), $block_node_types[$block->module][$block->delta])) {

By stacking the db records this way:

$block_node_types[$record->module][$record->delta][$record->type] = 1;

we could replace those slow in_array() tests with fast isset() checks.

+++ modules/node/node.module	9 Mar 2010 16:53:34 -0000
@@ -2244,6 +2250,93 @@ function node_block_admin_configure_subm
+    if ($block->theme != $theme_key || $block->status != 1) {
+      // This block was added by a contrib module, leave it in the list.
+      continue;
+    }

Is it guaranteed that $block->theme is set for all blocks and that it is always a string? Otherwise, we should test for isset($block->theme) first, and use a type-agnostic comparison (!==).

Same question basically applies to $block->status.

143 critical left. Go review some!

andypost’s picture

FileSize
15.05 KB

Nice hits, also changed

+      elseif (arg(0) == 'node' && arg(1) == 'add' && in_array(arg(2), array_keys($node_types))) {
+      elseif (arg(0) == 'node' && arg(1) == 'add' && array_key_exists(arg(2), $node_types)) {

$block-theme and $block->status come from http://api.drupal.org/api/function/_block_load_blocks/7
So additional checking is useless

sun’s picture

Thanks!

+++ modules/node/node.module	9 Mar 2010 18:30:14 -0000
@@ -2244,6 +2250,93 @@ function node_block_admin_configure_subm
+  $node_types = node_type_get_types();
...
+      elseif (arg(0) == 'node' && arg(1) == 'add' && array_key_exists(arg(2), $node_types)) {
...
+        if (!isset($block_node_types[$block->module][$block->delta][arg(2)])) {

We can apply basically the same optimization here:

$node_types = array_keys(node_type_get_types());

and use isset() afterwards.

Additionally, we could prepare

if (arg(0) == 'node' && arg(1) == 'add') {
  $node_add_arg = arg(2);
}

upfront and only test for

isset($node_add_arg) && isset($node_types[$node_add_arg])

in the loop to remove all repetitive function calls to arg() and whatnot from the loop.

+++ modules/node/node.module	9 Mar 2010 18:30:14 -0000
@@ -2244,6 +2250,93 @@ function node_block_admin_configure_subm
+    if ($block->theme != $theme_key || $block->status != 1) {
+      // This block was added by a contrib module, leave it in the list.
+      continue;
+    }

The comment basically states that it is possible that a block does not come from the database and was altered in. Hence, auto-applied properties from the regular block loading may not necessarily be set?

143 critical left. Go review some!

andypost’s picture

FileSize
16.13 KB

Done as proposed but no need in array_keys() because node_type_get_types() returns keyed array with values different from NULL

Also you were right about $block-theme and $block->status - $blocks could be altered and this needs explicit check!
So I changed all this checks to proposed (isset() && !==) - block.api.php also

Status: Needs review » Needs work

The last submitted patch, 684774-followup-d7.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
16.72 KB

I cant understand why explicit compare fail so I leave isset() && !=

Also added tests for node-type visibility

andypost’s picture

when $block-theme and $block->status returned from _block_load_blocks() are always strings, suppose because nature of PDO in fetchAllAssoc()

If contrib module adds own blocks with hook_block_info_alter() they could set status as numeric so strict type check is wrong here

sun’s picture

+++ modules/block/block.module	9 Mar 2010 22:14:55 -0000
@@ -640,15 +640,8 @@ function block_block_info_alter(&$blocks
+    if ((isset($block->theme) && $block->theme != $theme_key) || (isset($block->status) && $block->status != 1)) {

+++ modules/block/block.api.php	9 Mar 2010 22:14:55 -0000
@@ -252,7 +252,7 @@ function hook_block_info_alter(&$blocks)
+    if ((isset($block->theme) && $block->theme != $theme_key) || (isset($block->status) && $block->status != 1)) {

+++ modules/node/node.module	9 Mar 2010 22:14:55 -0000
@@ -2244,6 +2250,96 @@ function node_block_admin_configure_subm
+    if ((isset($block->theme) && $block->theme != $theme_key) || (isset($block->status) && $block->status != 1)) {

I think this should be

!isset($block->theme) || $block->theme != $theme_key || !isset($block->status) || $block->status != 1
+++ modules/node/node.module	9 Mar 2010 22:14:55 -0000
@@ -2244,6 +2250,96 @@ function node_block_admin_configure_subm
+  if (arg(0) == 'node' && arg(1) == 'add') {
+    $node_add_arg = arg(2);

arg(2) can be empty on node/add, so we need to append " && arg(2)" to the condition.

Additionally, note that we still have that weird, totally confusing and totally needless node-type-name-to-URL-and-back munging in D7:

If you create a new content type "Foo bar", then the machine name will be "foo_bar". HOWEVER, the URL to add a content for this is:

node/add/foo-bar

Therefore, $node_add_arg needs to munge back like we do in all other places in core:

$node_add_arg = strtr(arg(2), '-', '_');

145 critical left. Go review some!

andypost’s picture

FileSize
16.75 KB

I think that better to check isset() first

!isset($block->theme) || !isset($block->status) || $block->theme != $theme_key || $block->status != 1

Also arg(2) fixed

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thank you! Awesome work. :)

aspilicious’s picture

#34: 684774-followup-d7.patch queued for re-testing.

aspilicious’s picture

Bumping this :) it still applies

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Looks good. Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.