I noticed this, while setting a menu item's description to NULL, but it will probably be the same for other fields defined as "not null" in the schema. Steps to reproduce:

- Install Drupal (of course)
- Enable the attached "Test module" (containing a menu item with a NULL description)
- Read the recent log messages to see that the menu_router_build() failed with an exception:
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'description' cannot be null

Bad enough that this prevents ANY changes in menu router items afterwards, but it also somehow fails to set an error message. The reloaded Modules page looks exactly like normal, with a "The configuration options have been saved." status message.

Apparently, NULL descriptions are not very common, but when dynamically creating menu items, it can happen. And since the documentation of hook_menu() doesn't mention anything like this, I also didn't check for the case.

The problem is, _menu_router_build() tries to ensure non-NULL descriptions with the following code:

$item += array(
  …
  'description' => '',
  …
);

Descriptions that were explicitly set to NULL won't be affected by this (as will be the case for other fields).
Either we somehow fix this, or clearly state in the documentation that NULL values are not permitted. Otherwise, a single module innocently setting a NULL value could break a whole Drupal site, with the cause very hard to find.

The attached patch is a first go at fixing this, with a simple array_filter(). But more qualified solutions/patches are welcome. ^^"

Comments

dawehner’s picture

Status: Active » Needs review

New status. I'm not sure about it, because this is babysitting broken code.

Status: Needs review » Needs work

The last submitted patch, menu_router_build.patch, failed testing.

drunken monkey’s picture

OK, obviously this isn't that easy to fix. Let's wait for someone with more knowledge about the menu system. (If given directions, I would also give this another go, but as it is I'm completely in the dark.)
Or am I wrong in the assumptions that the bot couldn't enable SimpleTest because my patch messed something up? Does the bot use the menu system for this?

sun.core’s picture

Priority: Major » Normal

array_filter() removes more than NULL. Overall, this issue sounds like babysitting broken code, so I'm actually inclined to mark it won't fix.

drunken monkey’s picture

Status: Needs work » Needs review
StatusFileSize
new955 bytes

array_filter() removes more than NULL. Overall, this issue sounds like babysitting broken code, so I'm actually inclined to mark it won't fix.

I know, that's why I also moved all defaults that equal to FALSE further down.

And regarding the "won't fix" – wouldn't mind that too much, but could we then at least do some comment additions, stating that the entries in the menu array mustn't be NULL? As it is, this is a maybe improbable, but still rather nasty and unexplained bug source.

Anonymous’s picture

Status: Needs review » Closed (won't fix)

I agree that this is an annoying bug to find once it has been introduced. However, I'm not sure that having a line of documentation in hook_menu's doc block will help developers figure it out. The thing that would prompt them to look at the hook_menu documentation would be to look at the log messages, but the log message itself is pretty self explanatory.

Since this issue is almost a year old and others suggested marking it as won't fix, I'm going to do that. Feel free to change it back if you disagree.