Block deletion form making assumption it doesn't check

I am not sure that drupal_not_found(); exit(); is the best response. Probably it would be better to set an error message and return to the block admin page?

Also, maybe the return value of block_custom_block_get() should be checked, and if there's no result, put up an error message and return to the block admin page?

As a note, the submit function returns to the block admin page with a success message normally:
http://api.drupal.org/api/function/block_custom_block_delete_submit/7
And the cancel destination for the "are you sure" is also admin/structure/block:
http://api.drupal.org/api/function/block_custom_block_delete/7

Still not quite right. As you can see from http://api.drupal.org/api/function/block_custom_block_get/7, that function doesn't receive the $module as input, so it's just trying to look up $block->delta as a BID.

So we need a check before that function even gets called to make sure $module == 'block'.

Either way this is done, I think the lack of validation checking (which could be in block_menu() or in the form as this patch does) is a major problem.

Actually... Since only custom blocks can be deleted with this form, why not change block_menu() so that the router path is
'admin/structure/block/manage/block/%/delete' instead of 'admin/structure/block/manage/%/%/delete', and change the form so that the module is not an input? The form won't work correctly unless module == 'block', so why even have that as an input?

Agreed on that! Let's try #10 suggestion.

Ummm.... Is this not possible to do without quite so many API changes, at this stage of D7, and without a patch to menu.inc?

The new hook_menu structure is a bit strange looking too, though I see why you have made those choices... but I'm not sure it was really necessary to introduce the magic %block and %block_custom_block (especially the first one -- what does that actually have to do with this issue?).

Maybe Damien Tournoud can comment too?

I have a few other comments.

a) Looking at the menu.inc hunk of the patch:

+        if (($item['type'] == MENU_DEFAULT_LOCAL_TASK) && !isset($item['load arguments']) && isset($parent['load arguments'])) {
+          $item['load arguments'] = $parent['load arguments'];
+          $item['load_functions'] = $parent['load_functions'];
+        }
         // Same for page callbacks.
         if (!isset($item['page callback']) && isset($parent['page callback'])) {

Why do you need to check that the type is MENU_DEFAULT_LOCAL_TASK? I don't think any of the other "inherit from parents" checks in this function do that.

In any case, this is new behavior, which needs to be documented in the function header of this function, or actually I think it needs to be documented in hook_menu(), or maybe both places?

b) As far as API changes, you have changed the names and arguments of several functions. So I'm adding a tag, so it can be announced if this patch is accepted.

c) Here's another problem:

-  $items['admin/structure/block/manage/%/%'] = array(
+  $items['admin/structure/block/manage/%block/%'] = array(
     'title' => 'Configure block',
     'page callback' => 'drupal_get_form',
     'page arguments' => array('block_admin_configure', 4, 5),
     'access arguments' => array('administer blocks'),
+    'load arguments' => array(5),
     'file' => 'block.admin.inc',
   );

You need to change the page arguments. block_admin_configure doesn't take these args any more.

d) I also don't understand the logic of this:

  *
  * @param $bid
  *   ID of the block to get information for.
+ * @param $module
+ *   The module that defines the block. If this is anything besides 'block',
+ *   this function will return FALSE, because the block would not be custom.
  * @return
  *   Associative array of information stored in the database for this block.
  *   Array keys:
