Removing the getCurrentItem method

[stof]

I open this issue to separate the discussion about this point as I originally asked in #2 but it got a bit lost in the other part of the issue.

As mentioned previously in #2, this method is currently lying as it returns the first current item found whereas the tree could contain several item.
So I see 3 solutions:

1. Forbid to have more than 1 current item. This would be quite hard to do in an efficient way and most people commenting on it previously thought it is valid to have several one.
2. Remove the method
3. Replace the method by a getCurrentItems method. This method would return an array containing all current items inside the tree of the item.

I personally think that the third option is not necessary. Implementing it using some PHP iterators would be probably more efficient as we would loop over the whole tree only when actually looping over the data instead of doing it first to create the array (and for people wanting an array absolutely, they could still use iterator_to_array).

So my personal vote would be 2

[stof]

@dbu @lsmith77 any input from the CMF team would be appreciated too

[dbu]

afaik we do not use the method in the cmf. knowing the current item is the job of the menu, and we delegate that to this library :-)
@uwej711 you did most of the MenuBundle - any input from you?

[uwej711]

I think it is used for the breadcrumb ... if it is removed we need to add some logic to render the proper breadcrumb which means we would iterate over the tree and search for the right menu item (and then we would probably also lie ...)

[dbu]

ups, thanks. indeed - a grep did not find it because its inside twig...

the breadcrumb topic is #24 . what would it mean for the breadcrumb if we expect more than one current item? pick the first one? show several breadcrumbs (that would be weird)? i would vote for using the first one - its kind of a confused scenario anyways when you have more than one.

i agree that there is cases where more than one active item could be. through (although most of them are most likely usability fails)

i think i see other use cases like showing the sub menu items on an overview page where i would want to know the current menu item as well. at least for the cmf, we can easily make the back-link from content to menu item, so we could do it that way instead of relying on the menu knowing what is "current".

[uwej711]

But the back-link only works when you have a content for a menu item which might not necessarily be the case - you could use the menu without any content nodes or without the content router. Basically you need some service that determines the current item for the breadcrumb. Then you are free to do what ever makes sense for the application.

[petesiss]

If there is a menu item at level 1, but directs straight to its first child (level 2), is that an example of more than 1 current item... because either menu items could be correctly reported as "current" ?

If so then I dont think this should be disallowed as it is quite common practice.

In addition whichever option is taken, surely there still needs to be a way to reliably get the current item in a template - otherwise I am not sure how things like the breadcrumbs, or displaying a menu specific to the current pages ancestor (e.g. a permanent level 2 sub menu in a side col) could work.

[stof]

@petesiss the issue is that as soon as you allow multiple current item, you cannot get the current item anymore as this does not make sense (which one is the current one among the different current item ?)

