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
Back to site links to wrong path in case the last path visited in *Page not found* page.
Steps to reproduce
- Login as an admin
- Visit a page that does exist - for example the front page.
- Visit any page which does not exist: For example:
http://example.com/not-found
- Goto any page that exist, for example Modules page
- Goto any other page that also exists, for example the config page
- Click *Back to site*. It links to 404 page in step 2, instead of page in step 5
Proposed resolution
Remaining Tasks
- Write patch
- Test, test and test
API Changes
None
Comment | File | Size | Author |
---|---|---|---|
#63 | interdiff_62-63.txt | 322 bytes | raman.b |
#63 | 2600652-63.patch | 12.49 KB | raman.b |
#62 | interdiff_60-62.txt | 6.83 KB | raman.b |
#62 | 2600652-62.patch | 12.63 KB | raman.b |
#60 | interdiff_58-60.txt | 818 bytes | snehalgaikwad |
Comments
Comment #2
dawehnerThis is totally an interesting bug.
Comment #3
nod_For resolution see what was done in the block overview page, there is no need to change the JS file, only adding a setting in drupalSettings is sufficient.
Comment #4
a_thakur CreditAttribution: a_thakur as a volunteer and at Srijan | A Material+ Company commenteddrupalSettings.path has currentPath variable which has value system/404, can we use this or is it better to add a new variable for this?
Comment #5
a_thakur CreditAttribution: a_thakur as a volunteer and at Srijan | A Material+ Company commentedPlease find the attached patch which fixes the issue, instead of adding a new variable to drupalSettings.path for 404 pages, we can instead use currenPath instead which has value 'system/404' for page not found.
Comment #6
nod_As I said, no need to change the JS file, see what's done in
Drupal\block\Controller::demo
.It's not strictly an admin page but sort of a "system" page. Either way, it'd solve the issue. If we start adding random stuff in the conditions we'll never see the end of it.
Comment #7
a_thakur CreditAttribution: a_thakur as a volunteer and at Srijan | A Material+ Company commentedThanks _nod for the review, will fix it as suggested.
Comment #8
a_thakur CreditAttribution: a_thakur as a volunteer and at Srijan | A Material+ Company commentedChanges made as suggested by nod_, there was also a typo in escapeAdmin.js file which I have fixed in this patch.
Comment #9
nod_leftover console.log
Comment #10
a_thakur CreditAttribution: a_thakur as a volunteer and at Srijan | A Material+ Company commentedmy bad..fixing it
Comment #11
a_thakur CreditAttribution: a_thakur as a volunteer and at Srijan | A Material+ Company commentedRemoved console.log().
Comment #12
a_thakur CreditAttribution: a_thakur as a volunteer and at Srijan | A Material+ Company commented@nod_: could you please review?
Comment #13
Truptti CreditAttribution: Truptti at Axelerant commentedVerified the patch in #11 and it works fine.Steps performed to test the fix are as follows:
1.Applied the patch
2.Access the site and login as admin (Home page will be displayed http://localhost/drupal/user/1)
3.Navigate to "http://localhost/drupal/admin/config/trees", it will display "Page not found" error
4.Then click on link "Appearance" and then "Content"
5.Click on "Back to Site" link and the flow navigates to home page "http://localhost/drupal/user/1" instead of the error page "http://localhost/drupal/admin/config/trees"
This can be marked as RTBC.
Comment #14
nod_The code is fine with me, the comment could be simplified a bit to be more accurate. Something along the lines of :
"Prevent the "back to site" functionality to save this page as the last non-admin page"
Comment #15
cilefen CreditAttribution: cilefen commentedComment #16
sushantpaste#15 works fine.
Comment #18
alexpottI'm pretty sure this will turn out to be fixed by #2595695: 4xx handling using subrequests: no longer able to vary by URL if that patch addresses currentPath and currentPathIsAdmin in drupalSettings - which I think it should.
Comment #19
Wim LeersAgreed; the patch here doesn't actually solve the problem. It merely treats the symptoms.
Comment #20
alexpottI've just tried to reproduce this problem - I can't. Also it is a bit hard to see what the issue being reported is because the steps to reproduce in the issue summary aren;t complete. But I guess that the bask to site link contained system/404?
Comment #21
alexpottChanging to maintainer needs more info since I can't reproduce.
Comment #22
alexpottAh I see this is a somewhat different issue - the proper steps to reproduce are in #13. So I think the fix of marking system/403 and system/404 as admin paths is wrong and once #2595695: 4xx handling using subrequests: no longer able to vary by URL lands it would actually break this approach. I think the back to site storer should only store urls that are 2xx responses.
But also is this behaviour really incorrect? If a user goes to /new_page_i_want_to_create then uses the toolbar to create something that responses on this url wouldn't you want to be taken back to it?
Comment #23
alexpottWell the node create example is a bad one because when you save the node you get taken to view the node in the site. But I've just manually tested the create view example and it totally works as designed for me.
Comment #24
Wim Leers+1
Comment #25
Wim LeersOops, obviously we can fix that here.
Comment #26
Wim Leers#2595695: 4xx handling using subrequests: no longer able to vary by URL was fixed. So let's do this.
Upon starting to do this, turns out that
is impossible.Hopefully @nod_ or @droplet has some idea.
Comment #27
Wim LeersTo clarify: JS cannot t get the response status AFAICT. Surprisingly.
Comment #28
a_thakur CreditAttribution: a_thakur as a volunteer and at Srijan | A Material+ Company commentedBumping into the issue after a long time..could not look into this earlier..agree with @Wim Leers that previous patch submitted only treats the symptoms..
Comment #29
a_thakur CreditAttribution: a_thakur as a volunteer and at Srijan | A Material+ Company commentedComment #30
a_thakur CreditAttribution: a_thakur as a volunteer and at Srijan | A Material+ Company commentedComment #31
a_thakur CreditAttribution: a_thakur as a volunteer and at Srijan | A Material+ Company commentedComment #32
a_thakur CreditAttribution: a_thakur as a volunteer and at Srijan | A Material+ Company commentedComment #33
a_thakur CreditAttribution: a_thakur as a volunteer and at Srijan | A Material+ Company commentedComment #34
a_thakur CreditAttribution: a_thakur as a volunteer and at Srijan | A Material+ Company commentedComment #35
a_thakur CreditAttribution: a_thakur as a volunteer and at Srijan | A Material+ Company commentedComment #36
a_thakur CreditAttribution: a_thakur as a volunteer and at Srijan | A Material+ Company commentedComment #37
a_thakur CreditAttribution: a_thakur as a volunteer and at Srijan | A Material+ Company commentedComment #38
subhojit777+1
Comment #39
nod_Changed the name from
currentPathIsAdmin
toescapeFromAdmin
. As in "this page needs to show the escapeFromAdmin link". And whenever the link is shown, the script will not save the page as one to go back to.As for saving only 2xx response, unless we add something to the markup or drupalSettings we can't do it as wim said.
Comment #41
alexpottDiscussed with @xjm, @catch, @cottser, and @effulgentsia. We agreed that this is not a major bug - the user is still on their site and whilst it probably makes sense to go back to the last 200 responses there are cases where this could be confusing. For example: you want to edit some block that is appearing on the 404 page and you click on the edit go into the admin screen makes some changes and then click back to site - if we implement this change you won't be going there.
Comment #42
droplet CreditAttribution: droplet commentedIt doesn't a bug IMO. ( but can be improve... )
Comment #43
tim.plunkettComment #46
nod_Need usability team feedback on #41.
It makes sense to me so I'd won't fix this issue.
Comment #52
nod_Need reroll and feedback about usability on #41
Comment #53
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedComment #54
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedRerolled patch for 9.1.x, please review.
Comment #56
snehalgaikwad CreditAttribution: snehalgaikwad as a volunteer and at QED42 for Drupal India Association commentedComment #57
pratik_kambleComment #58
snehalgaikwad CreditAttribution: snehalgaikwad as a volunteer and at QED42 for Drupal India Association commentedComment #60
snehalgaikwad CreditAttribution: snehalgaikwad as a volunteer and at QED42 for Drupal India Association commentedComment #62
raman.b CreditAttribution: raman.b at OpenSense Labs commentedResolves the failed test cases in #60
Comment #63
raman.b CreditAttribution: raman.b at OpenSense Labs commentedRemoves the extra new line added in
core/modules/toolbar/js/escapeAdmin.js
Comment #64
samiullah CreditAttribution: samiullah at Salsa Digital commentedLooks good
Applied patch #63
Steps to Test:
1.Login as an admin
2. Visit a page that does exist - for example the front page.
3. Visit any page which does not exist: For example: http://example.com/not-found
4. Visit any admin page example admin/modules
5. Visit other admin page example admin/config
6. Click on "Back to Site"
Expected: User should navigate back to frontpage
Comment #65
abhisekmazumdarThe patch #63 can be applied on 9.1.x. Also the patch resolves the 404 error page issue.
The patch looks good to me. Moving this to RTBC.
Comment #66
alexpottWill this cause javascript to be loaded on a all 404 pages even if the user is anonymous?
I don't think we should mix the name change with the fix. Also in the new world of deprecating things we need to support the old name until Drupal 10. This change is going to break contrib - http://grep.xnddx.ru/search?text=currentPathIsAdmin&filename=
Rather than changing the name can we change this logic to only store 2xx GET requests?
Comment #67
alexpottAlso this can be tested with an automated test.
Comment #68
nod_+1 to remove the rename, it's too late now.
To store only 2xx get request, we'll need to either add a meta with the HTTP response code the JS can use, or add a value in drupalSettings, which will trigger the loading of a little JS on all pages.
For the meta we should use
<meta http-equiv="Status" content="">
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/meta#attr-http... while status is not mentioned in the doc, it seems to have some use for indieweb webmentions: https://indieweb.org/meta_http-equiv_status so it could be useful to add anyway.Probably should add the meta to everything except 2xx responses though.
Comment #69
alexpottThe
<meta http-equiv="Status" content="">
idea is super interesting and worth exploring. I'm not sure about which way around this should be. One thought is that we add the setting only for logged in users atm and then proceed with the simple fix because logged in users will already have drupal settings.