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

Reference: https://www.drupal.org/core/beta-changes
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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

I try to figure out why we need a theme token and not just rely on the passed in theme + \Drupal\Core\Theme\ThemeAccessCheck::access

catch’s picture

Issue summary: View changes
catch’s picture

Status: Active » Needs review
FileSize
1.02 KB

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

Run #5580748b08260 Run #5580750c53c17 Diff Diff%
Number of Function Calls 35,101 35,063 -38 -0.1%
Incl. Wall Time (microsec) 194,025 200,429 6,404 3.3%
Incl. CPU (microsecs) 182,203 186,601 4,398 2.4%
Incl. MemUse (bytes) 21,479,096 21,465,656 -13,440 -0.1%
Incl. PeakMemUse (bytes) 21,633,112 21,616,072 -17,040 -0.1%
dawehner’s picture

+++ b/core/modules/system/system.module
@@ -666,7 +666,12 @@ function system_js_settings_alter(&$settings, AttachedAssetsInterface $assets) {
+      if ($theme_key !== $this->configFactory->get('system.theme')->get('default')) {

Quick note: We have an API wrapper for that: \Drupal\Core\Extension\ThemeHandlerInterface::getDefault

Status: Needs review » Needs work

The last submitted patch, 3: 2507093.patch, failed testing.

dawehner’s picture

+++ b/core/modules/system/system.module
@@ -666,7 +666,12 @@ function system_js_settings_alter(&$settings, AttachedAssetsInterface $assets) {
+      if ($theme_key !== $this->configFactory->get('system.theme')->get('default')) {

so yeah $this does not work in an ordinary function. ... Let's use \Drupal::service('theme_handler')->getDefault() instead.

claudiu.cristea’s picture

Assigned: Unassigned » claudiu.cristea
timmillwood’s picture

Assigned: claudiu.cristea » Unassigned
Status: Needs work » Needs review
FileSize
1.19 KB
claudiu.cristea’s picture

So fast :)

dawehner’s picture

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

Fabianx’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Yes, a quick test would be great for:

- normal theme
- seven (admin) theme

Looks great, besides that.

Wim Leers’s picture

+++ b/core/modules/system/system.module
@@ -689,8 +689,13 @@ function system_js_settings_alter(&$settings, AttachedAssetsInterface $assets) {
+      if (\Drupal::theme()->getActiveTheme()->getName() !== \Drupal::service('theme_handler')->getDefault()) {
...
+          ->get(\Drupal::theme()->getActiveTheme()->getName());

Both of these are getting the theme name. Let's store it in a variable.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
1.97 KB
1.72 KB

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

dawehner’s picture

You could for example have a different admin theme + have a route on /admin/... which then should generate the theme token.

claudiu.cristea’s picture

claudiu.cristea’s picture

Issue tags: -Needs tests
dawehner’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

+1

Added a beta eval

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This 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!

  • alexpott committed 767cec2 on 8.0.x
    Issue #2507093 by claudiu.cristea, catch, timmillwood, dawehner: Don't...

Status: Fixed » Closed (fixed)

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