@dbu the issue for the breadcumb would be the way you order the items to use the first one. It depends of the way you traverse the tree. With the following tree, assuming that C,D and G are marked as current items, the order in which they will be found (assuming we don't stop traversing the tree when finding the first one) would be D, C, G:

        A
  B           C
D    E      F    G

[dbu]

@stof: yes, i see that.

the question is: what can we do if we want a breadcrumb? (which is something used on many sites and imo a valid use case)

just take the "first" one. will work whenever you actually have just one current item. i would be ok to call that render_first_breadcrumb('menuname'). and maybe keep the current item method and call it getFirstCurrentItem() to make it dead clear to everybody that there might be more. we can document whether its a depth first search or something else (or even undetermined, i think about handling the common use case. getOneCurrentItem would be fine too)

users who have a valid use case for multiple current items will need their own traversal anyways because they need to do special things. but it would be sad to make it very awkward to build a breadcrumb just to allow for a special case.

[stof]

Well, the current traversal is the easiest solution (it loops over the children and calls the method on the child) so if we decide to keep the method with a different name, I won't change the way we traverse the tree. I don't even want to imagine how many loops would be needed for a traversal per level.

[petesiss]

How about allowing to identify one "real" item amongst a wider set of items that share a URI?

You would only need to specify this in the case where you wanted to reuse a URI and so create confusion over the true current item.

E.g. In the case of a level 1 page directing to its first child, the first child would be flagged the real item, so would be used for breadcrumbs etc.

I only keep reusing that example because I cant think of any other typical use cases.

[petesiss]

Or would you consider allowing any menu items which duplicate the URI to be defined as aliases, so they take the name of another menuItem, rather than the URI itself.

That way, the currentItem matching could ignore them, so there would only ever be one true current item.

So then the getCurentItem stops lying, but there is still a way to use the same URI over multiple menuItems.

[stof]

@petesiss the point is that you could set an item as current manually if you want. There is a ->setCurrent() method. So you can have as many current item as you want (they could even all be current if you want).

And taking the name of another item instead of the uri does not make any sense. getUri must return an uri, otherwise the link will be broken. And this would not help anyway as the name does not allow you to get the item. It is only unique among the direct children of an item, not among the whole tree

[dbu]

when people create menu items with the same uri, the getOneCurrentItem would indeed go an unexpected way. even if people setCurrent manually, it could happen that another item matching the current uri will be returned as current item before that item.

in the cmf context, we could work around that by manually calling setCurrent(false) on all items except the current one. not sure if that is doable for everybody. so i still think getOneCurrentItem has some value, but we need to well explain the limitations to the users (and probably some people will still not understand it and be confused when their breadcrumb does not show the full path in the parent-menuitem-links-to-same-as-child scenario)

[petesiss]

Yeah - I know that you can have multiple current items and set them manually with ->setCurrent() . But thats what this discussion is about I guess - whether that is appropriate as per your option 1.

Re the alias idea, of course ->getUri() would return the URI of the target item in the case it was an alias. That is what I would have assumed. Re the name - yes - would need to be the path I guess to make it unique.

[stof]

@petesiss I really don't like this alias idea. the uri is not meant to be a unique identifier of the item, and the item should not depend on other items to get its uri. It would be totally inefficient (you would have to traverse it to the root and then back to the item). And it would also be really easy to break it: add the root item inside another item and the path will need to change; and remove the other item of the tree and the alias will become broken.

[petesiss]

@stof Ok - your concerns make sense.

Im just keen now to come with a way of avoiding multiple current items as having templates getting used all over a large app yet not being able to determine what actual menuItem is current will present all kinds of troubles, eg #26

But if uri's can not be a unique identfier, yet remain our only identifier for matching... then conceptually there may be no alternative.

Do we know what all these use cases for not making URI's unique?

Sorry for suddenly spamming all your KnpMenu tickets this last week. Im working with it all day every day at the moment at work, so all these discussions feel very relevant.

[stof]

well, you can have have 2 different items linking to the same page. Think for instance about a menu containing links for all articles of a blog, organized by categories. As soon as an article is associated to 2 categories, it will appear twice in the menu.

[uwej711]

Or a shop application where a product is listed in several categories. Usually you want to display either the breadcrumb corresponding to the way the user took to the page or some "primary" or "canonical" breadcrumb (whatever that is determines your application ...).
But the current item is not only handy for the breadcrumb but also for highlighting the currently selected menu.

[dbu]

Re: highlight: if i understand correctly, this discussion is not about removing isCurrent, only getCurrentItem(). the menu will still render any current items with the current css class, and isCurrentAncestor would still be used to mark the navigation path(s) to the current item(s). correct, stof?

i would go with the imperfect case of getOneCurrentItem and let it up to the user of the lib to ensure the right item is considered current when in a situation that leads to more than one current item, i.e. using setCurrent(false) on shortcut menu entries or things like that. (the uri magic only triggers if setCurrent was not called on the item). in the shop, the breadcrumb is probably based on the users activity trail or otherwise determined on session history, and thus out of scope for this menu library. i feel we can keep it simple in the menu and just need to make the users understand the limitations.

[stof]

@dbu I never ever throught about removing isCurrent from the object. This method is useful and used by the renderers whereas getCurrentItem is never used by KnpMenu itself and is probably coming from some legacy stuff.

[stof]

I removed the method in #49 as it does not make sense anymore once moving the matching to an external class. However, see the discussion on #50 as one goal is to provide a replacement (getting all current items instead of only one)

[peterrehm]

@stof How would you recommend now to figure out the current item to build a breadcrumb menu? For me it seems more difficult with the removed functionality

[peterrehm]

@stof I have figures it out according to KnpLabs/KnpMenuBundle#122 (comment) - but like in this code I have a function for the mainMenu and another one just for the porpose to figure out the current item to use it in the breadcrumb. Obviously still the function might be lying but I can ensure that there wont be duplicates in the tree. Is there any other way to get this more performant or is this the best solution?