Posted by shp on April 23, 2012 at 6:50pm
29 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | menu system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | accessibility, DrupalWTF, LONDON_2013_APRIL, menu system, Needs tests |
Issue Summary
It's required when using multilevel menus. The top level items should not be <a href="..."></a> links - just <div class="..."></div> (or span). They must not to lead to any path - just open some dynamic menu.
Comments
#1
+1.
Searching for how to allow empty paths reveals that this is a popular requested feature, especially to accomplish multi level menu parent items that should lead to nowhere, just contain more items.
I tried modifying drupal_valid_path to accept "#" as a valid path, but didn't worked. Menu item is not displayed in the menu item list admin page (however, it is saved in the DB). I can't figure it out with my current core knowledge. Anyway, the # path may not be the best solution.
Also tagging to make it more visible (added "DrupalWTF" tag because seems such an obvious feature to me).
It's a must have to allow multilevel menus, and ML menus are needed in lots of scenarios, IMO.
Related modules:
https://drupal.org/project/special_menu_items
https://drupal.org/project/void_menu
#2
In my opinion "
<none>" is better than "#".P. S. The extension of the patch must be ".patch" and the status of the issue must be "needs review" to be patch automatically tested.
#3
Searching around later I found also
<none>as a possible path, and I like it more, too.I attached the patch in txt form to avoid the testbot go through it, since I actually know it is failing and it was just a "trying luck" quick attempt :)
#4
Definitely seems that going the
<none>way would be safer. Using # may conflict with several issues:#123103: Retain #anchors during path alias -> normal path saving
#325533: Allow <current>#fragment as a menu path
#759808: Disallow # character in path aliases
#5
Curios why this is tagged with accessibility? The accessibility issue hasn't been defined.
#6
I'm the maintainer of special menu items. I think it would be better to create a more extensible system using token. The basic idea is module can register a function than return the link content for token like , , ...
See #1287610: Create a Menu Item Type API
#7
Looks like there is a patch here, although it's needs work. Marking appropriately. We would also need tests for this.
#8
bumping the bot as this should be tested. It applies nicely in .git, but not sure if it needs the .patch extension.
#9
basically it's #1.
#10
The last submitted patch, allow-empty-menu-path-9.patch, failed testing.
#11
#9: allow-empty-menu-path-9.patch queued for re-testing.
#12
The last submitted patch, allow-empty-menu-path-9.patch, failed testing.
#13
With the following patch the menu get displayed but it behave like the
<front>. I tried to modify the theme_menu_link function but it doesn't have any effect, certainly a little piece I'm missing.#14
+1 for "
<none>" instead of "#"!#15
Also agree that the path should be "", rather than "#"
#16
With the following patch if a menu item as a path
<none>it will not be outputted as a link but simply as text. I believe some work is still needed but it should be a good starting point.edit: Here are some things missing:
<none>special path just like the<front><none>path we should automatically expend itI certainly forgot some other remaining tasks so if you have other ideas feel free to add them.
#17
Hi, just saw this from another module.
This would be a great solution to this problem. I havn't tried out this patch yet, however I was wondering if it still creates an <a></a> tag, because when applying styles if there is no <a> tag in e.g. a main menu, when it comes to creating interactive menus, not having the <a> tag can conflict with some menu styling techniques.
#18
@AaronMcH: The patch in #16 simply remove the < a > tag if a the link is < none >.
Here is a new patch which adds a < span > around the element title. This should resolve the possible styling issue I hope. The only possibility that I see with the < a > tag would be to make an href="#", but that would break javascripts relying on the url to define a state for instance.
#19
#20
The problem that I am having is that "#" isn't recognised as a path when trying to create a new menu item. I wonder if you could make it configurable under the settings for that menu, as I could think of a number of possible HTML tags that a person might want to use.
That actually give me another idea, what if "<none>" just removed the "<a>" tag from the output, However the user could use say "<tag>p class="note"" or "<tag>a href="#"", where anything after the "<tag>" would be wrapped around "<>"e; in the output, and at the end the closing tag would be inserted.
What do you think?
Thanks
#21
#19: allow_empty_menu_path-1543750-18.patch queued for re-testing.
#22
#19: allow_empty_menu_path-1543750-18.patch queued for re-testing.
#23
The last submitted patch, allow_empty_menu_path-1543750-18.patch, failed testing.
#24
Re-rolled
#25
The last submitted patch, allow_empty_menu_path-1543750-24.patch, failed testing.
#26
needed to fire menu minipanels if wanting to use click trigger instead of hover
#27
Related issue #916388: Convert menu links into entities
#28
#24: allow_empty_menu_path-1543750-24.patch queued for re-testing.
#29
The last submitted patch, allow_empty_menu_path-1543750-24.patch, failed testing.
#30
Just a re-roll, but I'm getting an error with
PHP Fatal error: Cannot redeclare menu_link_load()that might be related to another patch. Not sure.#31
#30: allow_empty_menu_path-1543750-30.patch queued for re-testing.
#32
The last submitted patch, allow_empty_menu_path-1543750-30.patch, failed testing.
#33
I see no code around that redeclares menu_item_load()
#34
+++ b/core/includes/common.inc@@ -2094,7 +2094,7 @@ function url($path = NULL, array $options = array()) {
// The special path '<front>' links to the default front page.
...
+ if ($path == '<front>' || $path == '<none>') {
The comment should be updated.
#35
Rerolling patch #30 with the help from folks at the Drupal Drop In Sprint - London February 2013
#36
+++ b/core/includes/common.incundefined@@ -2118,7 +2118,7 @@ function url($path = NULL, array $options = array()) {
// The special path '<front>' links to the default front page.
- if ($path == '<front>') {
+ if ($path == '<front>' || $path == '<none>') {
#34 not addressed
+++ b/core/includes/theme.incundefined@@ -1801,7 +1801,13 @@ function theme_links($variables) {
- $item = l($link['title'], $link['href'], $link);
+ if ($link['href'] != '<none>') {
+ $item = l($link['title'], $link['href'], $link);
+ }
+ else {
+ $item = '<span>' . $link['title'] . '</span>';
+ }
+
wrong code indent
+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.phpundefined@@ -143,7 +143,7 @@ public function save(EntityInterface $entity) {
// This is the easiest way to handle the unique internal path '<front>',
// since a path marked as external does not need to match a router path.
- $entity->external = (url_is_external($entity->link_path) || $entity->link_path == '<front>') ? 1 : 0;
+ $entity->external = (url_is_external($entity->link_path) || $entity->link_path == '<front>' || $entity->link_path == '<none>') ? 1 : 0;
this comment should be changed as #34 suggests too
#37
Rerolled patch #35 based on @andypost's feedback.
#38
Great! Now it needs test coverage for RTBC
#39
+1 for RTBC
i tested on latest d8 build and works like a charm
#40
#41
See #38 - it needs automated tests.
#42
I tried to add some tests but I'm not sure if I made anything right. It works but the test may belong to another file, and the tests are probably too specific.
#43
The last submitted patch, allow_empty_menu_path-1543750-42.patch, failed testing.
#44
#42: allow_empty_menu_path-1543750-42.patch queued for re-testing.
#45
that works for path
<none>when I added a new menu item to the footer - admin/structure/menu/manage/footerI think someone should look over the tests and mark it RTBC.
#46
Combined the 2 patches from #42.
#47
+++ b/core/includes/common.incundefined@@ -1811,6 +1812,10 @@ function url($path = NULL, array $options = array()) {
+ else if ($path == '<none>') {
The drupal code style is a bit odd here: it should be "elseif"
+++ b/core/includes/path.incundefined@@ -199,7 +199,7 @@ function drupal_valid_path($path, $dynamic_allowed = FALSE) {
+ if ($path == '<front>' || $path == '<none>' || url_is_external($path)) {
Maybe it's easier to read when it's using an in_array($path, array('http://drupal.org/node/1354 there should be an empty lines after the class.
+++ b/core/modules/system/lib/Drupal/system/Tests/Menu/EmptyLinkPathTest.phpundefined@@ -0,0 +1,70 @@
+ function testEmptyLinkPath($module = 'menu_test') {
it's odd to have a parameter. I don't think that something calls the test method with a parameter :) .. Additional this method should be marked as public.
#48
It doesn't happen a lot but I had a usecase where I had to create a "#something" path (yes, an internal page link) on a menu item. But drupal doesn't allow that.
Probably another issue but I would see this solved too.
#49
Updated patch #46 based on @dawehner's 1st and 3rd suggestions.
@aspilicious, this patch will not allow you to enter "#something" as a menu path. Sorry to disappoint you :-(
#50
I'd be interested in that too. Separate issue then?
#51
@aspillicious @klonos: comment #4 has links to issues that may be related.
#52
+++ b/core/modules/system/lib/Drupal/system/Tests/Menu/EmptyLinkPathTest.phpundefined@@ -0,0 +1,69 @@
+class EmptyLinkPathTest extends WebTestBase {
Is there a reason why we can't use DrupalUnitTestBase here?
#53
> Is there a reason why we can't use DrupalUnitTestBase here?
Because url(), drupal_valid_path(), etc. hits the database. The relevant tables will be absent during unit test runs.