Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#34 | 684774-followup-d7.patch | 16.75 KB | andypost |
#31 | 684774-followup-d7.patch | 16.72 KB | andypost |
#29 | 684774-followup-d7.patch | 16.13 KB | andypost |
#27 | 684774-followup-d7.patch | 15.05 KB | andypost |
#24 | 684774-followup-d7.patch | 15.05 KB | andypost |
Comments
Comment #1
webchickAccording to Firefox, that other issue never made it to RTBC. Therefore, this one needs review.
Comment #4
andypost+1 seems reasonable, less interlinking between "optional" modules in future
Comment #5
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #7
sun.core CreditAttribution: sun.core commentedyo, @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!
Comment #8
sunhm, 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.
Comment #9
sunLooks like that condition was simply superfluous.
Comment #10
andypostI 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!
Comment #11
webchickCommitted to HEAD.
Comment #12
andypostThis commit introduce a new bug #735636: Restricting a custom block to certain content types while adding it leads to a fatal error
Comment #13
andypostBack 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 ?
Comment #14
andypostQuick fix, now working on tests - I think to merge tests for visibility and custom block functionality
Comment #15
andypostAlso node module should cleanup {block_node_type} when block deleted, but what to do with module-defined blocks?
Comment #16
andypostPatch 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
Comment #17
andypostPatch 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
Comment #18
sunWow, 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:
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.
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.
Comment #19
andypostFixed 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.
Comment #20
sunThanks, 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.
Comment #21
andypostPatch with node_block_info_alter() implementation
Comment #22
andypostA bit optimized patch - calls to menu_get_object() and node_type_get_types() are moved out of foreach loop
Comment #23
andypostglobal $theme_key was lost
Comment #24
andypostglobal $theme_key was lost
Comment #25
andypostMarked as duplicate #724410: Custom box block creation - error
Comment #26
sunBy 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.
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!
Comment #27
andypostNice hits, also changed
$block-theme and $block->status come from http://api.drupal.org/api/function/_block_load_blocks/7
So additional checking is useless
Comment #28
sunThanks!
We can apply basically the same optimization here:
$node_types = array_keys(node_type_get_types());
and use isset() afterwards.
Additionally, we could prepare
upfront and only test for
in the loop to remove all repetitive function calls to arg() and whatnot from the loop.
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!
Comment #29
andypostDone 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
Comment #31
andypostI cant understand why explicit compare fail so I leave
isset() && !=
Also added tests for node-type visibility
Comment #32
andypostwhen $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
Comment #33
sunI think this should be
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!
Comment #34
andypostI think that better to check isset() first
Also arg(2) fixed
Comment #35
sunThank you! Awesome work. :)
Comment #36
aspilicious CreditAttribution: aspilicious commented#34: 684774-followup-d7.patch queued for re-testing.
Comment #37
aspilicious CreditAttribution: aspilicious commentedBumping this :) it still applies
Comment #38
Dries CreditAttribution: Dries commentedLooks good. Committed to CVS HEAD. Thanks.