Rehaul menu builder

They are bugs and should be fixed, no need to close the issue.

I might have misread: if the error comes from views.module and it's not related to the drupal, this issue can be closed/reassigned to views.module

Moving to views since it seems more related to that.

This is an issue of mysql 5 not likeing the blank value of '' being inserted.

Does anyone still see this? I think this may have been more of a bug with early betas than anything with Views; or it may've been an early Views bug. I never see this kind of behavior myself.

I just got the same message with a fresh installation using Apache 2.0.55, PHP 5.1.2 (module), and MySQL 5.0.18 under Windows. It happened in the admin/settings page after changing url rewrite and date things.

Location: /drupal47/?q=admin/settings
Referrer: http://localhost/drupal47/?q=admin/settings
Message: Out of range value adjusted for column 'pid' at row 1 query: INSERT INTO menu (mid, pid, path, title, description, weight, type) VALUES (3, -8, 'admin/filters', 'input formats', '', 0, 22) in D:\share\drupal-4.7.0-beta4\includes\database.mysql.inc on line 124.

The problem seems to be that the sql insert wants to assign the pid a negative value, while the column is defined in database.mysql as unsigned.

The same occurs where resetting the menus:

user warning: Out of range value adjusted for column 'pid' at row 1 query: INSERT INTO menu (mid, pid, path, title, description, weight, type) VALUES (71, -8, 'admin/filters', 'input formats', '', 0, 22) in D:\share\drupal-4.7.0-beta4\includes\database.mysql.inc on line 124.

It seems to me that the project can change to 'Drupal' and the menu (or 'mysql database') component.

the version is 4.7.0 beta4

downgrading.

-K

Good call to flag this as a menu system bug ... I think that someone who's worked on the menu system needs to have a look at this.

Should the pid values be negative? If so, the defintion of the table needs to be changed in database.mysql (and database.pgsql?), and we need to write/fix an update function.

What does it mean that the value was adjusted? If MySQL is changing the value, is this going to mess Drupal up?

I don't think it's wise to inflict these warnings on users, even if they are harmless. This is a pretty simple fix, so let's keep this as 'critical' until someone has a chance to look at it

Let's please not mark things critical just "so someone has a chance to look at it". The point of using a priority system is lost and affects the project as a whole if abused. Please mark critical bugs as critical, normal bugs as normal and minor bugs as minor.

I hope you can see where I'm coming from ...

Thanks,
-K

> What does it mean that the value was adjusted?

mysql> SELECT @@sql_mode;
+----------------------------------------------------------------+
| @@sql_mode                                                     |
+----------------------------------------------------------------+
| STRICT_TRANS_TABLES,...                                        |
+----------------------------------------------------------------+
1 row in set (0.00 sec)

mysql> INSERT INTO menu (mid, pid, path, title, description, weight, type)
    -> VALUES (3, -15, 'admin/filters', 'input formats', '', 0, 22);
Query OK, 1 row affected, 1 warning (0.05 sec)

mysql> select * from menu;
+-----+-----+---------------+---------------+-------------+--------+------+
| mid | pid | path          | title         | description | weight | type |
+-----+-----+---------------+---------------+-------------+--------+------+
|   3 |   0 | admin/filters | input formats |             |      0 |   22 |
+-----+-----+---------------+---------------+-------------+--------+------+
1 row in set (0.00 sec)

this means, all negative pid values get "adjusted" to 0, which definitely is not what we want.

even worse:

mysql> set sql_mode='STRICT_ALL_TABLES';
Query OK, 0 rows affected (0.00 sec)

mysql> INSERT INTO menu (mid, pid, path, title, description, weight, type)
    -> VALUES (3, -15, 'admin/filters', 'input formats', '', 0, 22);
ERROR 1264 (22003): Out of range value adjusted for column 'pid' at row 1

mysql> select * from menu;
Empty set (0.00 sec)

ie. *no* values are inserted. so

> If MySQL is changing the value, is this going to mess Drupal up?

i'd think so.

> Should the pid values be negative?

don't think so. there is this comment in menu.inc::_menu_build():

// Menu items not in the DB get *temporary* negative IDs.

