menu_link_maintain needs to return the mlid of the inserted menu entry

This little patch might help one of your cases. Also apply please http://drupal.org/node/172754 it might help some others if not, please patch.

I would say let's do what D5 does. My patch strives to get there.

osinet, I have problems with your patch. I have rerolled mine with your drupal_set_breadcrumb call, but I do not see what else have you changed. The drupal_set_title in aggregator_page_category has the wrong index and also, there is a title callback to set up the title so why do you need a drupal_set_title?

subscribe

My aggregator cleanup from a time ago caused this -- I was aware of the problem so I have readded the menu links in here but the issue title is nowhere near appropriate, the breadcrumb fix is a minor fix compared to the rest of the patch.

Why, oh why do we need to fiddle in the menu table in aggregator module?

As promised in the menu comment if this gets in, i will love a bit node_menu too.

Let me mention this is does not simply restore the category menu but by moving from router to links, it will scale easily to hundreds or thosuands of feeds.

huh i has written ['cid'] in the delete instead of ['mlid']

This is much cleaner: we use the module field.

typo in the code comment here: "Get an link by its path and the hadnling module."

also, would it not make more sense to call it function menu_link_load_by_path rather than function() menu_link_load_by_module() since module is an optional parameter?

this fixes the typo. Also added a full stop at the end of another comment. Don't credit etc.

Your patch did not apply. I have rolled from mine. To clarify, I simply removed the module default. There is no point in menu_link_load_by_path as the return of such a thing can not be defined because a path does not define a menu link, one path can have many links. Adding a module constraint makes it meaningful.

@chx - the result may still not be unique, even with the module constraint. Also, since we only want one result, it may be better to use db_query_range(). Also, we may not want to use m.*, since some fields in {menu_links} and {menu_router} have the same name. I think with MySQL the below may work correctly (i.e. the ml. fields clobber the m. fields, but that may be db-specific - is there an ANSI SQL standard for this issue?)

Actually - this is probably something that needs to get fixed in menu_link_load() as well (so maybe this code is my fault). I think the code here is more correct: http://api.drupal.org/api/function/menu_tree_page_data/6

something like:

function menu_link_load_by_module($path, $module) {
  if ($item = db_fetch_array(db_query_range("SELECT m.load_functions, m.to_arg_functions, m.access_callback, m.access_arguments, m.page_callback, m.page_arguments, m.title, m.title_callback, m.title_arguments, m.type, ml.* 
                   FROM {menu_links} ml LEFT JOIN {menu_router} m ON m.path = ml.router_path 
                   WHERE ml.module = '%s' AND ml.link_path = '%s'", $module, $path, 0, 1))) {
    _menu_link_translate($item);
    return $item;
  }
  return FALSE;
}

"Menu system is cool", Episode N: "do what I mean" featuring menu_link_maintain. Just does what needs to be done.

+ *   The path this link points to.
+ * @param $Link_title
+ *   The title of the link. Unused for delete.

Shouldn't link_title be lowercase?

Of course it needs to be and what stopped you from fixing the patch -- you surely had the time if you had the time to discover such an earth shattering bug :/ if I am not here after hours then this would have held up the patch totally absolutely needlessly.

Code in #20 mostly looks fine to me. I didn't try testing it, but it's clear and well-documented. 1 tiny nit-picking problem: "uncostumized" is a typo in a phpdoc comment. No time to re-roll myself, and it's not worth setting this to CNW over a typo in a comment. menu_link_maintain() looks like a nice improvement. Other than the typo, and oh, yeah, testing it, ;) I'd mark this RTBC...

I just grabbed a text-editor and edited the previous patch file for that typo in #21...

I am setting this to RTBC -- I know it works and I am fairly sure the core commiters will like the "do what I mean" approach.

Further testing revealed a notice thrown on delete.

OK, fixed the phpdoc for the menu link maintain function.

Before:

+/**
+ * This function helps your module maintains its links.
+ *
+ * It will insert, update or delete an uncustomized link belonging to a
+ * certain module pointing to the given path.
+ *
+ * @param $module
+ *   The name of the module.
+ * @param $op
+ *   insert, uodate or delete.
+ * @param $link_path
+ *   The path this link points to.
+ * @param $link_title
+ *   The title of the link. Unused for delete.
+ */

After:

+/**
+ * Insert, update or delete an uncustomized menu link related to a module.
+ *
+ * @param $module
+ *   The name of the module.
+ * @param $op
+ *   Operation to perform: insert, update or delete.
+ * @param $link_path
+ *   The path this link points to.
+ * @param $link_title
+ *   Title of the link to insert or new title to update the link to.
+ *   Unused for delete.
+ */

"This function" was pointless. We never tell something to the developer (ie. your module) and maintain was already in the function name, so it was not helpful. There was a typo in the op list and the link title missed explanation on how it is actually used. Now committed this patch.

This is an absolute no brainer.

Agreed. +1 on the basic change.

Not sure how i feel about return; break; right after each other like that.

+ the break is more consistent and defends us from a bug in the future if someone re-factors where the return happens
- the break isn't necessary as it currently stands

In the interest of defensive programming, I'm happy to leave it how it is, so I'm not changing this from RTBC, but I wanted to at least raise the point. Not sure if there's existing precedent in core on how to handle this.

Thanks for continuing to make Drupal better, chx. ;)

I think having the break there is good for code cleanliness and consistency. So committed.

The addition of "drupal_set_breadcrumb" in "modules/aggregator/aggregator.pages.inc" seems to possibly mess up the breadcrumbing.

See attached screen shot of "Categories" being duplicated.