Use proper menu router paths for the block module (but no argument loader functions) [#606640]

As per #473268: D7UX: Put edit links on everything -- and see also #606608: Use proper menu router paths for comment/* -- but actually a good cleanup in its own right.

Block module defines these two router items:

  $items['admin/structure/block/configure'] = array(
    'title' => 'Configure block',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('block_admin_configure'),
    'access arguments' => array('administer blocks'),
    'type' => MENU_CALLBACK,
    'file' => 'block.admin.inc',
  );
  $items['admin/structure/block/delete'] = array(
    'title' => 'Delete block',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('block_custom_block_delete'),
    'access arguments' => array('administer blocks'),
    'type' => MENU_CALLBACK,
    'file' => 'block.admin.inc',
  );

Then, in the page callback function, it searches for the (undeclared) 5th and 6th parameters in the URL, which are the block module and delta to be configured/deleted.

For proper consistency, we need to rename these URLs to follow the more standard Drupal pattern, e.g. something like:

admin/structure/block/manage/%/%/configure
admin/structure/block/manage/%/%/delete

Note that I added "manage" as one of the URL parameters to avoid possible conflicts with other URLs in the module (e.g. admin/structure/block/list).

Comment	File	Size	Author
#30	drupal.block-menu.30.patch	5.54 KB	sun
#30
#29	block.patch	5.54 KB	catch
#29
#24	drupal.block-menu.24.patch	5.54 KB	sun
#24
#19	606640-19-eojthebrave-block_router_paths.patch	27.6 KB	eojthebrave
#19
#14	606640-14-eojthebrave-block_router_paths.patch	27.06 KB	eojthebrave
#14
#12	606640-12-eojthebrave-block_router_paths.patch	26.9 KB	eojthebrave
#12
#11	606640-11-eojthebrave-block_router_paths.patch	26.9 KB	eojthebrave
#11
#3	606640-3-eojthebrave-block_router_paths.patch	19.25 KB	eojthebrave
#3

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

sun

German

Karlsruhe

CreditAttribution: sun commented 16 October 2009 at 17:55

Issue tags:

+D7 API clean-up

subscribing

Comment #2

sun

German

Karlsruhe

CreditAttribution: sun commented 16 October 2009 at 17:55

subscribing

File	Size
606640-3-eojthebrave-block_router_paths.patch	19.25 KB

Comment #4

sun

German

Karlsruhe

CreditAttribution: sun commented 16 October 2009 at 19:27

+++ modules/block/block.module
@@ -95,18 +95,18 @@ function block_menu() {
+  $items['admin/structure/block/manage/%/%/configure'] = array(
...
     'type' => MENU_CALLBACK,
...
+  $items['admin/structure/block/manage/%/delete'] = array(
...
     'type' => MENU_CALLBACK,

We should convert these into local tasks already.

I originally envisioned to use

admin/structure/block/manage/%block/%/configure

here, along with a

'load arguments' => array(5),

and introduce a new

function block_load($module, $delta)

that loads and returns the $block from the DB.

Not sure whether that complicates things...

Aside from that, this patch looks pretty straightforward.

I'm on crack. Are you, too?

Comment #5

David_Rothstein CreditAttribution: David_Rothstein commented 16 October 2009 at 19:33

I'm not the bot, but it looks pretty good to me :)

-  $items['admin/structure/block/delete'] = array(
+  $items['admin/structure/block/manage/%/delete'] = array(
     'title' => 'Delete block',
     'page callback' => 'drupal_get_form',
-    'page arguments' => array('block_custom_block_delete'),
+    'page arguments' => array('block_custom_block_delete', 4, 5),

Looks like there's an inconsistency here (you're passing an extra parameter to the page callback). Actually, I missed the distinction while writing the above issue (that this path only applies to custom blocks).... I wonder if we should actually rename this url to 'admin/structure/block/manage/block/%/delete' in that case?

A minor thing: The function signatures are currently:

function block_custom_block_delete($form, &$form_state, $bid = 0) {
function block_admin_configure($form, &$form_state, $module = NULL, $delta = 0) {

Seems like the latter parameters should no longer be optional, now that we're using the menu system? (Actually it may have never made sense before that way either, in which case this can be dealt with later.)

Comment #6

effulgentsia CreditAttribution: effulgentsia commented 16 October 2009 at 19:34

@sun: why "%block/%"? Any reason not to use the unique bid for loading %block, and then from the block object, the page callback would have access to both 'module' and 'delta'?

Comment #7

David_Rothstein CreditAttribution: David_Rothstein commented 16 October 2009 at 19:35

and introduce a new

function block_load($module, $delta)

that loads and returns the $block from the DB.

Not sure whether that complicates things...

Seems like it might complicate... note that the two callbacks require different things here (one needs stuff from the block_custom table, one doesn't), so maybe better to not make that change in this issue?

Comment #8

David_Rothstein CreditAttribution: David_Rothstein commented 16 October 2009 at 19:52

Any reason not to use the unique bid for loading %block, and then from the block object, the page callback would have access to both 'module' and 'delta'?

I think there are some places where we might need to construct these URLs but don't know the bid offhand (but already know the module/delta), so this would probably complicate the code in those places.... plus, the URLs would be less meaningful.

I'd vote for sticking with "%/%" to keep this patch as simple as possible :)

Comment #9

effulgentsia CreditAttribution: effulgentsia commented 16 October 2009 at 19:58

Furthermore, if we start using .../BID/configure instead of .../MODULE/DELTA/configure, then we can get rid of the word "manage" from the path, because a BID is always an integer. So we would just have /admin/structure/block/BID/configure (and delete), making it more like nodes and comments.

@eojthebrave: note also, http://drupal.org/node/606608#comment-2159830. probably same should apply in this patch if converting to local tasks.

Comment #10

effulgentsia CreditAttribution: effulgentsia commented 16 October 2009 at 20:02

oops. cross-posted with David. Never mind then. However, a helper function block_get_bid(module, delta) would be pretty trivial to write if the benefits were assumed to be worth it. I'm fine with the idea of KISS though, given that we're technically past API freeze.

Comment #11

eojthebrave

he/him

English

Minneapolis, MN

CreditAttribution: eojthebrave commented 16 October 2009 at 20:15

File	Size
606640-11-eojthebrave-block_router_paths.patch	26.9 KB

Now uses %block and block_load.

Changed path for delete to .../manage/block//delete

Changed block/configure to MENU_LOCAL_TASK, but not block/delete since this causes every block to get a delete tab, but it only applies to custom blocks.

@effulgenstia, while I totally agree that we should get rid of the module/delta paths lets leave that to another issue. Something like #430886: Make all blocks fieldable entities

Comment #12

eojthebrave

he/him

English

Minneapolis, MN

CreditAttribution: eojthebrave commented 16 October 2009 at 20:26

File	Size
606640-12-eojthebrave-block_router_paths.patch	26.9 KB

Extra white space removed.

Comment #13

16 October 2009 at 20:45

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #14

eojthebrave

he/him

English

Minneapolis, MN

CreditAttribution: eojthebrave commented 16 October 2009 at 21:26

File	Size
606640-14-eojthebrave-block_router_paths.patch	27.06 KB

Should fix test.

Also, changed MENU_LOCAL_TASK back to MENU_CALLBACK. The earlier change was causing some exceptions, and also caused a random "Configure block" tab to show up on block/overview page and link to nothing.

Comment #15

eojthebrave

he/him

English

Minneapolis, MN

CreditAttribution: eojthebrave commented 16 October 2009 at 21:27

Status:

Needs work

» Needs review

Comment #16

sun

German

Karlsruhe

CreditAttribution: sun commented 16 October 2009 at 21:51

Status:

Needs review

» Reviewed & tested by the community

Nice job!

Comment #17

sun

German

Karlsruhe

CreditAttribution: sun commented 16 October 2009 at 21:55

Status:

Reviewed & tested by the community

» Needs work

oh wait.

Comment #18

sun

German

Karlsruhe

CreditAttribution: sun commented 16 October 2009 at 21:59

+++ modules/block/block.admin.inc
@@ -88,12 +88,12 @@ function block_admin_display_form($form, &$form_state, $blocks, $theme) {
-        'admin/structure/block/delete/' . $block['delta']),
+        'admin/structure/block/manage/' . $block['delta'] . '/delete'),

Here, we miss a $block['module'], it seems.

Ah, no, we're missing a /block/ before the delta, but well, since $block['module'] == 'block', we can also keep it consistent and use the variable. :)

I'm on crack. Are you, too?

Comment #19

eojthebrave

he/him

English

Minneapolis, MN

CreditAttribution: eojthebrave commented 16 October 2009 at 22:59

Status:

Needs work

» Needs review

File	Size
606640-19-eojthebrave-block_router_paths.patch	27.6 KB

Fixed. With tests so it won't happen again.

Comment #20

sun

German

Karlsruhe

CreditAttribution: sun commented 16 October 2009 at 23:28

Status:

Needs review

» Reviewed & tested by the community

Awesome! Thanks so much, eojthebrave!

Comment #21

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 16 October 2009 at 23:49

Status:	Reviewed & tested by the community	» Fixed
Issue tags:	-D7 API clean-up

Awesome!

Committed to HEAD!

Comment #22

David_Rothstein CreditAttribution: David_Rothstein commented 23 October 2009 at 04:50

Status:

Fixed

» Active

See @catch's analysis at http://drupal.org/node/607244#comment-2179262 -- the way the block_load() function is being used here seems to be not so good for performance.

We may need to go back to something like the earlier simpler approach, which did not involve using a menu loader function (but rather unnamed placeholders)?

Comment #23

catch

he/him

English

CreditAttribution: catch commented 23 October 2009 at 05:50

Subscribing. The issue here is that the menu system calls all loader functions when checking access callbacks - just in case they're needed, which they're not here - so moving to anonymous placeholders ought to fix it. We can also fix it in a roundabout way by using fieldable blocks, but that's a much more complicated patch which doesn't really have anything to do with this issue.