Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Imagine you are having a panel page with the path node/%/details (with % as node_id).
Now you create a menu item for node/%/details (doesn't matter whether as a regular item or a local task).
Result: The menu item is /only/ shown when visiting node/1/details (resp. node/2/details...). It is /not/ shown when you are on node/1 (resp. node/2...) or on node/1/foo (resp. node/2/foo...).
Actually, I would also expect the menu entry on node/1 or node/1/foo, and this should not be a problem, as we have all context we need.
Imagine, you want node/%/details to be some additional information to the node, and you want it to be rendered as an additional tab.
Now, if you're on node/%, how can you find the additional tab if it is not displayed?
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | panels_page_menu_partially_match_4.patch | 3.82 KB | Pancho |
| #1 | panels_page_menu_partially_match.patch | 3.85 KB | Pancho |
Comments
Comment #1
PanchoThis is my first take on the whole menu item rewrite task.
Enclosed patch fixes this and shows non-cached menu items always if the appropriate context is given.
This IMHO means that: all fixed parts that come before the last %-wildcard need to match, while trailing fixed parts don't matter.
So the menu item for foo/%/bar/%/test will be shown on:
- foo/1/bar/1
- foo/1/bar/1/test
- foo/1/bar/1/test/1
- foo/1/bar/1/edit
- foo/2/bar/1/notest
- foo/2/bar/1/anothertest/1
- foo/1/bar/2008/yetanothertest/1
as in all these cases the fixed parts that come before the last %-wildcard do match.
...
But it won't be shown on:
- foo/1/notbar/1/test
- notfoo/1/bar/1/test
...
I somewhat tested this, and found the behavior appropriate, but please think over this again, whether there are cases where this may be not wanted. From my point of view it should be just right though.
Comment #2
PanchoComment #3
sdboyer CreditAttribution: sdboyer commentedI definitely think this is an avenue worth pursuing. I can't think of a specific example, but I can absolutely generally imagine cases where we would NOT want the sub-tab generating behavior to happen; whether or not that behavior is used should be determined on panel_page-by-panel_page basis. Fortunately, it should only require the addition of a single checkbox to the form where the panel_page's path is set - just ask if they'd like to have tabs automatically generated for sub-items in the path, or not, then store that into the db (maybe just a new 'create_subtabs' field), then check against that value when deciding which behavior to use in hook_menu().
If we do use a new field, make sure that the current behavior corresponds to '0', as we really don't want the fun and games of $pane->shown to happen all over again.
Comment #4
PanchoThinking over this issue again strengthened my opinion. Given, that I don't miss an important point, I'm opposed to making this optional. Aside from usability (yet another option), it needs to be noted that this is not a feature, but a bugfix. With the patch applied, Panels implements the menu system exactly like core does and like all other modules do.
Suppose you are on the path foo/1/bar/1, and for this path there are the menu subitems (tabs or whatever) foo/1/bar/1/test and foo/1/bar/1/notest. What should happen to them? Obviously they need to be rendered, otherwise their existence would be hidden. If they are local tasks they need to be rendered as tabs, if they are menu items they need to be listed as children in the menu. Same applies to siblings.
And IMHO this is no way optional. Even if there was a case where this is wanted: Any deviation from this behaviour would constitute a serious UI consistency break in Drupal menus. And that we don't want.
Patch did not apply anymore, so I rerolled it with only some minor comment changes.
Comment #5
sdboyer CreditAttribution: sdboyer commentedI'm going to take a look at the patch in action before I respond, I may be misunderstanding the thrust of the changes.
Comment #6
sdboyer CreditAttribution: sdboyer commentedOK, so, from a different angle - I've stepped through the changes in that patch, and I don't see the difference that they're supposed to make. Where's the step where we take additional trailing args into consideration? Or has that part just not been written yet?
A bigger issue, though - or not, depending on how you want to look at it - is that as far as I can tell, panels_pages actually already does this. I tested it on 4b by overriding a group nid on my og_panels local sandbox, since og_panels creates a path sub-structure that follows the nid. it handled it without any problem, and all the other fields showed up as they're supposed to. In fact, now that I'm thinkin about it, I'm realizing that this problem was accounted for and solved a while ago - merlin made the choice to, when there were other local task tabs present, simply provide a single "Settings" tab as a way to get into the normal panels_page editing interface.
So, unless I've misunderstood...it seems like we already have what you're going for.
Comment #7
PanchoUh, would be very surprised if that was the case, but I'll check it all over next week. Thanks for reviewing anyway!
Comment #8
sdboyer CreditAttribution: sdboyer commentedI'd suggest trying it with OG Panels in the course of your testing - I'm pretty sure it's right along the lines of what you're talking about, and it's pretty easy to get set up.
Comment #9
merlinofchaos CreditAttribution: merlinofchaos commentedCommitted! Thanks Pancho!
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.