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

subscribing

subscribing

Lets see how the bot likes this.

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?

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

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:

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.)

@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'?

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?

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 :)

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.

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.

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 custom blocks fieldable

Extra white space removed.

The last submitted patch failed testing.

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.

Nice job!

oh wait.

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?

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

Awesome! Thanks so much, eojthebrave!

Awesome!

Committed to HEAD!

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)?

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.

Two things:

1) Revert menu arguments for block configuration paths to /%/% without autoloader for $block to increase performance.

2) Introduce a missing MENU_CONTEXT_NONE constant to allow a custom module/theme to expose the "Delete" link for blocks as well.

Great patch.

Nope, it's still using proper menu router paths, but no longer argument loaders.

The last submitted patch failed testing.

Can't reproduce the syntax error locally, resubmitting.

bot?

Looks good. Committed to CVS HEAD. Thanks.

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