gotcha: menu wildcards, foreach() and MENU_SUGGESTED_ITEM

[Augustin (Beginner)]

Hello,


I just noticed a bug in directory.module, and I see the exact same bug in
tagadelic.module.

This bug involves the new menu system.
In D5, we used to be able to dynamically create a series of menu items with a
foreach loop within hook_menu().

When upgrading to D6, the tendency has been to remove the foreach loop, and
replace the arg(n) with a wildcard loader, but it is the wrong solution.

In most cases, you don't want to remove the foreach() loop you had in D5.
Within D6, it is perfectly ok to have a loop, recursively declaring several
well defined menu items, e.g. one item for each vocabulary. When you have
such a loop, do not use the wildcard loader. For a detailed example see this
issue:
http://drupal.org/node/283198

I have updated the documentation in the handbook.


Blessings,


Augustin.

[Augustin (Beginner)]

Hi again,

On the issue, crell write:

" A foreach loop inside a D6 menu handler is 99% of the time the WRONG way to
do it."
http://drupal.org/node/283198

Have I stumbled upon the 1% of cases where it's the right thing to do?
If you agree with crell and if I am wrong, I certainly would like to know it,
and I'll revert the documentation changes I made.

What is the proper way to have MENU_SUGGESTED_ITEM with wildcards and without
a loop?
See the issue for code examples.


Thanks,

Augustin.

[Larry Garfield]

On Wed, 16 Jul 2008 14:15:05 +0800, "augustin (beginner)" <drupal.beginner@...> wrote:

>
> Hi again,
>
> On the issue, crell write:
>
> " A foreach loop inside a D6 menu handler is 99% of the time the WRONG way
> to
> do it."
> http://drupal.org/node/283198
>
> Have I stumbled upon the 1% of cases where it's the right thing to do?
> If you agree with crell and if I am wrong, I certainly would like to know
> it,
> and I'll revert the documentation changes I made.
>
> What is the proper way to have MENU_SUGGESTED_ITEM with wildcards and
> without
> a loop?
> See the issue for code examples.
>
>
> Thanks,
>
> Augustin.

I will defer to chx and pwolanin on this front.  I am by and large quoting chx here, who said something very similar to me back in February.  I won't say you have definitely not found that 1%, but the whole point of all the fancy math in the menu system is to not have to loop but to simply declare matchable patterns.  Looping should be very strongly discouraged.

--Larry Garfield

[Karoly Negyesi]

> In most cases, you DO want to remove the foreach() loop you had in D5.

there, I fixed for you :)

> Within D6, it is ALMOST NEVER NEEDED TO HAVE a loop, recursively declaring several
> well defined menu items, e.g. one item for each vocabulary. When you have
> such a loop, DO use the wildcard loader.

there, I fixed for you :)

You forgot about the distinction of router items and menu links. You will want to use a single router item and save several links with menu_link_save as you see fit.

Regards

Karoly Negyesi

[Augustin (Beginner)]

On Thursday 17 July 2008 12:02:08 Karoly Negyesi wrote:
> You forgot about the distinction of router items and menu links. You will
> want to use a single router item and save several links with menu_link_save
> as you see fit.

Ah, yes!
Not knowing too much about the internals, I was looking at it from the
perspective of a module developer and what was possible using hook_menu()
only.

Thank you very much for your feedback.

I have updated the issue with a copy of your comment:
http://drupal.org/node/283198

And since this problem is not particularly obvious, I have updated my handbook
edits to correspond to the sanctioned solution:
http://drupal.org/node/103114
http://drupal.org/node/109153



Thanks again to Larry and Karoly for their comments.


Blessings,

Augustin.

P.S. to Larry: I guess I *still* need my beginner handle for a while longer,
after all... ;)

[Peter Wolanin]

Menu module is one example in core where several links are created and
added to the navigation menu.  For some things like node types, we
probably could have done the same (rather than using a foreach) but we
had also hit the "don't fix it if it works" phase of the core cycle.

-Peter

On Thu, Jul 17, 2008 at 12:28 AM, augustin (beginner)
<drupal.beginner@...> wrote:

> On Thursday 17 July 2008 12:02:08 Karoly Negyesi wrote:
>> You forgot about the distinction of router items and menu links. You will
>> want to use a single router item and save several links with menu_link_save
>> as you see fit.
>
> Ah, yes!
> Not knowing too much about the internals, I was looking at it from the
> perspective of a module developer and what was possible using hook_menu()
> only.
>
> Thank you very much for your feedback.
>
> I have updated the issue with a copy of your comment:
> http://drupal.org/node/283198
>
> And since this problem is not particularly obvious, I have updated my handbook
> edits to correspond to the sanctioned solution:
> http://drupal.org/node/103114
> http://drupal.org/node/109153
>
>
>
> Thanks again to Larry and Karoly for their comments.
>
>
> Blessings,
>
> Augustin.
>
> P.S. to Larry: I guess I *still* need my beginner handle for a while longer,
> after all... ;)
>
>