Block configuration form is displayed when an invalid delta is passed through the URL

I can replicate the original issue, but the patch seems to not create a 404. I'm running WAMP, with latest 7.x install, clean install. Once applied, the form still appears but I get this error message on the form:

Notice: Trying to get property of non-object in block_admin_configure() (line 257 of C:\wamp\www\drupal\modules\block\block.admin.inc).
Notice: Trying to get property of non-object in block_admin_configure() (line 261 of C:\wamp\www\drupal\modules\block\block.admin.inc).
Notice: Trying to get property of non-object in block_admin_configure() (line 265 of C:\wamp\www\drupal\modules\block\block.admin.inc).
Notice: Trying to get property of non-object in block_admin_configure() (line 266 of C:\wamp\www\drupal\modules\block\block.admin.inc).
Notice: Trying to get property of non-object in block_admin_configure() (line 274 of C:\wamp\www\drupal\modules\block\block.admin.inc).
Notice: Trying to get property of non-object in block_admin_configure() (line 280 of C:\wamp\www\drupal\modules\block\block.admin.inc).
Notice: Trying to get property of non-object in block_admin_configure() (line 280 of C:\wamp\www\drupal\modules\block\block.admin.inc).
Notice: Trying to get property of non-object in block_admin_configure() (line 301 of C:\wamp\www\drupal\modules\block\block.admin.inc).
Notice: Trying to get property of non-object in block_admin_configure() (line 302 of C:\wamp\www\drupal\modules\block\block.admin.inc).
Notice: Trying to get property of non-object in block_admin_configure() (line 301 of C:\wamp\www\drupal\modules\block\block.admin.inc).
Notice: Trying to get property of non-object in block_admin_configure() (line 302 of C:\wamp\www\drupal\modules\block\block.admin.inc).
Notice: Trying to get property of non-object in block_admin_configure() (line 390 of C:\wamp\www\drupal\modules\block\block.admin.inc).
Notice: Trying to get property of non-object in block_admin_configure() (line 391 of C:\wamp\www\drupal\modules\block\block.admin.inc).

The form is still editable, and when submitted results in this:

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'delta' cannot be null: INSERT INTO {block} (theme, delta, module, region, pages, status) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5); Array ( [:db_insert_placeholder_0] => bartik [:db_insert_placeholder_1] => [:db_insert_placeholder_2] => [:db_insert_placeholder_3] => [:db_insert_placeholder_4] => [:db_insert_placeholder_5] => 0 ) in block_admin_configure_submit() (line 502 of C:\wamp\www\drupal\modules\block\block.admin.inc).

You are correct. I neglected to do so. After rechecking/patching I do get the 404 for all instances of an invalid delta I tried. My mistake.

