Posted by profix898 on June 1, 2007 at 12:19pm
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | block.module |
| Category: | task |
| Priority: | normal |
| Assigned: | profix898 |
| Status: | needs work |
Issue Summary
As follow up from provide a 'global' block administration interface here is a patch which makes
- the
_block_rehash()function reusable (removes the global$theme_key) and - removes the hardcoded
$form_state['redirect'] = 'admin/build/block'in favor of a url argument (drupal_get_destination())
This simplifies the process to provide a blocks interface with 'by block' and 'by theme' functionality from a contributed module (see the discussion on that in the referenced issue).
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| block_reuse.patch | 3.11 KB | Idle | FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in block_reuse.patch. | View details | Re-test |
Comments
#1
#2
Nobody? Its a simple patch that doesn't hurt IMO.
#3
i would test ... but it says version 6.x ... I'm at 5.1 ... can I put it there?
Julie
#4
@spyderpie: The patch is for the development version (6.x) of Drupal and there is not much to test here. The patch doesnt provide any new functionality, it only rearranges the some small pieces of code in block.module. To test the patch you should install a development snapshot and play with the block administration. If you dont experience any difference with/without the patch then it is ready ...
#5
... still needs review ...
#6
Okay - give me about 2 days and I will test .... just got a lot going on .. (and two munchkins stealing my computer time! LOL)
#7
Rerolled for latest HEAD.
#8
... still applies ...
#9
Without something like this, it doesn't seem possible for a module to remove a block it has previously declared via an API function - rather one would have to delete items from the {blocks} and {blocks_roles} tables, which seems a bit ugly. This is kind of blocking this issue: http://drupal.org/node/156626
#10
@pwolanin: It seems you decided to delete blocks from the tables directly. I think a reusable block_rehash() would be useful in several locations and it is needed for being able to extend blocks administration in contributed modules. Otherwise we'd have to duplicate the whole function. Any chance to have this RTBC?
#11
I'll look in more detail. Seems the original code neglects to delete form {block_roles} if a block goes away?
#12
File to patch: modules/block/block.module
patching file modules/block/block.module
Hunk #1 succeeded at 210 (offset 57 lines).
Hunk #2 succeeded at 197 (offset 2 lines).
Hunk #3 FAILED at 205.
Hunk #4 FAILED at 231.
Hunk #5 FAILED at 254.
Hunk #6 FAILED at 514.
4 out of 6 hunks FAILED -- saving rejects to file modules/block/block.module.rej
Needs rerolled (Pre-split era?) and also needs to be rolled from the correct directory.
#13
I disagree this being a bug and especially disagree it being critical. Critical means a part of core is totally broken.
#14
Moving to 7.x
#15
subscribing
#16
This seems like an important missing piece of API to me
#17
The theme-key portion of this patch seems to have been implemented by #581118: Blocks admin user interface should not do theme switching
#18
Is this still an issue?
#19
If the function is usable externally it should be renamed as block_rehash()
#20
The theme issue does appear to be fixed as mentioned above; however, I agree with #19 that if the goal is to make this function globally reusable it should be renamed to block_rehash().
I've attached a patch to accomplish this. Note that this patch will break any contrib modules that currently call _block_rehash. An alternative way around this (although not very clean) would be to leave _block_rehash() as is and create a new function block_rehash() that just calls _block_rehash().
#21
#22
there is only a single module in contrib that uses the function so renaming should be managable
patch looks good to me
cd ~/drupalcat CVS/Root
:pserver:anonymous:anonymous@cvs.drupal.org:/cvs/drupal
cvs -q up
find . | xargs grep -si _block_rehash
./modules/block/block.admin.inc: $blocks = _block_rehash($theme);
./modules/block/block.module:function _block_rehash($theme = NULL) {
./modules/dashboard/dashboard.module: $blocks = _block_rehash();
./modules/dashboard/dashboard.module: * - blocks: An array of block objects from _block_rehash().
./modules/dashboard/dashboard.module: * - block: A block object from _block_rehash().
#23
API change => Drupal 8.
#24
Surely "private" functions don't form part of the API ??!
So effectively this is just adding to the API which should be OK - even after launch if it's too late for 7.0
#25
Subscribing. I have noticed there are cases in which I want to change which blocks are available programmatically, and would like to have a more elegant way to do so than with a function that has a hardcoded variable.
#26
Does not apply against 8.x so setting to 'needs work' and asking for a re-test.
#27
#20: block_rehash.patch queued for re-testing.
#28
The last submitted patch, block_rehash.patch, failed testing.
#29
seems not valid for now so could be closed