API page: http://api.drupal.org/api/drupal/modules--system--system.api.php/functio...

Describe the problem you have found:

Auto-loaders for hook_menu (such as node_load()) should return FALSE when the specified ID does not return a loadable object -- eg a node ID for a node that does not exist.

Returning FALSE causes the menu system to return a 404 for the user, whereas returning NULL does not.

Comments

jhodgdon’s picture

That statement is correct -- I found it documented somewhere recently when I ran into the same issue -- let's see... well I don't know where. But I agree it should be added to the hook_menu() documentation.

Sounds like a good Novice project.

joachim’s picture

I tested it today as I was building a module that used a menu loader and trying to figure out how to magically get a 404 for an invalid loader ID :)

jhodgdon’s picture

Same thing I ran into last week. :)

mkadin’s picture

As a young drupal novice, I'd be willing to fix this up in the docs...how do I edit them / submit a patch?

jhodgdon’s picture

See:
http://drupal.org/novice (novice guide to patching in general)
http://drupal.org/node/144223 (guide to fixing API documentation specifically)
Thanks!

jhodgdon’s picture

Also:
http://drupal.org/core-office-hours - a "drop-in" time on IRC where you can get help contributing patches for Drupal Core

mkadin’s picture

Status: Active » Needs review
StatusFileSize
new1.1 KB

Here's a patch. First time writing documentation. Thoughts? I'll backport it afterwards if folks like the explanation.

jhodgdon’s picture

That's a great start, thanks! I have a few suggestions on your prose:

+ * to replace the integer 1 in the argument array. Note that an "auto-loader"
+ * function should return FALSE when it is unable to provide a loadable object.
+ * Take the 'node/%node/edit' menu item as an example. The node_load() "auto-
+ * loader" function will return FALSE for the path 'node/999/edit' if a node
+ * with an nid of 999 does not exist. hook_menu will return a 404 error in this
+ * case, indicating to the user that the page was not found.

a) Above in this documentation, the load function is called a 'load function' (without the quotes) rather than an '"auto-loader" function' (with quotes). We should be consistent.
b) The style is a bit conversational... "Take the..." -- maybe it would be better to just say "For example, the node_load() load function for the 'node/%node/edit' path returns FALSE..." ?
c) Rather than saying "nid", please use "ID" or "node ID" in documentation.
d) hook_menu is not what returns the 404 error -- hook_menu() is just a hook that modules can implement. Probably it would be best to say "The menu routing system will then return a 404 error." (I don't think we need to explain that a 404 error means page not found -- that's pretty common knowledge amongst people who would be reading hook_menu() documentation).
e) For future reference, whenever you refer to a function in documentation, be sure to put () after it, so that when the doc is displayed on api.drupal.org, it will turn into a link to that function.

mkadin’s picture

Status: Needs review » Needs work

Thanks for the feedback, I'll get right on it.

I got the "auto-loader" terminology from a few blocks of text up the docs...

"Registered paths may also contain special "auto-loader" wildcard components
in the form of '%mymodule_abc', where the '%' part means that this path
component is a wildcard, and the 'mymodule_abc' part defines the prefix for a
load function, which here would be named mymodule_abc_load()."

Should I change the existing "auto-loader" use? Keep what I got? Change mine, leave the existing?

jhodgdon’s picture

Right. It says '"auto-loader" widcard components' once to refer to the %node types of things in paths, and then it refers to the *functions* as "load function" for the rest of the paragraph. Correct?

mkadin’s picture

Status: Needs work » Needs review
StatusFileSize
new1.04 KB

Yup that's right. I've changed the text around. Here's a new patch:

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Excellent, thanks!

webchick’s picture

Version: 8.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed to 8.x and 7.x. Thanks, mkadin! :)

Moving down to 6.x.

albert volkman’s picture

Status: Patch (to be ported) » Needs review
StatusFileSize
new1.19 KB

Here's a D6 backport for the documentation project.

jhodgdon’s picture

Status: Needs review » Fixed

Thanks! I've committed this to D6. Ignore any test failures above (Documentation project git repository not Drupal Core).

Status: Fixed » Closed (fixed)

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

miaoulafrite’s picture

Hello there!

nice thread

i'm currently in the situation where i use this "auto-loader" and i would like to define a callbck function instead of this 404 when node id does not exist

can you please point me some directions on how to?

jhodgdon’s picture

It looks like you need some programming support. I'm sorry, but this is a Drupal Core bug report (which has been closed)... And we don't really handle support requests in the Drupal Core issue queue as a regular practice.

There are several support options listed if you click on "Support" at the top of Drupal.org, which will take you to:
http://drupal.org/support

There you can find out about the Drupal IRC channels, and the Forums, which are our two main support mechanisms in the Drupal community.

Good luck with your issue!