+++ b/modules/block/block.moduleundefined
@@ -112,14 +112,15 @@ function block_menu() {
+  $items['admin/structure/block/manage/%block_object/%/configure'] = array(

Does this callback also need 'load arguments' => array(5)?

Powered by Dreditor.

Is this the same issue as #866972: Fatal error when attempting to edit non-existent block? (Only difference seems to be in which module is used in the URL, "block" vs "menu" - they lead to very different symptoms.)

@David_Rothstein
Yeah, the problem is the same. They are both a result of the fact that the menu system isn't calling the load function for the block. In fact nothing is checking to see whether or not the block really exists. The only reason we get an error for custom blocks is because block_custom_block_form() tries to add some defaults to the $edit array, but with a non-existent block an operation exception is thrown. If it didn't try to add those defaults, it would probably pop up with its form just like you see with non-existent Menu blocks. Something different might happen for every module you try.

I closed the other issue as a duplicate.

#3 needs a reroll.

To answer #8, the change to the callback in #3 does include 'load arguments' => array(5).

This patch is far too out date for a reroll, but I can look into how this is working now and try to create a new patch.

I actually made a mistake here. I thought this issue was against 8.0.x. I will re-evaluate this based on 7.x

Let's try this one.

After recreating the issue, I installed The patch in #16 and it gave me the 404 as expected.

RTBC++

1. Replicated issue on 7.x
2. Applied patch
3. Flushed cache
4. Verified path now gives 404

I'm getting a 403 (Forbidden) instead of a 404. Any reason that would be? This at least alleviates #866972 which originally led me here, so that's a big plus. This works for me, just wanted to clarify my experience was a different end result than a 404 as others have reported.

It's also giving me a 403 on blocks that ARE valid as well...any ideas? Should it matter that I ran the patch against 7.38 (it applied cleanly)?

This patch has a couple API changes (i.e. to the function signature of the block_admin_configure form, and to the URL patterns in hook_menu()) and I'm not sure why we need any of them?

Couldn't we just change the existing menu item to use a custom page callback instead (rather than "drupal_get_form") and then in the page callback return MENU_NOT_FOUND if the block doesn't exist, or return drupal_get_form('block_admin_configure', ....) if it does?

Adding relevant error messages to issue summary, so people can find this issue.

The other issue, #866972: Fatal error when attempting to edit non-existent block, which is now closed as duplicate, has a much simpler solution.

It simply checks, in the form, whether the block exists, and calls drupal_not_found() + drupal_exit() if not.
I propose we either do that, or we let the form display a "Block does not exist" text, without even bothering with a 404 header.

@David_Rothstein (#23)

Couldn't we just change the existing menu item to use a custom page callback instead (rather than "drupal_get_form") and then in the page callback return MENU_NOT_FOUND if the block doesn't exist, or return drupal_get_form('block_admin_configure', ....) if it does?

That would be another option, yes.
For my taste, let's aim for a minimum diff solution.

This patch has a couple API changes (i.e. to the function signature of the block_admin_configure form, and to the URL patterns in hook_menu()) and I'm not sure why we need any of them?

If we avoid signature changes, it means the form callback will keep as parameters $module, $delta, and have to call block_load().
Any function which does this should not rely on the block existing, but should have its own fallback logic.

Actually, if $delta === NULL, or if the block for the given delta does not exist, then block_load() returns a stub block object, which is used for the form to add new blocks.

On the other hand, block_custom_block_get($delta) returns FALSE if the delta does not exist.

I think we have to implement fallback behavior in all places where a delta can be NULL, 0, '' or it points to a non-existing block.
E.g. even if we implement fallback behavior in block_admin_configure(), the function block_block_configure() should not rely on that.

It simply checks, in the form, whether the block exists, and calls drupal_not_found() + drupal_exit() if not.
I propose we either do that, or we let the form display a "Block does not exist" text, without even bothering with a 404 header.

With all the different ways that these form builder callbacks are called, I think it is safer to show a form element saying "Block does not exist", than calling drupal_not_found() + drupal_exit().

How could such a form / element look like?

Actually, if $delta === NULL, or if the block for the given delta does not exist, then block_load() returns a stub block object, which is used for the form to add new blocks.

More problems.
block_load() attempts to load information about the block from the block database table.
But this table contains one entry per theme, for each block.
Or in some cases it might not contain any entry, even if the block does exist via hook_block_info(), because it was not written to the database yet.

In block_admin_configure_submit(), the new values for title, pages etc are stored for all entries in block table with the given module and delta.

This means we should assume that most of the columns in block table are synced between themes. Only the region and weight are not synced.

The reliable way to determine if a block exists or not is this:

  $info = module_invoke($block->module, 'block_info');
  if (isset($info[$block->delta])) {

Currently, this is being called from block_admin_configure(), but the result is only used to set the page title.
If the block does not exist, the form is still shown.

I think this is the place where we need to add the fallback logic.

It simply checks, in the form, whether the block exists, and calls drupal_not_found() + drupal_exit() if not.
I propose we either do that, or we let the form display a "Block does not exist" text, without even bothering with a 404 header.

If we return a crippled form, this will confuse implementations of hook_form_alter() which attempt to alter this form.

The proposal by @David_Rothstein in #23 would prevent this problem.

The drupal_not_found() + drupal_exit() would also prevent this problem, however I am not sure if it has any undesired side effects.

@mark.labrecque (#16)
Regarding the patch in #16.
Aside of what @David_Rothstein in already said in #23 about API changes.

+function block_object_load($module, $delta) {
+  return db_query('SELECT * FROM {block} WHERE module = :module AND delta = :delta', array(':module' => $module, ':delta' => $delta))->fetchObject();
+}

This is the wrong approach. Looking into the database is not the reliable way to check if a module + delta is valid or not.
Instead, we should call module_invoke($module, 'block_info'), and check if the delta exists (unless the delta is NULL for creating a new block).

I made a branch / PR on github which also contains code style changes.
https://github.com/donquixote/drupal/compare/7.x...7.x-1154742-block-edit
https://github.com/donquixote/drupal/compare/7.x...1154742-31-PR
(the tag will remain the same, the branch might change in the future)

I am going to upload a patch here, but this will only contain the first of those commits.
https://github.com/donquixote/drupal/commit/4e224f3528cd8327897da9e63e70...

The patch does not contain the change to block_block_configure
https://github.com/donquixote/drupal/commit/72b790c26895d7d15d90cea46e19...
(or any of the code style changes)

I do not post an interdiff to #16, because this is a completely new approach.

The combination drupal_set_message() + drupal_not_found() + drupal_exit() works nicely, it seems.
I added an additional exit; just to make the IDE happy.
Static code validation is important!

The code looks a bit complex, but this is simply because it takes care of various cases that were previously not reliably taken care of.