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.
Comment | File | Size | Author |
---|---|---|---|
#22 | interdiff.txt | 2.93 KB | Wim Leers |
#22 | toolbar_subtrees_not-2535118-22.patch | 8.75 KB | Wim Leers |
#15 | toolbar_subtrees_not-2535118-15.patch | 6.96 KB | borisson_ |
#15 | interdiff.txt | 1.71 KB | borisson_ |
#11 | 2535118-toolbar-ajax.interdiff-8-11.txt | 572 bytes | longwave |
Comments
Comment #1
Wim LeersThere are two ways of solving this:
ajaxPageState
sent anyway, because that containstheme
and atheme_token
, which we need to ensure that the current user is in fact allowed to use the specified theme.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.
Comment #2
Wim LeersComment #3
Fabianx CreditAttribution: Fabianx as a volunteer commentedI like option 2).
Comment #4
dawehnerYeah the later one just make sense, given that we have the framework for the ajax theme already.
Comment #5
BerdirThere'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.
Comment #6
Wim Leers#5: hah, interesting!
Comment #7
longwaveMy first serious attempt at JavaScript in Drupal 8!
Comment #8
longwaveOops, this isn't JSONP any more.
Comment #9
idflood CreditAttribution: idflood at Stimul.ch commentedI'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.
Comment #10
borisson_Looking over the code, I can only see one small nitpick that should be fixed.
comment is wider than 80 cols.
Comment #11
longwaveThanks for reviewing and testing! Fixed #10
Comment #12
Fabianx CreditAttribution: Fabianx as a volunteer commentedI would RTBC this, but could we have a test for the bug discovered here, so we don't regress?
Comment #13
longwaveI 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.
Comment #14
Wim LeersLooks great :)
And I fear it is indeed impossible to write an integration test for this.
[…] that builds an AJAX command with the rendered subtrees.
s/response/response's command/
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.)
s/array()/[]/
Comment #15
borisson_Fixed the nits from #14
Comment #16
Fabianx CreditAttribution: Fabianx as a volunteer commentedRTBC, I agree this is very difficult to test and current test coverage passes.
Comment #17
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPatch 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:
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:
Comment #18
longwaveShouldn't it be cached in localStorage and avoid the AJAX request entirely from the second page onwards?
Comment #19
effulgentsia CreditAttribution: effulgentsia at Acquia commentedOn 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.
Comment #20
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #21
effulgentsia CreditAttribution: effulgentsia at Acquia commentedFYI: 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.
Comment #22
Wim LeersThis mixes up several things.
The solution is therefore quite simple: cache it per-theme.
Manually tested & confirmed that this solves the problem.
Comment #23
borisson_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.
Comment #24
Wim LeersWe 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.
Comment #25
Wim LeersGiven 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.
Comment #26
longwaveAlso reviewed and manually tested #22, looks good to me: RTBC+1
Comment #27
effulgentsia CreditAttribution: effulgentsia at Acquia commentedRTBC+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.
Comment #28
alexpottI 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!
Removed unused use on commit.
Comment #30
Wim LeersSorry about the
eslint
errors. Though… I don't see them? Only JSDoc warnings.Comment #31
Fabianx CreditAttribution: Fabianx as a volunteer commentedI agree with the solution.
FWIW, to switch this to a GET request just for the toolbar should be possible via as little code as:
So I think it makes sense to explore that independent from the more general issue.
Comment #32
alexpott@Wim Leers - I tried to say that this patch introduced 3 new warnings and 0 errors so we could leave the warnings till later.
Comment #33
Wim LeersAhhh :)
Comment #34
Wim LeersComment #36
leisurman CreditAttribution: leisurman commentedI 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: