API page: https://api.drupal.org/api/drupal/includes%21menu.inc/constant/MENU_NOT_... and https://api.drupal.org/api/drupal/includes!menu.inc/constant/MENU_ACCESS...

These both say:

> Internal menu status code

That's completely wrong, and incomplete.

'Internal' (to me at least) suggests that this is a constant used only by the menu system, which developers of other modules should not use. That is not true. Any page callback may use either of these as a return value, for example as seen in book module:

function book_export($type, $nid) {
  // Check that the node exists and that the current user has access to it.
  $node = node_load($nid);
  if (!$node) {
    return MENU_NOT_FOUND;
  }
  if (!node_access('view', $node)) {
    return MENU_ACCESS_DENIED;
  }

So:

- docs should not say these are internal
- docs should explain how these are used (as the return from a page callback) and why.
- docs should also mention in passing that using menu loaders and access callbacks in hook_menu() is often a better solution, but that these constants allow finer control when doing it in hook_menu() isn't viable.

CommentFileSizeAuthor
#23 interdiff-10159500-23.txt369 bytesrichardbporter
#23 menu_return_constants-2140157-23.patch862 bytesrichardbporter
#22 interdiff-10159500-22.txt795 bytesrichardbporter
#22 menu_return_constants-2140157-22.patch1.18 KBrichardbporter
#19 menu_return_constants-2140157-19.patch1.19 KBrichardbporter
#12 menu_return_constants-2140157-12.patch815 bytesrichardbporter
#10 menu_return_constants-2140157-10.patch826 bytesrichardbporter
#9 menu_return_constants-2140157-9.patch812 bytesrichardbporter
#6 menu_return_constants-2140157-6.patch1.99 KBrichardbporter

Comments

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Version: 7.x-dev » 8.x-dev

Yeah, and there are more of them, and they're also present in D8... although I don't know if they're actually being used in D8. There is still a function ajax_prepare_response present in D8 and not (yet) marked deprecated that uses them.

So let's do this in 8.x first.

joachim’s picture

joachim commented

I was under the impression that in D8 you return some Symfony doodad instead.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

I don't know, but ajax_prepare_response() is called from the Drupal ajax handler, and it has those constants in it.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Version: 8.0.x-dev » 7.x-dev

These don't exist in 8.x any more.

joachim’s picture

joachim commented
Issue tags: +Novice
richardbporter’s picture

richardbporter commented
StatusFileSize
new menu_return_constants-2140157-6.patch1.99 KB

I don't know if this issue is still relevant but I gave it a shot. I made an assumption from #1 that the rest of the menu_status_codes should not be marked as internal.

I'm happy to re-roll with improved text and any more docs needed.

richardbporter’s picture

richardbporter commented
Assigned: Unassigned » richardbporter
Status: Active » Needs review
joachim’s picture

joachim commented
Status: Needs review » Needs work

Thanks for the patch.

It's a good start, but the text shouldn't be exactly the same for each, and it needs a bit of work.

+++ b/includes/menu.inc
@@ -224,27 +224,42 @@ define('MENU_CONTEXT_INLINE', 0x0002);
+ * This flag can be used as the return value from a page callback, although it
+ * is preferred to use a hook_menu() implementation.

This is a constant, rather than a flag. PHP calls the flags when they're being passed to a function, such as preg_match_all(), but as elements of the language, they're constants.

So looking in detail at each one:

- MENU_FOUND: There's no need for any description text on this beyond the first line. Core doesn't use this, and in fact nobody will ever use this; it makes no sense. I suspect it's just included for the sake of being complete.

 define('MENU_NOT_FOUND', 2);

This should say that it's preferable to use a menu loader callback defined in a hook_menu() item.

 define('MENU_ACCESS_DENIED', 3);

This should say that it's preferable to use a menu access callback defined in a hook_menu() item.

+++ b/includes/menu.inc
 define('MENU_SITE_OFFLINE', 4);
 define('MENU_SITE_ONLINE', 5);

These ones ARE internal-only, I think. At least, I don't imagine a situation where a module would want to return either of these. So I'd say they should be left unchanged.

richardbporter’s picture

richardbporter commented
StatusFileSize
new menu_return_constants-2140157-9.patch812 bytes

Thanks for the feedback. Re-rolled and left MENU_FOUND, MENU_SITE_OFFLINE and MENU_SITE_ONLINE alone. I removed the 'flag' misnomer and changed the docs for MENU_NOT_FOUND and MENU_ACCESS_DENIED.

I didn't replace 'flag' with anything and I think it makes sense without it but it could explicitly say 'constant' or 'status code'. I wasn't sure what was preferable.

richardbporter’s picture

richardbporter commented
StatusFileSize
new menu_return_constants-2140157-10.patch826 bytes

Changed the docs for MENU_NOT_FOUND and MENU_ACCESS_DENEID to match the suggestion in #8.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Thanks for the patches! Don't forget tot change the issue status to "needs review" when you upload a new patch.

So... The docs for MENU_ACCESS_DENIED look good to me, but I'm less sure about MENU_NOT_FOUND.

+++ b/includes/menu.inc
@@ -229,12 +229,18 @@ define('MENU_CONTEXT_INLINE', 0x0002);
+ * Menu status code -- Menu item was not found.
+ *
+ * This can be used as the return value from a page callback, although it is
+ * preferable to use a menu loader callback in a hook_menu() item.
  */
 define('MENU_NOT_FOUND', 2);

First off, I don't think we use the term "menu loader callback" anywhere in the hook_menu() docs.... No, I checked: they're not called "callbacks", but "load functions". Let's keep the terminology consistent here.

Also... so ... I don't get this. If the "menu item" is not found, that doesn't seem to be the same as a load function, which would be used to indicate that a placeholder item indicated by the URL is not found, not that the menu item itself was not found? Given the example in the issue summary, it seems like this constant is being used by the Book module to mean "throw a 404 not found", not that "the menu item is not found".

Let's see. So yes, drupal_deliver_html_page() has the code that uses these. Definitely, if the return value from a page callback is MENU_NOT_FOUND, it gives a 404 page, so these docs are not really correct about saying the "menu item" is not found.

And using a load function might not be appropriate in all cases... but anyway it's probably OK to mention that in at least some cases, that would be a good idea.

richardbporter’s picture

richardbporter commented
StatusFileSize
new menu_return_constants-2140157-12.patch815 bytes

I changed the terminology to match that of the hook_menu() docs. I'm not sure I understand the difference between 'menu item not found' and a 404 enough to word the docs correctly for that.

richardbporter’s picture

richardbporter commented
Status: Needs work » Needs review
joachim’s picture

joachim commented

> If the "menu item" is not found, that doesn't seem to be the same as a load function, which would be used to indicate that a placeholder item indicated by the URL is not found, not that the menu item itself was not found?

It's not the same, but I am struggling to think of times when you're return a 404 for a path that doesn't have placeholders. If '/foo/bar' isn't found, then surely you just wouldn't declare it in the first place? For example, in Flag module, when the current state of the site means that a path isn't valid, eg 'flag/myflag/node/46' isn't accessible because the node is already flagged, then we return an access denied rather than a not found.

You tend to have a path that isn't found if there's a variable component to it, and when that variable gets a value that doesn't exist: such as a node ID that doesn't exist. Hence in that sort of circumstance, you'd want to use a menu loader, because then the work of returning a 404 is done for you.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

You're probably right. In any case, "menu item" to me refers to "an array element of a hook_menu return value" not "something that was loaded from a placeholder", so I would prefer that the documentation not say that a "menu item" was not found. In fact I wouldn't say "menu item access denied" either. Let's just say "not found" and "access denied" and leave "menu item" out of the documentation entirely.

richardbporter’s picture

richardbporter commented

Does that apply to the other status codes as well? MENU_FOUND and MENU_SITE_OFFLINE both refer to menu items too.

joachim’s picture

joachim commented

> "menu item" to me refers to "an array element of a hook_menu return value" not "something that was loaded from a placeholder", so I would prefer that the documentation not say that a "menu item" was not found

Agreed.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

OK then let's just make sure that in these constants, if we say "menu item" we mean "an array element of hook_menu() return value", and that we don't use the term "menu item" for other things.

Thanks!

richardbporter’s picture

richardbporter commented
StatusFileSize
new menu_return_constants-2140157-19.patch1.19 KB

I finally got back around to this, sorry for the delay.

I changed the docs again to reflect the suggestions in #17 and #18 if I've understood those correctly.

richardbporter’s picture

richardbporter commented
Status: Needs work » Needs review
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Looks OK to me... Although I think the wording of this is a bit odd:

+ * This can be used as the return value from a page callback, although it is
+ * preferable to use a load function in an array element of hook_menu() return
+ * value.

The problem here is that load functions are implicit rather than explicit in hook_menu() in Drupal 7. So you don't really put the load function into the hook_menu() return value (unlike the access callback that is mentioned in the other constant's docs). So can we reword this, maybe just saying:

