Comments

jhodgdon’s picture

Sounds like a good thing to include. Thanks for reporting this -- I had never heard of this functionality.

sven.lauer’s picture

Status: Active » Needs review
StatusFileSize
new1.44 KB

Rolling a first patch ... as this is rather complex functionality, and we want the information to be communicated as concisely to be possible, I expect this to need a couple of iterations ...

jhodgdon’s picture

Status: Needs review » Needs work

Hmmm... After reading this documentation patch, I'm not understanding when I would use _load() vs. _to_arg(). It seems that both of them return something that replaces the wildcard in the argument list, correct? So I'm not understanding what the difference is, and the text you wrote doesn't illuminate that for me. Maybe say something like:

_load() and _to_arg() functions are similar, in that both replace wildcards with values. Use _load() when ...

Also, a punctuation issue: to punctuate "e.g." correctly, it should be:
+ * the wildcard; e.g., to render a link to the path.

Also, "user-ID" should be "user ID" without the hyphen.

sven.lauer’s picture

Issue tags: +Needs backport to D7
StatusFileSize
new2.2 KB

Thank you for your feedback.

It seems that both of them return something that replaces the wildcard in the argument list, correct?

Not quite. This is what _load() does. _to_arg() returns something that replaces the wildcard in the path.

I am attaching another patch that tries to make this clearer (but the section on _to_arg() is starting to get quite long. Is there a way to do emphasis in doc comments? I simply put *asterisks* around the words I wanted to emphasize, but I doubt that is the right way to do it.

I've also included a paragraph on the arguments that _to_arg()-functions will be called with. Not sure if this is the right place (but this is a general problem, I find: There is no canonical place/way to document callback signatures).

The documentation on the callback signature for the _load() functions (and how you influence it via the load_arguments entry) is quite impenetrable right now, I will open a separate issue for that, though.

sven.lauer’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1244980-doc-hook_menu-_to_arg-4.patch, failed testing.

sven.lauer’s picture

Status: Needs work » Needs review
StatusFileSize
new2.2 KB

Stupid me generated the patch backwards. This one should work.

sven.lauer’s picture

Assigned: Unassigned » sven.lauer
Status: Needs review » Needs work

Silly me. At the very least, the info on the function signature should be a list. Will re-roll tomorrow.

jhodgdon’s picture

I'll avoid critiquing the non-list... :)

But after reading this new explanation of the difference between load and to_arg, I'm even more confused. When is the menu system generating links...??? Maybe it's when you have tabs? Hmmm..

How about something like this:

_load() and _to_arg() functions may seem similar at first glance, but they have different purposes and are called at different times. _load() functions are called when the menu system is collecting arguments to pass to the page callback function (and other callback functions????). _to_arg() functions are called when the menu system is generating links to related paths, such as the tabs for a set of MENU_LOCAL_TASK items.

sven.lauer’s picture

Okay, I'll wait with re-rolling until we've settled on a wording here. Obviously, my powers of explanation are challenged by this task. Let me try and make this clear, maybe then you'll have an idea for a better wording.

_to_arg() functions are called in _menu_link_map_translate() (in menu.inc) [Which has its own related doc issue, I think, see below].

This function is called in two places in core: One is _menu_translate() (also in menu.inc). The doc for this function sums it up quite nicely:

/**
 * Handles dynamic path translation and menu access control.
 *
 * When a user arrives on a page such as node/5, this function determines
 * what "5" corresponds to, by inspecting the page's menu path definition,
 * node/%node. This will call node_load(5) to load the corresponding node
 * object.
 *
 * It also works in reverse, to allow the display of tabs and menu items which
 * contain these dynamic arguments, translating node/%node to node/5.
...

This working "in reverse" is exactly what _to_arg() functions do. So, yes, local tasks are an example.
_menu_translate() is call in menu_local_tasks(), menu_contextual_links() with the third param set to TRUE (i.e. only in these two functions is it instructed to call the _to_param() functions.

The other place is _menu_link_translate(), whose doc says:

/**
 * This function is similar to _menu_translate() but does link-specific
 * preparation such as always calling to_arg functions
 *
 * @param $item
 *   A menu link.
 * @param $translate
 *   (optional) Whether to try to translate a link containing dynamic path
 *   argument placeholders (%) based on the menu router item of the current
 *   path. Defaults to FALSE. Internally used for breadcrumbs.
 ...

Again, the translation governed by second argument is what _to_arg() functions do. The only place this is used in core ($translate = TRUE).

So, yes, your suggested wording maybe is the right way to go. This bit of functionality is a bit exotic, but that is all the more reason to try and document it properly.

Sorry for writing so extensively, but does that clear things up?

As I said above, there also seems to be an issue with the documentation of _menu_link_map_translate(), it says:

  /**
 * This function translates the path elements in the map using any to_arg
 * helper function. These functions take an argument and return an object.
 * See http://drupal.org/node/109153 for more information.
 ...

This is incorrect, _to_arg() functions return STRINGS not objects (in contrast to load() functions.

sven.lauer’s picture

Oh, and yes, the _load()ed object will be passed to call hook_menu()-callbacks instead of the corresponding path fragment.

jhodgdon’s picture

Is there a problem with what I suggested in #9, and if so, can you just correct it?

sven.lauer’s picture

StatusFileSize
new2.3 KB

Sorry, I misread your suggestion at first and then was unsure because you said you were still confused.

I guess we at least want to make clear that the _load functions work for all menu callbacks. The attached patch does that, and also turns the paragraph on the signature of _to_arg() functions into a list.

sven.lauer’s picture

Status: Needs work » Needs review

P. S. The patch also applies cleanly against current D7 HEAD.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for all of the iterations! I think this is ready to go into d8 and d7 now.

sven.lauer’s picture

Thank YOU for you patient help!

dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x and 8.x. Thanks.

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