but it's easy to fix. This needs a test but leaving at needs review for the bot.

CommentFileSizeAuthor
#24 1326950.24.patch10.92 KBmiraj9093
#18 drupal8.menu-q.18.patch2.13 KBsun
#17 drupal8.menu-q.17.patch2.09 KBsun
#16 drupal8.menu-q.16.patch2.11 KBsun
#15 1337076_15.patch983 byteschx
#7 1337076-menu-7.patch807 bytesaspilicious
#5 user-wscci-fix.patch817 bytesaspilicious
menu_no_get_q.patch658 byteschx

Comments

chx’s picture

chx commented

Note that menu_get_item will still use $_GET['q'] but that's the only one. This makes menu_set_active_item actually work.

Status: Needs review » Needs work

The last submitted patch, menu_no_get_q.patch, failed testing.

aspilicious’s picture

aspilicious commented
Status: Needs work » Needs review

This is blocking a wscci patch.
Thnx chx for working on this.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work 
+++ b/includes/menu.inc
@@ -2291,7 +2291,7 @@ function menu_get_active_menu_names() {
 function menu_set_active_item($path) {
-  $_GET['q'] = $path;
+  menu_get_item(NULL, menu_get_item($path));

menu_*s*et_item() in the first?

I realize it makes no difference currently, but for API usage intentions' sake...?

aspilicious’s picture

aspilicious commented
Status: Needs work » Needs review
StatusFileSize
new user-wscci-fix.patch817 bytes

Lets try this one.

Don't mind the name, just to trigger the testbot

Status: Needs review » Needs work

The last submitted patch, user-wscci-fix.patch, failed testing.

aspilicious’s picture

aspilicious commented
Status: Needs work » Needs review
StatusFileSize
new 1337076-menu-7.patch807 bytes

Second try

chx’s picture

chx commented

I inlined menu_set_item for speed...

Status: Needs review » Needs work

The last submitted patch, 1337076-menu-7.patch, failed testing.

aspilicious’s picture

aspilicious commented

Ok backtraced the error. It's not a shortcut issue it's a page not found issue.

        $path = drupal_get_normal_path(variable_get('site_404', ''));
        if ($path && $path != $system_path) {
          // Custom 404 handler. Set the active item in case there are tabs to
          // display, or other dependencies on the path.
          menu_set_active_item($path);
          $return = menu_execute_active_handler($path, FALSE);
        }

Menu_get_item(); returns false after this :)

chx’s picture

chx commented

That's only possible really if you dont have access to the $path or it's an invalid $path in the first place.

aspilicious’s picture

aspilicious commented

I'll try to print $path

aspilicious’s picture

aspilicious commented

Ok I spend some hours on backtracking this

So I'm going to summarize now, I have no ide how to fix this.

1) When visiting 'site.com/page-that-does-not-exist' MENU_NOT_FOUND gets triggered in drupal_deliver_html_page // common.inc
2) It does some stuff and than the page gets passed to drupal_render // common.inc
3) Somewhere in drupal render toolbar_pre_render() gets called // toolbar.module
4) If you go a few levels deeper toolbar_in_active_trail() gets called // toolbar.module
5) That function calls menu_get_active_trail() // menu.inc

If you call menu_get_item in menu_get_active_trail it returns false. ==> fail

chx’s picture

chx commented

This ties into #1299056: Remove misnamed and useless menu_set_active_trail() function.

This is like a spectacular mess. To the best my understanding, there is a current router path but there is another path which we want menu links to think we are on. We have an ass-amount of setters and getters around these, completely lost at purpose and direction. We need to clean house very seriously.

chx’s picture

chx commented
Status: Needs work » Needs review
StatusFileSize
new 1337076_15.patch983 bytes

I discussed with sun who pointed out that the preferred link defaults to FALSE. So I have rolled this patch with 4 lines of context because it's nicely visible so.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new drupal8.menu-q.16.patch2.11 KB

- Added comment for inlined menu_set_item().
- Added 'original_href' directly in menu_get_item().

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new drupal8.menu-q.17.patch2.09 KB

- Replaced that implode() with simply $path.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new drupal8.menu-q.18.patch2.13 KB

Provide original_href to _menu_translate() already.

pwolanin’s picture

pwolanin commented

sure, the fewer direct references to $_GET the better

aspilicious’s picture

aspilicious commented
Title: Menu is $_GET['q'] bound unnecessarily » Clean up the many active / preferred things in menu API

Changing title

xjm’s picture

xjm
she/her
Primary language English
commented

Tangentially related: #1811792: drupal_set_title() should not be necessary to set the homepage title

areke’s picture

areke commented
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll

The patch needs to be re-rolled as it doesn't apply anymore

pwolanin’s picture

pwolanin commented
Status: Needs work » Postponed

Should wait until #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit is done

miraj9093’s picture

miraj9093 commented
Issue tags: -Needs reroll +Needs Review
StatusFileSize
new 1326950.24.patch10.92 KB

patch re-rolled ! I am updating this issue after long time , some changes making the patch bigger in size.

miraj9093’s picture

miraj9093 commented

i think its right time to work on this issue now..

dawehner’s picture

dawehner
Primary language German
commented

Note: We would actually have to convert this function to use a route name + route parameters instead of using a path.

miraj9093’s picture

miraj9093 commented
Status: Postponed » Active

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

quietone commented
Status: Active » Closed (outdated)
Issue tags: - +Bug Smash Initiative

The menu link system was changed in #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module and child issues in 2014.

Therefore, closing as outdated. If this is incorrect reopen the issue, by setting the status to 'Active', and add a comment explaining what still needs to be done.

Thanks!

quietone’s picture

quietone commented