... although it is preferable to use a load function to accomplish this; see hook_menu() documentation for details.

or something like that?

Thanks!

richardbporter’s picture

richardbporter commented
StatusFileSize
new menu_return_constants-2140157-22.patch1.18 KB
new interdiff-10159500-22.txt795 bytes

That makes sense. I forgot to remove the word 'Internal' in the last patch. I provided an interdiff as well.

Would it be appropriate to use an @see reference to the hook_menu() docs here?

richardbporter’s picture

richardbporter commented
StatusFileSize
new menu_return_constants-2140157-23.patch862 bytes
new interdiff-10159500-23.txt369 bytes

My code editor removes whitespace on save. I removed the affected lines from this patch.

richardbporter’s picture

richardbporter commented
Status: Needs work » Needs review
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

This looks good to me. Regarding @see, yes you can do that, but if hook_menu() is mentioned in context in the docs (as it is in this patch, which is better than a generic @see anyway because it tells you *why* you'd want to @see it), we don't also need an @see. Thanks!

David_Rothstein’s picture

David_Rothstein commented
Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks!

Fixed the following on commit since the grammar seemed slightly off otherwise. Maybe it's a big enough change that I should have put this back to "needs review", but on the other hand it really just makes the second half of the patch consistent with the first half:

diff --git a/includes/menu.inc b/includes/menu.inc
index 1fc1e58..1fe5a64 100644
--- a/includes/menu.inc
+++ b/includes/menu.inc
@@ -232,7 +232,7 @@ define('MENU_FOUND', 1);
  * Menu status code -- Not found.
  *
  * This can be used as the return value from a page callback, although it is
- * preferable to use a load function to accomplish this; see hook_menu()
+ * preferable to use a load function to accomplish this; see the hook_menu()
  * documentation for details.
  */
 define('MENU_NOT_FOUND', 2);
@@ -241,8 +241,8 @@ define('MENU_NOT_FOUND', 2);
  * Menu status code -- Access denied.
  *
  * This can be used as the return value from a page callback, although it is
- * preferable to use an access callback in an array element of hook_menu()
- * return value.
+ * preferable to use an access callback to accomplish this; see the hook_menu()
+ * documentation for details.
  */
 define('MENU_ACCESS_DENIED', 3);

  • David_Rothstein committed 29c287b on 7.x 
    Issue #2140157 by rbp, jhodgdon, joachim: menu return constants not...
richardbporter’s picture

richardbporter commented

I agree, that sounds better. Thanks!

Status: Fixed » Closed (fixed)

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