@@ -471,7 +476,10 @@ function _block_rehash($theme = NULL) {
  *   - body: Block contents.
  *   - format: Filter ID of the filter format for the body.
  */
-function block_custom_block_get($bid) {
+function block_custom_block_load($bid, $module = 'block') {
+  if ($module != 'block') {
+    return FALSE;
+  }
   return db_query("SELECT * FROM {block_custom} WHERE bid = :bid", array(':bid' => $bid))->fetchAssoc();
 }

If $module must be 'block', then why even include it in the function as an argument at all? Why not just put 'block' explicitly into the hook_menu() in the patch, rather than having it be [module], if 'block' is the only value of [module] that would work?

Given that both Damien and I are uncomfortable with the changes to menu.inc, I think we need a patch that doesn't require that change. If you want to make that change to the menu system, it should be done on its own issue, so it's done right and consistently, not as part of a change to the block module.

Regarding the API change tag, it doesn't matter if it's a big change or a small change. It's still a change to documented functions, and it needs to be recorded, documented, and announced.

#23: block.patch queued for re-testing.

Oh gosh, this is still sitting here. It's likely still a bug, and although an API change, probably serious enough a bug to warrant getting in... Sigh.

#23: block.patch queued for re-testing.

Hmmm. As a note, I think that the issue could be fixed in a less comprehensive way that wouldn't require the API changes here. block_custom_block_delete() could just verify that 'module' == 'block' before it does anything.

http://api.drupal.org/api/drupal/modules--block--block.admin.inc/functio...

At this point, that's probably more reasonable?

That looks like the right check, but as noted in some of the other comments above, the check should go into a (probably new) access function in hook_menu() so that the page cannot even be accessed, rather than in the page generating function, right?

If for some reason it can't go there, then just returning nothing is not the right solution. At a minimum it should do a drupal_get_message() and a drupal_goto() because I think returning nothing would just show a blank page.

It's not a missing argument -- it's an incorrect use of the function.The menu router item is only intended to be used for blocks defined in the user interface -- the block module only puts up delete links for UI-created blocks, not for blocks coming from other modules. The only way the function would even be called from the UI is if someone types in an incorrect URL (or is tricked into visiting an incorrect URL).

So I think there are three possibilities of how to handle this wrong URL:

a) Do what node/[nonexistent node ID]/delete would do (and probably most similar things like taxonomy delete of a nonexistent term): have a 404 error. The way to get a 404 is to have the menu router item be something like block/%block_delete/%delete, and then have a block_delete_load() function that would return FALSE if it's not a block it can delete.

b) Have a 403 error. The way to do that is to have an access function that returns FALSE if it's not a block the user can delete or the user doesn't have permission to delete blocks.

c) Send you back to the block admin page with a message saying you can't delete this block. The way to do that is to put it in the function that this patch uses, but instead of just "return", do drupal_set_message and drupal_goto.

To me, any of these three seems like an acceptable solution... thoughts?

You've actually chosen option (b), not option (a) from #32 -- this will give a 403 error (not authorized) if an incorrect URL is chosen, not a 404 error (not found). To implement option (a), you would need an auto-load function.

Anyway, I think it's probably OK for it to give a 404... This patch looks mostly OK but:
- Yes, we need a test. Someone needs to create one (it should verify that you get a 404 if you try to delete a standard block such as maybe the user login block?). Then make a patch with just the test and upload it (so we can verify that the test fails without the code change), then make a patch with the test and the code change and upload it (so we can verify that the test passes).
- The documentation of the new access callback needs to follow the standards:
http://drupal.org/node/1354#menu-callback
(after "Access callback:" start with a verb).
- Also in the documentation, I don't think the module is "calling" the path. I think I would use phrasing more like "the module in the path" and "only blocks added by the block module can be deleted" instead.

#36: block_arg_check-851628-36.patch queued for re-testing.

You're definitely on the right track now! But this is a bit weird:

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

/**
+ * Checks that value of argument equals 'block'.
+ *
+ * @param string $module
+ *   Module argument from URL path.
+ *
+ * @return bool
+ *   Returns TRUE if argument equals 'block'.
+ */
+function block_delete_load($module) {
+  return $module == 'block';
+}

Normally the magic load functions should load something and return it, and then return FALSE if the load doesn't work. The reason is that the output of the load function replaces the URL path segment. So with this load function, if the URL is admin/structure/block/manage/block/5/delete, then block_custom_block_delete() form function is going to get arguments ($module = TRUE, $id = 5). Before it got ('block', 5). Although the function is not checking the argument (which was the whole reason for this issue in the first place -- it probably should be? That still needs to be added to the patch in case the form function is called directly, right?), it shouldn't be passed TRUE for $module.

So I think this load function should be instead:

if ($module != 'block') {
  return FALSE;
}
return $module;

right?

The tests passed, but... this doesn't look right to me:

-  $items['admin/structure/block/manage/%block/delete'] = array(
+  $items['admin/structure/block/manage/%block_delete/%/delete'] = array(

The previous version has manage/%x/delete and this new one has manage/%x/%/delete, which is a different path entirely. That can't be right? Something weird is going on...

Can you also attach the test as a separate patch so we can verify that the test fails without the patch? That is the best way I know of to assess test validity. Thanks!

Um. Is that the right URL in the test, given comment #42/#43?

So is this issue still a problem in Drupal 8?

In Drupal 7, you could formulate a URL that would look like it was for deleting a non-block-module block, and it would result in deleting a block-module block. Maybe in Drupal 8 you can't do that any more, because the URLs have changed, and we should just move this issue to Drupal 7 and fix it there?