Back to site links to wrong path in case the last path visited is 404 page

This is totally an interesting bug.

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.

As I said, no need to change the JS file, see what's done in Drupal\block\Controller::demo.

      '#attached' => array(
        'drupalSettings' => [
          // The block demonstration page is not marked as an administrative
          // page by \Drupal::service('router.admin_context')->isAdminRoute()
          // function in order to use the frontend theme. Since JavaScript
          // relies on a proper separation of admin pages, it needs to know this
          // is an actual administrative page.
          'path' => ['currentPathIsAdmin' => TRUE],
        ],

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.

leftover console.log

Verified 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.

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"

#15 works fine.

I'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.

Agreed; the patch here doesn't actually solve the problem. It merely treats the symptoms.

I'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?

Changing to maintainer needs more info since I can't reproduce.

Ah 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?

Well 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.

I think the back to site storer should only store urls that are 2xx responses.

+1

Oops, obviously we can fix that here.

#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 I think the back to site storer should only store urls that are 2xx responses. is impossible.

Hopefully @nod_ or @droplet has some idea.

To clarify: JS cannot t get the response status AFAICT. Surprisingly.

+1

Changed the name from currentPathIsAdmin to escapeFromAdmin. 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.

Discussed 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.

It doesn't a bug IMO. ( but can be improve... )

Need usability team feedback on #41.

It makes sense to me so I'd won't fix this issue.

Need reroll and feedback about usability on #41

Rerolled patch for 9.1.x, please review.

Resolves the failed test cases in #60

Removes the extra new line added in core/modules/toolbar/js/escapeAdmin.js

Looks 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

The 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.

1. +++ b/core/modules/system/src/Controller/Http4xxController.php
   @@ -54,6 +54,12 @@ public function on403() {
   +      '#attached' => [
   +        'drupalSettings' => [
   +          // Do not save this page in as a non-admin page to go back to.
   +          'path' => ['escapeFromAdmin' => TRUE],
   +        ],
   +      ],
   

   Will this cause javascript to be loaded on a all 404 pages even if the user is anonymous?

2. +++ b/core/modules/system/system.module
   @@ -712,7 +712,7 @@ function system_js_settings_alter(&$settings, AttachedAssetsInterface $assets) {
   -    'currentPathIsAdmin' => $current_path_is_admin,
   +    'escapeFromAdmin' => $current_path_is_admin,
   

   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=

3. +++ b/core/modules/toolbar/js/escapeAdmin.es6.js
   @@ -13,7 +13,7 @@
   -    !pathInfo.currentPathIsAdmin &&
   +    !pathInfo.escapeFromAdmin &&
        !/destination=/.test(windowLocation.search)
      ) {
        sessionStorage.setItem('escapeAdminPath', windowLocation);
   @@ -35,7 +35,7 @@
   
   @@ -35,7 +35,7 @@
          const $toolbarEscape = $('[data-toolbar-escape-admin]').once(
            'escapeAdmin',
          );
   -      if ($toolbarEscape.length && pathInfo.currentPathIsAdmin) {
   +      if ($toolbarEscape.length && pathInfo.escapeFromAdmin) {
   

   Rather than changing the name can we change this logic to only store 2xx GET requests?

Also this can be tested with an automated test.

+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.

The <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.

I tested this on 11.x today and confirmed this is still a bug.

The Toolbar Module was approved for removal in #3476882: [Policy] Move Toolbar module to contrib.

This is Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

The deprecation work is in #3484850: [meta] Tasks to deprecate Toolbar module and the removal work in #3488828: [meta] Tasks to remove Toolbar module.

Toolbar will be moved to a contributed project before Drupal 12.0.0 is released.