backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

Menu link with path `user` is never active when clicked #2413

Open Gormartsen opened 7 years ago

Gormartsen commented 7 years ago

So, if you make Home link in menu pointed to user , it will never be active when you click it.

It happens due to next code:

https://github.com/backdrop/backdrop/blob/1.x/core/modules/user/user.pages.inc#L553

function user_page() {
  global $user;
  if ($user->uid) {
    menu_set_active_item('user/' . $user->uid);
    return menu_execute_active_handler(NULL, FALSE, 'menu_default_route_handler');
  }
  else {
    return backdrop_get_form('user_login');
  }
}

Basically, router_item has path=user/% while link has user

Any suggestions how to fix that? Simply make a hack where link_path user need to be set active if router_item->path == user/% ?


PoC #1 PR by @Gormartsen https://github.com/backdrop/backdrop/pull/1683 PoC #2 PR by @quicksketch https://github.com/backdrop/backdrop/issues/1727

Gormartsen commented 7 years ago

PS: For authorized users.

Gormartsen commented 7 years ago

There is an extra case:

In this case, menu item will not get class active on front page.

Gormartsen commented 7 years ago

PR https://github.com/backdrop/backdrop/pull/1683 is POC patch.

Gormartsen commented 7 years ago

I am sorry if I used labels not properly! canadian way eh?

klonos commented 7 years ago

Labels are just right @Gormartsen 👍

quicksketch commented 7 years ago

I can't seem to find the other issue I believe exists, but I think we proposed at one point that we simply remove the menu item at "/user" and turn it into a redirect instead. If logged in, redirect to user/%. If logged out, redirect to /user/login. There's also an issue to remove the profile page entirely at #109.

But, if we were to take that approach, no special handling of /user would really be necessary. Although, this problem would still exist if you were to put /user in the menu itself. Thoughts?

Gormartsen commented 7 years ago

Yes, if profile page will be removed entirely, there is no need to do so.

Please check commit: https://github.com/backdrop/backdrop/commit/b3cf0681406832ea677d21beb438624a746b1622

It was commit, where module node became optional and site_frontpage became user by default.

So if user will be removed, all default values for site_frontpage need to be updated as well.

quicksketch commented 7 years ago

I'm not comfortable with making exceptions in the menu system itself for the weird /user path, but I think we could avoid this problem entirely just by fixing the menu callback user_page() to restore the original menu path after rendering the profile page.

I filed a PR at https://github.com/backdrop/backdrop/pull/1727. @Gormartsen could you try that out?

Gormartsen commented 7 years ago

@quicksketch tested. See http://1727.backdrop.backdrop.qa.backdropcms.org

I changed QA sandbox to next settings:

Results:

anonymous user:

authorised user:

Gormartsen commented 7 years ago

I'm not comfortable with making exceptions

I understand it. Another way to fix it is completely rewrite user_menu by adding user/edit user/view items (for tabs on my account page) and remove menu_execute_active_handler from user_page()

quicksketch commented 7 years ago

Results:

Well, rats. Not working quite yet then. Let's remove this from the bug fix release and keep working on suitable solutions.