Use hook_domain_bootstrap_full() to set $conf['theme_default'] rather than hook_custom_theme() [#1870890]

the request
To be able to set theme_default via hook_domain_config().

Basically, to provide a fallback to hook_custom_theme() when the theme() layer is initiated prior to calls to menu_get_custom_theme().

i.e. any theme function called during entity load.

The issue (verbose background info / debugging steps)

On most nodes on this site, you edit and switch to admin theme and save, revert back to the correct theme, however some nodes will show the default domains theme just after saving. Refreshing the page will show the correct domains theme.

Adding some debugging information on the menu.inc, everything appears fine. Exporting the code / db locally, everything appeared fine (a missing local file bypassed the call to theme()).

Some debugging output in menu_get_custom_theme() on save produces:

    menu_get_custom_theme(): hook_custom_theme returned seven (admin theme)
    Content type Node title has been updated.
    menu_get_custom_theme(): hook_custom_theme returned community2012 (correct domain theme)

Moving into drupal_theme_initialize() we get this:

  print variable_get('theme_default', 'bartik'); // main domain default theme
  print (!empty($user->theme) && drupal_theme_access($user->theme) ? $user->theme : variable_get('theme_default', 'bartik')); // master2012 (default theme on master site)
  print $custom_theme; // undefined

Since $custom_theme = menu_get_custom_theme(); doesn't initialise the theme, I'm guessed that this was being called via a theme function prior to menu_get_item() :(

In theme() function, the following code found the cause:

  global $theme, $user;
  if (!isset($theme) && $user->uid == 1) {
    if (is_array($hook)) {
      drupal_set_message('Function theme() called, $theme was not set. The $hook parameters were: ' . implode(',', $hook));
    }
    else {
      drupal_set_message('Function theme() called, $theme was not set. The $hook parameter was: ' . $hook);
    }
  }

which spits out:

    Content type Node title has been updated.
    Function theme() called, $theme was not set. The $hook parameter was: image_resize_filter_image

Tracing this back even further:

This is called via the filter module as the Image resize filter is enabled on the selected filter on the body field of this content type, called via the Text module fields text_field_load() via _text_sanitize() / check_markup() / theme('image_resize_filter_image').

Changing to a non-cached filter solved the issue (but not an option as caching is vital on this large site).

Cross-posting: I created a new issue to resolve this on the Image resize filter: #1870888: Calling theme() within a cached filter breaks hook_custom_theme(); it would be nice to allow modules to mis-behaviour and not break the site, so leaving open for feedback.

Comment	File	Size	Author
#11	1870890-11.patch	1.1 KB	SpadXIII
#7	1870890-domain-theme-load.patch	1.1 KB	agentrickard

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

agentrickard

he/him

English

Georgia (US)

CreditAttribution: agentrickard commented 20 December 2012 at 20:31

Status:

Active

» Closed (duplicate)

Please do not cross-post issues.

There is no hook_domain_config(). This sounds like a problem in Image Resize Filter.

Comment #2

Alan D. CreditAttribution: Alan D. commented 20 December 2012 at 23:22

Title:	Optionally set domain theme using domain config.	» Use hook_domain_bootstrap_full() to set $conf['default_theme'] rather than hook_custom_theme()
Status:	Closed (duplicate)	» Active

afaik, there is no documentation anywhere that states you shouldn't use a theming function when implementing a filter or even during a field load.

OK, you have ruled out Domain config integration, so after briefly looking at DA bootstrap, maybe have Domain Theme use a custom implementation of hook_domain_bootstrap_full() to update $conf['default_theme'] to build in some robustness into the system?

Also, this would allow users to continue to use $user->theme with Domain Access.

I actually see this as a bug report, but leaving as a feature request as it is such an edge case. I will not reopen if you consider this not worthy of fixing.

Comment #3

agentrickard

he/him

English

Georgia (US)

CreditAttribution: agentrickard commented 21 December 2012 at 18:50

I originally closed this thinking that the problem is in the other module.

I can see the benefit to moving this from hook_custom_theme(), actually. That might also let use remove the special-case handling of admin themes.

Patch?

Comment #4

Jeroen CreditAttribution: Jeroen commented 17 December 2013 at 11:00

I'm also running into the issue where menu_get_custom_theme is called before menu_set_custom_theme.

On the website there's a node with an image in its body-field. When clearing cache and reloading the page field_attach_load tries to get the node from the cache:

$cache = cache_get_multiple($cids, 'cache_field');

It fails because we just cleared the cache and continues loading all the fields for that node. If the body field contains an image it will end up in the image_resize_filter module which returns a themed image. This causes the menu_get_custom_theme() to be called from drupal_theme_initialize(). There's no theme set yet and thus falls back to the default theme.

The Drupal bootstrap continues and menu_set_custom_theme does get called, setting the correct theme, but the damage has been done. $theme_key in GLOBALS already got set to the default theme.

When reloading the page cache_get_multiple() returns the now cached node. The individual fields of the node are not fetched & processed and the theme does not get called early and all goes well.

I'm wondering what could be done about this. To me it seems it's not really a Domain module problem, although I'm not sure. If anyone could shed any light onto this, please do and let me know.

Comment #5

agentrickard

he/him

English

Georgia (US)

CreditAttribution: agentrickard commented 3 January 2014 at 19:59

The approach outlined in #2 is still valid. We just need someone to code a patch for testing.

Comment #6

chrisivens CreditAttribution: chrisivens commented 14 January 2014 at 11:59

From initial investigation of the modules we have installed so far, not having the theme_default variable being set for a domain has implications on the modules like context, context_condition_theme and wysiwyg.

They assume that the theme_default contains the theme that is generally used for display and build caches etc based on this information. Unfortunately this has lead to contexts being built for SecondarySite for the theme that is active on PrimarySite.

Consequently SecondarySite displays fine whereas the PrimarySite has the wrong data cached.

I'm not certain where the best place to put the fix for this is yet. The options are somewhere like domain_theme_custom_theme or within hook_domain_bootstrap_full for domain_theme which would mean it needs adding to the list of bootstrap modules with domain_bootstrap_register()

If there's a general consensus on where the variable should be set, I don't mind submitting a patch.

Comment #7

agentrickard

he/him

English

Georgia (US)

CreditAttribution: agentrickard commented 2 February 2014 at 15:15

File	Size
1870890-domain-theme-load.patch	1.1 KB

Domain Conf handles this in hook_domain_bootstrap_full(), so that would be the place to try.

The patch should be trivial, it's the testing that's the problem.

A few things that could be problematic:

* Module weight such that this fires after domain_conf.
* Other modules (like Display Suite) expecting certain configuration values.

This module, for the record, predates Context.

Comment #8

agentrickard

he/him

English

Georgia (US)

CreditAttribution: agentrickard commented 2 February 2014 at 15:17

Status:

Active

» Needs review

Comment #9

chrisivens CreditAttribution: chrisivens commented 3 February 2014 at 10:42

I agree that the testing is problematic here. There's a a gazillion use-cases and I'm not sure if there's a one-size fits all solution.

I think the change in the above patch #7 makes domain_theme fall into line with the standard way of setting a custom theme so should be more predictable by other modules. Personally I'd love it to be committed because it solves my issue nicely.

The issue with context really should remain in the hands of the context module and how it handles custom themes and multiple themes on the same site. It's a huge can of worms but not relevant here.

Comment #10

agentrickard

he/him

English

Georgia (US)

CreditAttribution: agentrickard commented 8 February 2014 at 14:53

I disagree. hook_custom_theme() should be the standard. That's what it is for.

I could see setting this as a variable in settings.php for advanced use cases.

Comment #11

SpadXIII CreditAttribution: SpadXIII at ezCompany commented 23 June 2015 at 07:55

File	Size
1870890-11.patch	1.1 KB

1 file was hidden/shown/deleted

File	Size
1870890-domain-theme-load.patch	1.1 KB

The patch in #7 doesn't fix the problem because it sets the wrong variable: default_theme instead of theme_default :)
Attached patch corrects the variable name.
Other than that, it seems to do the trick for me.