as it seems, these *temporary* negative IDs are just never made positive again (which nobody noticed before because mysql 5.0.2 didn't have the STRICT modes)

> I think that someone who's worked on the menu system needs to have a look at this.

which probably would be JonBob/Jonathan, who wrote above comment. i won't make this.

> This is a pretty simple fix

hm ...

> so let's keep this as 'critical' until someone has a chance to look at it

seeing the sql error STRICT_ALL_TABLES: +1 (and changed accordingly)

Here is a patch that fixes this problem.

menu.inc:597

$new_items[$mid] = array(
    'pid' => $new_pid,
    'path' => $item['path'],
    'title' => $item['title'],
    'description' => isset($item['description']) ? $item['description'] : '',
    'weight' => $item['weight'],
    'type' => $item['type'],
+  'children' => $item['children'],
);

$new_item did not have the children key in item array, so the next iteration could not rewrite the newly generated 'mid'-s to $new_item.

menu.inc:609

    foreach ($new_items as $item) {

is rewritten to

    foreach (array_keys($new_items) as $mid) {
      $item = $new_items[$mid];

because as far as I can see, php clones the array that the foreach iterates on, so changing the value in $item does not effects $new_items.

Some drupal coder should review, maybe I'am wrong.

If so, please let me know.

I don't have access to MySQL 5 or PHP 5 to test this patch. We really need someone on this platform to verify that this fixes this issue.

But I can see how this patch does fix two problems with the existing code. The logic behind the patch certainly looks correct.

Doesn't work for me. Tested on fresh drupal-cvs. Apache, Mysql 5.0.18, PHP 5.1.1.

With postgresql negative values would have been accepted into db, because there is no 'unsigned' column type.

OTOH, I've checked my {menu} table and there are no negative pid values. How to reproduce the problem?

Yes, I'm with cvbge - I _can_ test this (e.g. have the environment and time) but cannot reproduce the original bug.

What modules/actions are necessary?

I have: PHP5.0.x
Mysql5.0.x
Drupal HEAD without any extra modules

Clicking around on admin/themes/menus as mentioned in the various reports of this bug did not create any errors in my log.

I reproduced this by reseting the menus. I also set sql_mode to STRICT_ALL_TABLES. The errors did not appear in the log, but on top of the page (on reload). As I0ve already said. This patch doesn't fix the problem.

Well, I'm using
Mysql 5.0.18-standard-log
PHP 5.0.4
Drupal CVS-HEAD

Here are the steps that I took:

1. SET sql_mode = 'STRICT_ALL_TABLES';
2. go to admin/menu/reset and "Reset all"
Expected to get the error, did not.

3. go to admin/settings/menu and "Reset to Defaults"
Expected to get the error did not.

So I'm not sure what to think. It seems the one thing consistent among people seeing the problem is PHP5.1, though I'm not sure why/how that would be related.

I spent some time looking at this issue yesterday and I do not think this patch will work because parent menu items must be processed before their children. Otherwise the children will be stored before their parent IDs are corrected.

Not all will be able to reproduce the origionally reported warnings due to PHP configuration differences. The symtom I found was that the edit menu item form had only a title field, which is caused by that form thinking the menu item was a top-level menu, which as a parent ID of 0.

Neil's fix looks elegant to me. Not tested.

I'm sorry that I don't know the logic behind the menu system. Anyway, this patch doesn't work right, IMO. After resseting the menus, the menu table is cleared. And empty fields are inserted. The only field, that has a Title set is the "Primary menu links" menu. The devel module outputs server calls like this

menu_save_item INSERT INTO menu (mid, pid, path, title, description, weight, type) VALUES (441, 0, '', '', '', 0, 0)
_menu_build SELECT * FROM menu ORDER BY mid ASC

So, the menu table is filled with empty menu items. When trying to edit a default menu item a link like this appears (http://localhost/?q=admin/menu/item/edit/-155), and all the fields in the edit page are empty.
If I do a page refresh of admin/menu page, more empty fields are inserted into the menu table.

I tested against php 4.4.2 and apache 2.0.55, mysql 4.1.5... Niels patch doesn't break anything here, but I don't have a php5 or windows platform to test on.

@paranojik I can't reproduce your error... Sounds like something else may be broken with you installation, can you share more details.

I've seen the new duplicate menu item bug paranojik descirbed once before. It was related to colliding paths in the menu system and url aliases. I believe that issue was fixed in HEAD.

You're right Niels. Sorry. I reaplied the patch on fresh HEAD revision. Reset menus produces no errors. Tested on same configuration as written above.

I was able to reproduce the problem, applied the patch and it disappeared.

The code is small and elegant.

applied

The previous patch fixes most of this issue, but still has a pair problems

After menu reset, some of the items -just a few of them- are not properly recreated, and still have negative ids in the 'administer menu' page. Some of them are 'administer/content' and 'administer/input formats' though this may depend on the modules you have enabled.

But, if you click again on 'list' or reload the page, the missing ones are created. So it works but needs two runs to do the work.

Besides that, _menu_build() is called for each iteraction which doesnt break anything but there's no real reason for it.

This patch is to apply on top of the previous one -already committed- and I think fixes this problem and the algorithm is somehow easier to follow.

Is it me or there some tabs in the code?

I can confirm this. I suspect the use of array indices is messing up using that array as a queue. Good catch on the rebuilding. The tabbing will have to be fixed and I have not tested this patch.

Fixed the tabs.

Neil, can you follow-up on this patch?

Also read http://drupal.org/node/53350 for more.

The code looks okay, but I'm worried this might infinate loop in one case. The code in HEAD currently is

if ($item['pid'] < 0 && isset($new_items[$item['pid']])) {

the code in the patch, which should be the negation of this, is

if ($item['pid'] >= 0) {

I think the second condition is necessary because not all menu items are given positive IDs since not all are editable. So I think this will infinate loop if an editable menu item has an uneditable parent since the pid will never be greater than 0. However, I'm not sure the menu system allows this case. It would be a nasty bug if it does happen.

I would rewrite this line as:

if ($item['pid'] >= 0 || !isset($new_items[$item['pid']])) {

See my comments at http://drupal.org/node/53714 .

Jose tells me there is no point in checking whether an item needs saving or not because the queue only contains the new items anyways. He is right. I rerolled still because Jose's patch failed to apply for me.

Thinking of Neil's suggestion that there might be infinite loops, the only chance of it happening is that an editable menu item has a parent that is either MENU_DYNAMIC_ITEM or MENU_LOCAL_TASK.

I guess you can build that menu on purpose, but does it really make sense?. And what should happen if a menu item is to be saved in the db -because it's MODIFIABLE_BY_ADMIN but has not a valid parent in the db?

I think it would be easy to add some double checking for infinite loops, but maybe it's better, in case we have such an inconsistent menu that the code just crashes. The alternative is to add some more code, but also some warning, which means one more string to translate... I mean, you can have modules producing inconsistent menus, but better the system crashes than allow this to go unnoticed.

I think our policy was that we do not babysit contrib modules. There are many ways to construct nonsense from the trivial omit of # from the beginning of a form attribute to adding a menu item under a dynamic menu item... Drupal would be bloatware if it'd try to cover all and would fail even then.

The patch menu_inc_plus_3.patch has solved the problem I had noticed with the menu system detailed at http://drupal.org/node/53350. Thanks and kudos to chx and all the people who have helped.

I am still unable to reproduce the problem, but the code looks sound and produces no new problems for me. If people are seeing a behavior improvement, then I see no harm in committing this.

Based on the last few comments, this status change seems reasonable.

applied.

The patch that was just committed looks good. I just thought I'd clarify one point, since a lot of posts in this thread seem to indicate confusion as to the meaning of negative mid values.

Unless I'm mistaken (in which case, Richard Archer, I know you'll correct me), I've always understood that menu items with POSITIVE mids get saved to the database, while menu items with NEGATIVE mids do NOT get saved to the database. If, after this patch has been applied, people are still seeing negative mids in their database's {menu} table, then there's something wrong, and it's probably the fault of the module that created the menu item.

Also, my understanding is that only dynamic (module-defined), non-cacheable menu items, or other special menu items (such as items of type MENU_CALLBACK) do not get saved to the DB. All module-defined menu items of type MENU_NORMAL_ITEM, that are cacheable, can and should always get saved to the DB, as well as (obviously) custom user-defined menu items.