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
We calculate the theme_token for ajaxPageState on every request.
However in AJAX requests, we only care about the non-default theme.
Calculating the theme token means a state get (for the private key), and initializing the csrf token service at least.
Proposed resolution
Don't bother setting the token when in the default theme.
Remaining tasks
User interface changes
API changes
Beta phase evaluation
Issue category | Task, because its not a bug, but just an optimization. |
---|---|
Issue priority | Major, because it improves the performance on every request |
Disruption | Nothing should rely on this rather internally drupalSetting, but people might rely that, but REALLY unluckily. |
Comment | File | Size | Author |
---|---|---|---|
#15 | interdiff.txt | 2.83 KB | claudiu.cristea |
#15 | don_t_calculate_the-2507093-15.patch | 3.28 KB | claudiu.cristea |
|
Comments
Comment #1
dawehnerI try to figure out why we need a theme token and not just rely on the passed in theme +
\Drupal\Core\Theme\ThemeAccessCheck::access
Comment #2
catchComment #3
catch@dawehner I think that only checks if the theme is installed. We don't have a way to check if for example a user can see a particular page (or AJAX response) in the admin theme or not.
Patch.
Comment #4
dawehnerQuick note: We have an API wrapper for that:
\Drupal\Core\Extension\ThemeHandlerInterface::getDefault
Comment #6
dawehnerso yeah $this does not work in an ordinary function. ... Let's use \Drupal::service('theme_handler')->getDefault() instead.
Comment #7
claudiu.cristeaComment #8
timmillwoodComment #9
claudiu.cristeaSo fast :)
Comment #10
dawehnerI think it would be cool to have a quick test to ensure that this variable is not always printed out.
Just a quick assumption, I would assume we don't show it at the moment at all.
Comment #11
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedYes, a quick test would be great for:
- normal theme
- seven (admin) theme
Looks great, besides that.
Comment #12
Wim LeersBoth of these are getting the theme name. Let's store it in a variable.
Comment #13
claudiu.cristeaI'm not sure that this test is complete. In fact, I don't understand exactly how to make an Ajax request that uses a different theme than the default. Right now the test is using the classy theme and behaves as expected.
Comment #14
dawehnerYou could for example have a different admin theme + have a route on /admin/... which then should generate the theme token.
Comment #15
claudiu.cristeaComment #16
claudiu.cristeaComment #17
dawehner+1
Added a beta eval
Comment #18
alexpottThis issue is a major task that will improve performance and the disruption it introduces is limited. Per https://www.drupal.org/core/beta-changes, this is a good change to complete during the Drupal 8 beta phase. Committed 767cec2 and pushed to 8.0.x. Thanks!