Problem/Motivation

#2534830: Toolbar subtrees not working reported that toolbar subtrees requests are 403s.

The root cause turns out to be a bug that has existed in Toolbar since the very beginning, but is only exposed since #2217985: Replace the custom menu caching strategy in Toolbar with Core's standard caching.. The problem is that we calculate the subtrees hash in the current theme (so Bartik on the frontpage, Seven on /admin/structure), but during the AJAX request, we always use the default theme.

This causes the hashes to be different, and hence the 403… on admin pages only.

(Go to /admin/appearance, and use Bartik as the administration theme, and it'll work at /admin/structure as well.)

Proposed resolution

TBD

Remaining tasks

TBD

User interface changes

None. (Except a toolbar that actually works in all cases again.)

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Title: Toolbar subtrees not working on admin pages » Toolbar subtrees not working on admin pages due to lack of theme negotiation on Toolbar's custom JSONP route
Status: Active » Needs review
Issue tags: +JavaScript, +Needs reroll, +Novice
FileSize
2.61 KB

There are two ways of solving this:

  1. either add a new theme negotiator for JSONP responses that do rendering, but then we need to have ajaxPageState sent anyway, because that contains theme and a theme_token, which we need to ensure that the current user is in fact allowed to use the specified theme.
  2. or convert the Toolbar module to use an AJAX response, with a custom AJAX command, so that we get all of the above for free

Personally, I'm leaning towards the latter, because if you're using the render system, you need to use the infrastructure that the AJAX system already provides anyway. Why reinvent the wheel?

Contrib that uses JSONP would likely not be using the render/theme system at all, so they can continue to use the pattern that Toolbar is using in HEAD.


Here's an outline for what needs to happen to go with option 2.

Wim Leers’s picture

Issue tags: -Needs reroll +js-novice
Fabianx’s picture

I like option 2).

dawehner’s picture

Yeah the later one just make sense, given that we have the framework for the ajax theme already.

Berdir’s picture

There's actually a similar problem with the language. We are trying to use the admin language feature on an 8.x site and run into all kinds of problems because the ajax callbacks use the default language, which prevents form submissions from working, translated labels showing up and so on.

That said, toolbar actually has the opposite problem, it should always use the admin language: #2313309: Admin toolbar and contextual links should always be rendered in the admin language (if set)

That's all not directly related, but after this change I think we can relatively easily switch the toolbar to /admin/toolbar/something without just inverting the problem mentioned here.

Wim Leers’s picture

#5: hah, interesting!

longwave’s picture

My first serious attempt at JavaScript in Drupal 8!

longwave’s picture

Oops, this isn't JSONP any more.

idflood’s picture

I've tested the patch in #8 and the subtree was working again on my local install. I've also quickly reviewed the code and it looks good.

borisson_’s picture

Status: Needs review » Needs work

Looking over the code, I can only see one small nitpick that should be fixed.

+++ b/core/modules/toolbar/src/Controller/ToolbarController.php
@@ -17,14 +18,14 @@
+   * Returns an AJAX response to render the subtree of each top-level toolbar link.

comment is wider than 80 cols.

longwave’s picture

Status: Needs work » Needs review
FileSize
6.97 KB
572 bytes

Thanks for reviewing and testing! Fixed #10

Fabianx’s picture

I would RTBC this, but could we have a test for the bug discovered here, so we don't regress?

longwave’s picture

I am happy to try and write a test for this if someone can suggest how to go about it. There is a test for the Ajax response itself but as the problem is partially solved by changing JS I am not really sure how to cover it from pure PHP.

Wim Leers’s picture

Status: Needs review » Needs work

Looks great :)

