Updated: Comment #N

Problem/Motivation

#1954892: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system introduced a performance regression by unconditionally loading the user even if it's the anonymous users, which makes no sense as those can't have a user theme.

I also think the implementation is broken as $user->theme is a FieldItemList object, it should use the getDefaultTheme() method defined in UserInterface which translates to $user->theme->value. It also seems to be using a deprecated function instead of using the service to check access directly. Setting to major for now, might be critical if we can confirm that it's broken and therefore a regression.

The original issue "fixed" that by adding the field module and the necessary tables to the affected DUBT tests.

I noticed this because #2149107: "View" area handler in Views gives fatal error resulted in a broken HEAD today, re-exposing that problem.

There's also a smaller performance problem that a lot of services with injected dependencies have, that they possibly instantiate a lot of dependant services but will never use them. Yes there's the issue to add lazy service objects but that won't help here, because it's not a service. In this specific cause, it causes DUBT tests to break because they now suddenly need field.module for the field.info service as the user storage controller is instantiated (without being used)

Proposed resolution

Remove it all. It has been a zombie since 2009: #292253: Remove the per-user themes selection from core.

Remaining tasks

RTBC if green.

User interface changes

API changes

$user->theme no longer exists, but it never really did in 8.x....

CommentFileSizeAuthor
#12 remove-user-theme-2163035-12-interdiff.txt1.25 KBberdir
#12 remove-user-theme-2163035-12.patch9.98 KBberdir
#9 remove-user-theme-2163035.patch8.72 KBberdir
#1 user-negotiator-regression-2163035-1.patch3.63 KBberdir

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new user-negotiator-regression-2163035-1.patch3.63 KB
dawehner’s picture

dawehner
Primary language German
commented

+2

The D7 code looks like the following:

  // Only select the user selected theme if it is available in the
  // list of themes that can be accessed.
  $theme = !empty($user->theme) && drupal_theme_access($user->theme) ? $user->theme : variable_get('theme_default', 'bartik');

so I guess global $user->theme was just not set for the anonymous user before? This makes 100% sense to be honest.

Regarding the bug: I guess this never worked ... given that I just copied the old code from drupal_theme_initialize...

The amount of broken HEADs is just too damn high.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

In the old code, it works directly on global $user, which is a UserAccount, which contains SELECT * from users as raw values, so it probably worked better than now :)

Yeah, I bit confused why you call a deprecated function that has a service replacement in a new class, because calling that directly should have expose this, as the function has that weird is_object() check.

Crell’s picture

Crell commented 
+++ b/core/modules/user/lib/Drupal/user/Theme/UserNegotiator.php
@@ -41,7 +41,7 @@ class UserNegotiator implements  ThemeNegotiatorInterface {
   public function __construct(EntityManager $entity_manager, AccountInterface $current_user) {
-    $this->userStorageController = $entity_manager->getStorageController('user');
+    $this->entityManager = $entity_manager;
     $this->currentUser = $current_user;
   }

It makes sense to me why we're not getting the storage controller (presumably performance, so if the user is anon we may not even initialize the storage controller?), but that's non-obvious. It could use a comment, either here or further down.

Otherwise this patch looks sane to me.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yes, that's exactly what it's about.

I think it's mostly the *change* that needs a comment/explanation (which is in the issue description somewhere), not the final code, but sure, I can add a comment.

dawehner’s picture

dawehner
Primary language German
commented

OH I see. Do we want another issue to replace drupal_theme_access()?

Crell’s picture

Crell commented

dawehner: Probably, yes.

Berdir: Mostly I want to make sure someone doesn't try to clean it up later, not realizing that there's a reason we're using the nominally longer code. It's a minor point either way; the code itself looks fine.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Oh wow, this is too awesome.

Per-user themes were removed in *2009*. It doesn't even existin in D7. Apparently we forgot to remote that check and the storage for it, we just removed the form alter. See #292253: Remove the per-user themes selection from core.

Going to re-purpose this issue to remove the remaining instead. Saves me from writing test coverage for it :p

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: Performance regression in Drupal\user\Theme\UserNegotiator » Remove $user->theme and Drupal\user\Theme\UserNegotiator
Issue summary: View changes
Issue tags: -Needs tests
StatusFileSize
new remove-user-theme-2163035.patch8.72 KB

Bye bye.

Updated proposed solution in issue summary ;)

dawehner’s picture

dawehner
Primary language German
commented

I always assumed that the feature was still supported on the API level, though you could always rebuild it using hook_custom_theme in D7.

Some other DUTB tests we could adapt:

  • TextPlainUnitTest
  • DisplayPageTest

Have you tried to remove them?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

As you said, the part in core is easy to reproduce with the hook/service, reserving storage for something that core doesn't offer makes no sense to me.

I only removed it from test classes that had it added in the original patch, those two need it for different reasons, the first actually stores and saves entities and obviously needs it and the second needs the field.info service due to a entity query in the menu link storage controller (which will need it anyway as soon menu links are converted to Entity Field API.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new remove-user-theme-2163035-12.patch9.98 KB
new remove-user-theme-2163035-12-interdiff.txt1.25 KB

Ah sorry, you did touch those tests but only added the users table, I didn't check for that.

Removing those calls again.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Thank you!

star-szr’s picture

star-szr
he/him
Primary language English
commented

This is great. Nice find @Berdir!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Not sure if it was intentional leaving the API support for this in when it was originally removed, but it's definitely redundant now - contrib can add the negotiatior and storage back just as much as the form alter.

Committed/pushed to 8.x, thanks!

andypost’s picture

andypost
he/him
Primary language Russian
commented 
+++ b/core/modules/user/user.install
@@ -58,13 +58,6 @@ function user_schema() {
-      'theme' => array(
-        'type' => 'varchar',
-        'length' => DRUPAL_EXTENSION_NAME_MAX_LENGTH,
-        'not null' => TRUE,
-        'default' => '',
-        'description' => "User's default theme.",
-      ),

Seems we need change notice for that, and maybe update function?

catch’s picture

catch
he/him
Primary language English
commented
Title: Remove $user->theme and Drupal\user\Theme\UserNegotiator » Change notice: Remove $user->theme and Drupal\user\Theme\UserNegotiator
Category: Bug report » Task
Status: Fixed » Active
Issue tags: +Needs change record

No need for an update, we'll just not migrate that from 7.x. However yes this does need a change notice.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: Change notice: Remove $user->theme and Drupal\user\Theme\UserNegotiator » Remove $user->theme and Drupal\user\Theme\UserNegotiator
Category: Task » Bug report
Status: Active » Fixed
Issue tags: -Needs change record

Created https://drupal.org/node/2164115.

Status: Fixed » Closed (fixed)

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

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Parent issue: » #2228093: Modernize theme initialization