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.Problem/Motivation
When a logged-in user requests a new password on '/user/password', the page shows an additional tab 'Log in'. This tab should not be available on this page, as the user is already logged in. This behavior is specific for Drupal 8 and is therefor a regression.
For logged-out users, the tab 'Log in' is missing as well.
Proposed resolution
@dinarcon #5 / #6
The extra tab is defined by the user.page plugin at user.links.task.yml. It has a base_route of user.page. The latter has a requirement of _user_is_logged_in: 'TRUE', at user.routing.yml, making the task available for logged in users. Having a 'Log in' link only available for logged in users is odd.
To mimic D7's behaviour, in user.links.task.yml the user.page plugin definition should be removed and and the user.login plugin definition should be changed to:
user.login:
route_name: user.login
base_route: user.page
title: 'Log in'
weight: -10
Remaining tasks
- Write a patch
- Review
- Add screenshots of the user/password page after patch
The tab 'Log in' should only be available for logged-out users.
User interface changes
The 'Request new password' page shows no tabs by default
API changes
None
Beta phase evaluation
| Issue category | Bug because the display is a regression compared to D7 |
|---|---|
| Issue priority | Normal because the functionality is not impacted |
| Unfrozen changes | Unfrozen because it only changes the display of tabs |
| Prioritized changes | The main goal of this issue is usability. |
Original report by @gaurav_varshney
The issue contanins when a user is logged in and go for "Request New Password" then the page display the "Log In" Link which i think should not for the logged in user.
Proposed Resolution -
we can remove the both tabs (Log in and Request new password).
| Comment | File | Size | Author |
|---|---|---|---|
| #31 | interdiff-31-23.txt | 742 bytes | idebr |
| #29 | log-in-tab-visibility-2303391-29.patch | 2.1 KB | gvso |
| #24 | 2303391-23-after-logged-in.png | 103.31 KB | idebr |
| #24 | 2303391-23-after.png | 83.9 KB | idebr |
| #23 | log-in-tab-visibility-2303391-23.patch | 2.01 KB | gvso |
Comments
Comment #1
gaurav_varshney CreditAttribution: gaurav_varshney commentedComment #2
idebr CreditAttribution: idebr commentedI believe the tabs will not be shown by default if there is only one tab available. This is also the behavior currently in D7:
Comment #3
idebr CreditAttribution: idebr commentedComment #4
AkshayKalose CreditAttribution: AkshayKalose commentedComment #5
dinarcon CreditAttribution: dinarcon commentedIMHO, none of tabs should be shown as proposed by @gaurav_varshney. That is the behaviour in D7 as @idebr shows and, as stated, having a single link task available will hide the tabs.
The extra tab is defined by the
user.pageplugin atuser.links.task.yml. It has abase_routeofuser.page. The latter has a requirement of_user_is_logged_in: 'TRUE', atuser.routing.yml, making the task available for logged in users. Having a 'Log in' link only available for logged in users is odd. In D7, there is a 'Log in' link for this page for anonymous users as seen below. This is not true for D8, but trying to achieve that might have introduced this issue.Comment #6
dinarcon CreditAttribution: dinarcon commentedTo mimic D7's behaviour, in
user.links.task.ymltheuser.pageplugin definition should be removed and and theuser.loginplugin definition should be changed to:Waiting for feedback. Tagging as Novice.
Comment #7
idebr CreditAttribution: idebr commentedUpdated the issue summary with the default template to clear the path forward.
Comment #8
AkshayKalose CreditAttribution: AkshayKalose commentedSuggestion from #6
Comment #9
kporras07 CreditAttribution: kporras07 commentedI think proposed solution in #8 will give us a bad name for the user and password login tab if another authentication method add pages below user/login/
For avoiding this issue and as a proposed solution for this issue I'd only change route_name in user.page in user.links.task.yml to user.login.
Patch attached with this proposed solution. (Screenshots are exactly the same as #8)
Comment #11
AkshayKalose CreditAttribution: AkshayKalose commentedThe above patch seems to be working for me.
Comment #14
kporras07 CreditAttribution: kporras07 commented@AkshayKalose; problem here is about integration tests. As I didn't modified the test; it fails on it. I'll try to correct this ASAP.
Comment #16
vaibhavjainPatch attached, looks to be working good.
Comment #18
vaibhavjainI believe the patch is fine, but it also needs to update the test case.
May be removing user.page with user.login in the file -> UserLocalTasksTest.php -> testUserLoginLocalTasks()
Comment #19
gvsoTest updated
Comment #20
gvsoRemoving extra spaces :)
Comment #23
gvsoComment #24
idebr CreditAttribution: idebr commentedLooks great, thanks everyone!
Screenshot after (logged out):
Screenshot after (logged in):
Comment #25
idebr CreditAttribution: idebr commentedUpdated the issue summary to include the change to the tab 'Log in'.
Comment #26
idebr CreditAttribution: idebr commentedIncluded a beta evaluation in the issue summary.
Comment #27
gvsoThanks @idebr, but as our dataProvider is not returning a second parameter anymore, should we remove the second parameter in the testUserLoginLocalTasks() method and the if statement too?
Comment #28
alexpott@gvso - yep good spot. We can remove the subtask handling from testUserLoginLocalTasks
Comment #29
gvsoThanks @alexpott. Test updated
Comment #30
gvsoComment #31
idebr CreditAttribution: idebr commentedAlright then! I added an interdiff for easier reviewing.
Comment #32
gvsoThanks @idebr, I forgot to do that :)
Comment #33
msound CreditAttribution: msound commentedPatch #29 works. I tested this manually, and ran phpunit for `user` group. RTBC++
Comment #34
webchickNice find and fix. Looks like Alex's feedback was addressed.
Committed and pushed to 8.0.x. Thanks!