Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Let's collection the blocking issues. It is kind of pointless to assign an issue to yourself if you can't work on it in like a month (common estimation from my experience).

tim.plunkett’s picture

Title: Remove {menu_router} » [meta] Remove {menu_router}
Assigned: tim.plunkett » Unassigned

Yeah the first two blockers are contextual and theme callback, as you added.

This also means we have to kill every usage of menu_get_item() :(

dawehner’s picture

Let's be honest ... we need to kill menu.inc first before we can get rid of {menu_router}.

Some other potential blockers might be the book module.

xjm’s picture

Title: [meta] Remove {menu_router} » [meta] Remove {menu_router} and the hook_menu() 'page callback' key
xjm’s picture

Issue summary: View changes

...

dawehner’s picture

Let's at least already try to reduce the menu_router table.

webchick’s picture

Issue tags: +8.x-alpha4

It may be a long shot, but it would be SO NICE to have this done by alpha4. (next Friday)

tim.plunkett’s picture

This is a meta, not a real actionable issue, and I don't think all of the sub issues in the issue summary will be done by then...

catch’s picture

Status: Active » Postponed
Issue tags: -8.x-alpha4

Yes there's no chance of this being done by alpha 4. Removing tag and marking postponed so it's explicit.

catch’s picture

Issue summary: View changes

.

xjm’s picture

Issue summary: View changes
Issue tags: +beta blocker
tim.plunkett’s picture

Title: [meta] Remove {menu_router} and the hook_menu() 'page callback' key » [meta] Remove {menu_router}
tim.plunkett’s picture

Issue summary: View changes
tim.plunkett’s picture

Title: [meta] Remove {menu_router} » Remove {menu_router}
Parent issue: » #1889790: [Meta] Allow modules to register links for menus, breadcrumbs and tabs (if not with hook_menu)

Moving this back to an actionable postponed issue, so we only have one meta.

xjm’s picture

Issue summary: View changes
dawehner’s picture

pwolanin’s picture

Issue summary: View changes
pwolanin’s picture

Issue summary: View changes
webchick’s picture

Status: Postponed » Active

#2177031: Remove menu_get_item() and every use of it. was just committed. I *believe* that now unpostpones this?

dawehner’s picture

Status: Active » Postponed

#2177041: Remove all implementations of hook_menu is certainly one issue we need and I doubt that we don't need many more.

xjm’s picture

@dawehner, do you mean that there are missing beta blockers that still need to be filed? It would be helpful to get an outline if so in order to help find resources for them.

tim.plunkett’s picture

Here's a patch that depends on the hook_menu removal. I've attached a combined one for now.

tim.plunkett’s picture

Status: Postponed » Needs review

Let's see how the combined one does.

EDIT: Stupid mistake. Moving this to a patch testing issue until hook_menu is in.

dawehner’s picture

Status: Needs work » Postponed

Yeah clearly we cannot just rip it out yet. One issue we also need is the book one, which decouples the menu_router and the book subsystem.
This is the reason why I just think that we cannot rip it out immediately and we need to break up the remaining parts into pieces.

tim.plunkett’s picture

Status: Postponed » Needs review
FileSize
172.12 KB
109.88 KB

Actually, book is just heavily tied to menu links, not the router.
I got this to pass, here's the full combined and split one again.

This patch's diffstat: 45 files changed, 104 insertions(+), 2107 deletions(-)

Berdir’s picture

FileSize
109.88 KB

Re-uploading the do-not-test patch.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
FileSize
17.53 KB
116.98 KB

Per discussion with @dawehner, I reinstated most of the doTestDescriptionMenuItems() test, and I finally removed the menu_item_route_access() helper function.
Also, since the callback and description parts of _menu_item_localize depended on the menu_router, I removed those.
We have the critical #2193777: Menu link translations need to work with content translation, shipped translations need to be compatible to follow-up with that, I'm just removing dead code.

I'm happy with this as is, but assigning myself for fixing any test failures, rerolls, or addressing reviews.

Status: Needs review » Needs work

The last submitted patch, 26: menu_router-2107533-26.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
117 KB
Berdir’s picture

We have the critical #2193777: Menu link translations need to work with content translation, shipped translations need to be compatible to follow-up with that, I'm just removing dead code.

Based on tests that I fixed in the hook_menu() removal issue, that dead code might not be as dead as you'd think. Let's see, there could be fails in the language ui negotiation something test, which indirectly verifies translatability of menu link descriptions...

tim.plunkett’s picture

It's pretty dead. Whether or not it was needed I cannot say, but it was not used. As evidenced by the passing patch.

dawehner’s picture

Nice work!

It's pretty dead. Whether or not it was needed I cannot say, but it was not used. As evidenced by the passing patch.

It was not tested, it does not mean that we did not relied on it.

  1. +++ b/core/includes/menu.inc
    @@ -543,29 +313,12 @@ function _menu_check_access(&$item, $map) {
    +function _menu_item_localize(&$item, $link_translate = FALSE) {
    

    As a former translator I would expect the description to be translated on admin/index

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/RouterRebuildSubscriber.php
    @@ -52,11 +61,39 @@ public function onKernelTerminate(PostResponseEvent $event) {
    +      $transaction = db_transaction();
    +      try {
    +        // Ensure the menu links are up to date.
    +        menu_link_rebuild_defaults();
    +        // Clear the menu, page and block caches.
    +        menu_cache_clear_all();
    +        _menu_clear_page_cache();
    +      }
    +      catch (\Exception $e) {
    +        $transaction->rollback();
    +        watchdog_exception('menu', $e);
    +      }
    

    I wonder whether we need to include the cache clearing into the lock statement?

  3. +++ b/core/modules/basic_auth/lib/Drupal/basic_auth/Authentication/Provider/BasicAuth.php
    @@ -70,7 +70,7 @@ public function cleanup(Request $request) {}
    -    if (user_is_anonymous() && $exception instanceof AccessDeniedHttpException) {
    +    if ($GLOBALS['user']->isAnonymous() && $exception instanceof AccessDeniedHttpException) {
    
    +++ b/core/modules/contact/contact.module
    @@ -228,7 +228,7 @@ function contact_form_user_form_alter(&$form, &$form_state) {
    -  $account_data = !user_is_anonymous() ? \Drupal::service('user.data')->get('contact', $account->id(), 'enabled') : NULL;
    +  $account_data = !\Drupal::currentUser()->isAnonymous() ? \Drupal::service('user.data')->get('contact', $account->id(), 'enabled') : NULL;
    

    Do we need this change?

  4. +++ b/core/modules/content_translation/content_translation.module
    @@ -182,62 +182,6 @@ function content_translation_entity_operation_alter(array &$operations, \Drupal\
    - * Implements hook_menu().
    - *
    - * @todo Split this into route definition and menu link definition. See
    - *   https://drupal.org/node/1987882 and https://drupal.org/node/2047633.
    - */
    -function content_translation_menu() {
    

    We should open a follow up in case we want to keep this functionality.

  5. +++ /dev/null
    @@ -1,60 +0,0 @@
    -  /**
    -   * Tests configuration context when rebuilding the menu router table.
    -   */
    -  public function testMenuRouterRebuildContext() {
    -    // Enter a language context before rebuilding the menu router tables.
    -    \Drupal::configFactory()->setLanguage(language_load('nl'));
    -    menu_router_rebuild();
    -
    -    // Check that the language context was not used for building the menu item.
    -    $menu_items = \Drupal::entityManager()->getStorageController('menu_link')->loadByProperties(array('route_name' => 'menu_test.context'));
    -    $menu_item = reset($menu_items);
    -    $this->assertTrue($menu_item['link_title'] == 'English', 'Config context overrides are ignored when rebuilding menu router items.');
    -  }
    -
    -}
    

    This is an important piece of functionality. We want to ensure that the menu link_title has the proper language context. Note: This tests menu_links_defaults_rebuild() or how it is called.

  6. +++ b/core/modules/system/lib/Drupal/system/Tests/System/AdminTest.php
    @@ -48,19 +48,6 @@ function setUp() {
       function testAdminPages() {
    -    // Go to Administration.
    -    $this->drupalGet('admin');
    -
    -    // Verify that all visible, top-level administration links are listed on
    -    // the main administration page.
    -    foreach (menu_get_router() as $path => $item) {
    -      if (strpos($path, 'admin/') === 0 && ($item['type'] & MENU_VISIBLE_IN_TREE) && $item['_number_parts'] == 2) {
    -        $this->assertLink($item['title']);
    -        $this->assertLinkByHref($path);
    -        $this->assertText($item['description']);
    -      }
    -    }
    

    Isn't that a totally valid test? We should maybe replace menu_get_router()

  7. +++ b/core/modules/system/system.admin.inc
    @@ -30,12 +30,7 @@ function system_theme_default() {
    +      // Rebuild the menu.
           \Drupal::service('router.builder')->setRebuildNeeded();
    

    Actually we rebuild the router. ... kinda pointless comment to be honest.

  8. +++ b/core/modules/user/lib/Drupal/user/Access/RegisterAccessCheck.php
    @@ -21,6 +21,6 @@ class RegisterAccessCheck implements AccessInterface {
    -    return ($account->isAnonymous() && (\Drupal::config('user.settings')->get('register') != USER_REGISTER_ADMINISTRATORS_ONLY)) ? static::ALLOW : static::DENY;
    +    return ($request->attributes->get('_menu_admin') || $account->isAnonymous()) && (\Drupal::config('user.settings')->get('register') != USER_REGISTER_ADMINISTRATORS_ONLY) ? static::ALLOW : static::DENY;
       }
    

    Is there a reason why we did not needed the _menu_admin flag before?

  9. +++ b/core/modules/user/lib/Drupal/user/EventSubscriber/MaintenanceModeSubscriber.php
    @@ -37,7 +37,7 @@ public function onKernelRequestMaintenance(GetResponseEvent $event) {
    -      if (user_is_anonymous()) {
    +      if ($user->isAnonymous()) {
    

    This also feels a bit out of scope.

  10. +++ b/core/modules/views/lib/Drupal/views/Tests/Handler/FilterNumericTest.php
    @@ -39,7 +39,7 @@ public static function getInfo() {
     
    -    $this->installSchema('system', array('menu_router', 'key_value_expire'));
    +    $this->installSchema('system', array('key_value_expire'));
       }
    

    <3

tim.plunkett’s picture

1) The description on /admin/index, or on any of the pages using SystemManager::getAdminBlock() like /admin, is not affected by _menu_item_localize().
It is pulled out of $link['localized_options']['attributes']['title'];, menu links don't have a 'description'.

2) This is just moved code, I'm not changing this around.

3) We need this change because I'm removing user_is_anonymous():

function user_is_anonymous() {
  // Menu administrators can see items for anonymous when administering.
  return $GLOBALS['user']->isAnonymous() || !empty($GLOBALS['menu_admin']);
}

I'm removing that because I'm killing that global.

5) I can add back the test, but menu_router_rebuild() no longer exists...

6) I have no idea how to rewrite this test now that menu_get_router() is gone

7) Removed

8) This was a sloppy conversion by whoever swapped out user_is_anonymous(). I can add a test for this.

9) See 4.

tim.plunkett’s picture

FileSize
117.44 KB
5.08 KB

Addressing 5,6,7.

pwolanin’s picture

FileSize
5.31 KB
119.95 KB

Cleanup of _menu_item_localize() to try to make it do something useful

pwolanin’s picture

tim.plunkett’s picture

That change looks good to me, +1

tim.plunkett’s picture

This is big enough to be a pain to reroll, but it also shouldn't stop too much from going in (hopefully no one else is changing menu_router code!).
Will retest next, but it'd be great to get another (or final?) review on this.

This could probably get a dedicated change notice for the few functions that are removed and as an overall announcement, but mostly we just need to update existing change records.

tim.plunkett’s picture

Issue tags: -Needs change record

Nevermind, we can just add https://drupal.org/node/2203305 to the list of change records to update/expand.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
122.41 KB

We can remove a few more constants. I hope I don't kill kittens with doing that here.

I just don't get the avoid commit conflicts tag. All patches should be kinda treated equally.

dawehner’s picture

FileSize
2.65 KB

Missing interdiff.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

HELL YES!

Dancing smiley face

Committed and pushed to 8.x. YAYYYYY. :) Great work, folks!!

ianthomas_uk’s picture

Title: Remove {menu_router} » Change Record: Remove {menu_router}
Assigned: tim.plunkett » Unassigned
Priority: Critical » Major
Status: Fixed » Active
Issue tags: +Missing change record

We need new or updated change records for at least some of the removed functions:

user_is_anonymous()
user_is_logged_in() (procedural replacement \Drupal::currentUser()->isAuthenticated())
user_register_access()
user_uid_only_optional_to_arg()
user_uid_optional_load()
user_uid_optional_to_arg()
menu_get_ancestors()
menu_unserialize()
menu_tail_to_arg()
menu_tail_load()
there are many more

If there is already a change record for these functions, then the function name should be added to it explictly so that it shows up in searches when people get fatal errors from old code.

https://drupal.org/node/2044515 needs updating (user_is_anonymous, user_register_access)
https://drupal.org/node/2031999 may be relevant
https://drupal.org/node/2203305 was mentioned in #38

Berdir’s picture

_load() and _to_arg() are menu load callbacks, those aren't called directly.

The user_ functions were mostly used as access callbacks, but there's probably some code that was calling user_is_anonymous()/user_is_logged_in() directly, could be added to the AccountInterface change record?

menu_get_ancestors() was an internal helper function, if there really was code somewhere that relied on it, it likely needs to be completely rewritten for 8.x and you won't even get as far as calling this :) menu_unserialize() even more so.

https://drupal.org/node/2044515 only mentions those functions in the old code, that does not need to be updated :)

Crell’s picture

Title: Change Record: Remove {menu_router} » Remove {menu_router}
Status: Active » Fixed

We talked about this in the WSCCI meeting today. We don't believe this warrants a separate change notice. The *entire* routing system is different. There's already a change notice that says "everything you knew about the old routing system is gone, here's how it works now, don't expect it to work." Documenting individual functions as missing now is a waste of time better spent on for-reals documentation.

dawehner’s picture

Yeah in case we would add all this tiny change records here, the confusion would be worse than the potential benifits from it.

xjm’s picture

Issue tags: -Missing change record
kfritsche’s picture

I stumbled across a old change record - https://drupal.org/node/1561492 - which is now not valid anymore, as menu_router_rebuild() is removed by this issue.
What should happen with this change record then? unpublish? update? or just link to the big routing change record?

Crell’s picture

IMO unpublish.

xjm’s picture

Did both. :) However, there's no reference to menu_rebuild() on https://drupal.org/node/1800686. We probably need to add it to a list of functions removed there?

xjm’s picture

Priority: Major » Critical

Somehow the priority didn't get set back to critical. :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.