Nothing in menu.api.php relates to the menu module. It should be moved to system.api.php for consistency and clarity.

Comments

jhodgdon’s picture

Title: Menu.api.php should be destroyed » Menu.api.php contents should be moved elsewhere
cwgordon7’s picture

Issue tags: +Novice

Tagging this "novice" because it's a relatively easy issue that will serve as a nice introduction to the Drupal patch process. Basically, the required steps are:

- Checkout Drupal HEAD (see http://drupal.org/node/320)
- Take the contents out of modules/menu/menu.api.php
- Move them to modules/system/system.api.php
- Properly organize/alphabetize them with the other system functions.
- Create a patch (see http://drupal.org/patch/create)
- Upload the patch to this issue in a followup

You can always ask here or on IRC (see http://drupal.org/irc) for help if you need it.

BruceBGordon’s picture

Assigned: Unassigned » BruceBGordon

Taking this task.

BruceBGordon’s picture

Status: Active » Needs review
StatusFileSize
new12.16 KB

Removed the file and moved changes as per spec above.

Status: Needs review » Needs work

The last submitted patch failed testing.

BruceBGordon’s picture

Status: Needs work » Needs review

Bug in infrastructure exposed by this patch which attempts to remove a file. Hopefully fixed by now. Marking as needs review.

Status: Needs review » Needs work

The last submitted patch failed testing.

dave reid’s picture

Status: Needs work » Needs review

Remarking as needs review since the PIFR issue with deleted files has been resolved, I think.

Status: Needs review » Needs work

The last submitted patch failed testing.

cwgordon7’s picture

Status: Needs work » Needs review

Wondering if this has been fixed yet, and there's one easy way to test...

Status: Needs review » Needs work
Issue tags: -Novice

The last submitted patch failed testing.

Status: Needs work » Needs review

cwgordon7 requested that failed test be re-tested.

Status: Needs review » Needs work
Issue tags: +Novice

The last submitted patch failed testing.

aspilicious’s picture

StatusFileSize
new50.66 KB

I tried this, dunno if it is still relevant.

aspilicious’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Hmmm.

I looked into all of the hooks currently in menu.api.php. Most are definitely invoked from menu.inc and not menu.module, so yes those should be moved to system.api.php

But it looks like hook_menu_insert, hook_menu_update, and hook_menu_delete are only fired from menu.module, so they should probably stay in menu.api.php -- see http://api.drupal.org/api/function/menu_save/7 and http://api.drupal.org/api/function/menu_delete/7

jhodgdon’s picture

Also those hooks that are currently in menu.api.php and moving to system.api.php need to be added to the "menu" group/topic, if they are not already, using

@addtogroup menu

aspilicious’s picture

Status: Needs work » Needs review
StatusFileSize
new45.82 KB

Not sure if I understand it entirely...
Let's try it...

cwgordon7’s picture

Why did you switch the group from "hooks" to "menu"?

cwgordon7’s picture

To clarify, the "hooks" group should still be used even if you're going to add the new group "menu".

aspilicious’s picture

hmmm can you use both at the same time?

* @addtogroup hooks
* @addtogroup menu
* @{

* @} End of "addtogroup menu".
* @} End of "addtogroup hooks".

cwgordon7’s picture

Actually I don't know why it would be in the "menu" group at all. We currently don't appear to list hooks in their conceptual topics (e.g. hook_form_alter() is not referred to as in the forms group.)

aspilicious’s picture

jhodgdon ping ;)

jhodgdon’s picture

Sorry, I don't tend to work much on weekends. Too busy having a real life. :)

- I agree with cwgordon that they should be in the hooks group.
- I also think the menu hooks should be in the menu group, or at least hook_menu() should be, but that's open for debate.
- I am not sure if addtogroup works with {}. You can just add it in the individual function doc header.

aspilicious’s picture

StatusFileSize
new45.73 KB

try this

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Assuming the test bot agrees, let's get this one in. It simply moves the existing doc to the right file.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Makes sense to me. Committed to HEAD. Thanks! :)

Status: Fixed » Closed (fixed)
Issue tags: -Novice

Automatically closed -- issue fixed for 2 weeks with no activity.