Just discovered this quirk which is related to #563708: Improve theme_get_setting() and make custom theme settings a true form_alter.

This _alter can be invoked twice when the active theme is used to view the theme specific settings form for a related theme. Example, Theme A is active and you'r viewing Theme A's or the child of Theme A's configuration page.

It's first invoked from system_theme_settings() and the second through drupal_alter(). The former passes $form in a very early state so it will be very different when it's called the second time. Any modifications done on the first pass will be included in the second also.

Why is it called twice? Because drupal_alter() works on active running themes while system_theme_settings() allows any theme that has focus in the theme specific settings page. There is a difference.

For example, if theme 'Z' is not active but you are on the Z settings page (admin/appearance/settings/z), the call from system_theme_settings() allows theme Z to have Z_form_system_theme_settings_alter() invoked. But what happens when the theme is active and running while looking at the theme specific settings form? It gets invoked twice because drual_alter comes in later and acts on that form again.

Renaming the call made from system_theme_settings() was a mistake since they work in a different context and nothing should be doing _alter calls outside of drupal_alter.

CommentFileSizeAuthor
#4 hook_form_system_theme_settings_alterx2_1135794_4.patch1.83 KBdvessel

Comments

dergachev’s picture

dergachev commented

We're seeing this as well.

I think it's important to resolve this quickly, as people are now beginning to write D7 themes with custom settings, and the resolution might break their code. For example see http://drupal.org/node/177868#comment-4191174

dergachev’s picture

dergachev commented

In case it's helpful, here's the exact places the SUBTHEME_form_theme_settings_alter() is being run twice.

First in system.admin.inc:562, in system_theme_settings() : 

// Process the theme and all its base themes.
foreach ($theme_keys as $theme) {
  // Include the theme-settings.php file.
  $filename = DRUPAL_ROOT . '/' . str_replace("/$theme.info", '', $themes[$theme]->filename) . '/theme-settings.php';
  if (file_exists($filename)) {
    require_once $filename;
  }

  // Call theme-specific settings.
  $function = $theme . '_form_system_theme_settings_alter';
  if (function_exists($function)) {
    $function($form, $form_state);
  }
}

And again in includes/form.inc:1040, in drupal_prepare_form ():


// Invoke hook_form_alter(), hook_form_BASE_FORM_ID_alter(), and
// hook_form_FORM_ID_alter() implementations.
$hooks = array('form');
if (isset($form_state['build_info']['base_form_id'])) {
  $hooks[] = 'form_' . $form_state['build_info']['base_form_id'];
}
$hooks[] = 'form_' . $form_id;
//$hooks == array('form','form_system_theme_settings');
drupal_alter($hooks, $form, $form_state, $form_id);
dergachev’s picture

dergachev commented

And some potentially relevant threads:

http://drupal.org/node/481142#comment-2525928
http://drupal.org/node/563708

dvessel’s picture

dvessel commented
Status: Active » Needs review
StatusFileSize
new hook_form_system_theme_settings_alterx2_1135794_4.patch1.83 KB

The fix is very simple but it would break any existing theme in a confusing way. That call from system_theme_settings() just needs to be changed to the old name. The bug is dampened a bit due to the default admin theme not being the same as the site wide active theme but it should be fixed. Here's a patch.

Jeff Burnz’s picture

Jeff Burnz commented

Is this the same bug as #943212: hook_form_system_theme_settings_alter() vs. THEME_form_system_theme_settings_alter() name-clash

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: hook_form_system_theme_settings_alter invoked twice » Introduce hook_settings() for themes
Version: 7.x-dev » 8.x-dev
Category: bug » task
Issue tags: +API change

This is - obviously - a major API change and can only be considered for D8+.

dvessel’s picture

dvessel commented
Status: Needs review » Closed (duplicate)