And I fear it is indeed impossible to write an integration test for this.

  1. +++ b/core/modules/toolbar/js/views/ToolbarVisualView.js
    @@ -249,7 +249,7 @@
    +     * Calls the endpoint URI that will render subtrees via AJAX.
    

    […] that builds an AJAX command with the rendered subtrees.

  2. +++ b/core/modules/toolbar/js/views/ToolbarVisualView.js
    @@ -282,9 +282,9 @@
    +          // The AJAX response will trigger the resolve method of the
    

    s/response/response's command/

  3. +++ b/core/modules/toolbar/src/Ajax/SetSubtreesCommand.php
    @@ -0,0 +1,44 @@
    +  /**
    +   * Implements \Drupal\Core\Ajax\CommandInterface::render().
    +   */
    

    Should be @inheritdoc. You probably copy/pasted this from an existing AJAX command, but they're all old, and should use @inheritdoc too.

    (You can fix that in a follow-up if you want, but please not here.)

  4. +++ b/core/modules/toolbar/src/Ajax/SetSubtreesCommand.php
    @@ -0,0 +1,44 @@
    +    return array(
    

    s/array()/[]/

borisson_’s picture

Status: Needs work » Needs review
FileSize
1.71 KB
6.96 KB

Fixed the nits from #14

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, I agree this is very difficult to test and current test coverage passes.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review

Patch looks great, and fixes it so that when the toolbar is in vertical orientation, it actually shows the drop-down arrows that it used to before the regression, which makes it very tempting to commit before tomorrow's beta. However:

+++ b/core/modules/toolbar/js/views/ToolbarVisualView.js
@@ -282,9 +283,9 @@
-          $.ajax(endpoint);
+          Drupal.ajax({url: endpoint}).execute();

Not obvious, but this changes the request from a GET to a POST, because Drupal.ajax() always uses POST until #956186: Allow AJAX to use GET requests is fixed. Which means that the result isn't cached, and I end up seeing flicker (on Chrome) of the toolbar as I navigate the site.

Some options:

  • Commit anyway, and wait for #956186: Allow AJAX to use GET requests to improve that.
  • Postpone this patch on that one.
  • Solve this in a way that's GET-compatible (i.e., not using Drupal.ajax or hacking it somehow).
  • Other ideas?
longwave’s picture

Which means that the result isn't cached, and I end up seeing flicker (on Chrome) of the toolbar as I navigate the site.

Shouldn't it be cached in localStorage and avoid the AJAX request entirely from the second page onwards?

effulgentsia’s picture

On my machine, when I use Chrome's network inspector, and I go back and forth between clicking "Content" on the toolbar and then "Back to site" from the /admin/content page, I see the AJAX POST request occurring each time.

effulgentsia’s picture

effulgentsia’s picture

Which means that the result isn't cached, and I end up seeing flicker (on Chrome) of the toolbar as I navigate the site.

FYI: With #2541794-1: [regression] Toolbar subtrees have regressed: either fix or revert the regressions, I do not see the extra http requests of #19, but I still see some toolbar flicker, e.g., when going between /admin/config/development/performance and /admin/config/development/logging. I thought there was a time when there wasn't that flicker, but I might be misremembering. Anyway, if there's a flicker either way, then #17.1 (commit what's here now and optimize later) seems acceptable to me.

Wim Leers’s picture

Not obvious, but this changes the request from a GET to a POST, because Drupal.ajax() always uses POST until #956186: Allow AJAX to use GET requests is fixed. Which means that the result isn't cached, and I end up seeing flicker (on Chrome) of the toolbar as I navigate the site.

On my machine, when I use Chrome's network inspector, and I go back and forth between clicking "Content" on the toolbar and then "Back to site" from the /admin/content page, I see the AJAX POST request occurring each time.

This mixes up several things.

  • Personally, I've always seen flicker. (EDIT: oh, #21 confirms this too.)
  • Yes, it does change it from a GET to a POST request.
  • But: that should not cause the flicker you mention, particularly because the toolbar is not relying on the response being cached by the browser: it's doing client-side caching of the toolbar already anyway.
  • That being said: I know what's causing the subtrees request to fire again: switching themes causes a different toolbar hash (see the IS for an explanation), so naturally it also causes the client-side cache to be invalidated.

The solution is therefore quite simple: cache it per-theme.

Manually tested & confirmed that this solves the problem.

borisson_’s picture

The patch in #22 looks right. I've also manually tested and I can confirm the AJAX request is gone after applying the patch.

I think we can open up a followup if we want to change this back to a GET-request. But this looks good to go otherwise.

Wim Leers’s picture

I think we can open up a followup if we want to change this back to a GET-request. But this looks good to go otherwise.

We can't do that, the Toolbar now uses the AJAX system for the subtrees request, and the AJAX system currently only works with POST requests. See #956186: Allow AJAX to use GET requests for making the AJAX system support GET requests.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Given the RTBC in #16 plus the small in changes I made in #22, which I thoroughly manually tested, plus the confirmation in #23 that this works as expected: moving back to RTBC.

longwave’s picture

Also reviewed and manually tested #22, looks good to me: RTBC+1

effulgentsia’s picture

Assigned: Unassigned » catch

RTBC+1 from me as well. In my opinion, the fix to client-side caching makes the POST request for a client-side miss not bad at all. I'd feel comfortable committing this if we weren't in a pre-beta commit freeze right now, but since we are, assigning to catch. I do think we should commit either this or #2541794-1: [regression] Toolbar subtrees have regressed: either fix or revert the regressions before tagging the beta, and I think this one is preferable.

alexpott’s picture

Assigned: catch » Unassigned
Status: Reviewed & tested by the community » Fixed

I agree with @effulgentsia. This patch adds 3 eslint warnings but as they're not commit blocking (ie not errors) going to ignore for later cleanup. Committed 0c810aa and pushed to 8.0.x. Thanks!

diff --git a/core/modules/toolbar/src/Tests/ToolbarAdminMenuTest.php b/core/modules/toolbar/src/Tests/ToolbarAdminMenuTest.php
index 15a1ada..3139b5d 100644
--- a/core/modules/toolbar/src/Tests/ToolbarAdminMenuTest.php
+++ b/core/modules/toolbar/src/Tests/ToolbarAdminMenuTest.php
@@ -7,7 +7,6 @@
 
 namespace Drupal\toolbar\Tests;
 
-use Drupal\Component\Serialization\Json;
 use Drupal\Core\Cache\Cache;
 use Drupal\Core\Language\LanguageInterface;
 use Drupal\Core\Url;

Removed unused use on commit.

  • alexpott committed 0c810aa on 8.0.x
    Issue #2535118 by longwave, Wim Leers, borisson_, effulgentsia: Toolbar...
Wim Leers’s picture

Sorry about the eslint errors. Though… I don't see them? Only JSDoc warnings.

$ eslint core/modules/toolbar/js

core/modules/toolbar/js/models/ToolbarModel.js
  137:4  warning  Missing JSDoc parameter description for 'attributes'  valid-jsdoc
  137:4  warning  Missing JSDoc parameter description for 'options'     valid-jsdoc
  137:4  warning  Missing JSDoc return description                      valid-jsdoc

core/modules/toolbar/js/toolbar.js
  180:4  warning  Missing JSDoc parameter description for 'model'     valid-jsdoc
  180:4  warning  Missing JSDoc parameter description for 'label'     valid-jsdoc
  180:4  warning  Missing JSDoc parameter description for 'mql'       valid-jsdoc
  237:2  warning  Missing JSDoc parameter description for 'ajax'      valid-jsdoc
  237:2  warning  Missing JSDoc parameter description for 'response'  valid-jsdoc
  237:2  warning  Missing JSDoc parameter description for 'status'    valid-jsdoc

core/modules/toolbar/js/toolbar.menu.js
  178:2  warning  Missing JSDoc parameter description for 'options'         valid-jsdoc
  178:2  warning  Missing JSDoc parameter description for 'options.class'   valid-jsdoc
  178:2  warning  Missing JSDoc parameter description for 'options.action'  valid-jsdoc
  178:2  warning  Missing JSDoc parameter description for 'options.text'    valid-jsdoc

core/modules/toolbar/js/views/ToolbarAuralView.js
  12:4  warning  Missing JSDoc parameter description for 'options'          valid-jsdoc
  12:4  warning  Missing JSDoc parameter description for 'options.strings'  valid-jsdoc
  29:4  warning  Missing JSDoc parameter description for 'model'            valid-jsdoc
  42:4  warning  Missing JSDoc parameter description for 'model'            valid-jsdoc

core/modules/toolbar/js/views/ToolbarVisualView.js
   12:4  warning  Missing JSDoc return description                           valid-jsdoc
   30:4  warning  Missing JSDoc parameter description for 'options'          valid-jsdoc
   30:4  warning  Missing JSDoc parameter description for 'options.strings'  valid-jsdoc
   57:4  warning  Missing JSDoc return description                           valid-jsdoc
   90:4  warning  Missing JSDoc parameter description for 'event'            valid-jsdoc
  110:4  warning  Missing JSDoc parameter description for 'event'            valid-jsdoc

✖ 23 problems (0 errors, 23 warnings)
Fabianx’s picture

I agree with the solution.

FWIW, to switch this to a GET request just for the toolbar should be possible via as little code as:

$ajax = Drupal.ajax({url: endpoint});
$ajax.options.type = 'GET';
$ajax.execute();

So I think it makes sense to explore that independent from the more general issue.

alexpott’s picture

@Wim Leers - I tried to say that this patch introduced 3 new warnings and 0 errors so we could leave the warnings till later.

Wim Leers’s picture

Ahhh :)

Wim Leers’s picture

Status: Fixed » Closed (fixed)

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

leisurman’s picture

Issue tags: -JavaScript +JavaScript

I am still getting this error on Drupal 9.3. When the admin toolbar is in a vertical position.

POST http://au1/toolbar/subtrees/VAQdABldryPtu9HzNVIjx1cgriyLzPRGyWJdtSLaUAo?_wrapper_format=drupal_ajax 403 (Forbidden)

Uncaught Drupal.AjaxError {message: "\nAn AJAX HTTP error occurred.\nHTTP Result Code: 40…tatusText: Forbidden\nResponseText: