Updated: Comment #47

Problem/Motivation

First the setup to recreate this problem:

- A sub theme set to the Admin theme
- Another sub theme (with same base theme as admin theme) set as the main front end theme
- Both sub themes have custom theme settings

When changing the theme settings for the front end theme, they are saved and take effect, however when the settings page reloads all the settings for the admin theme are loaded instead, not the front end themes settings.

To show this clearly please watch this video.

--

@bfroehle summarizes the problem technically in #2:

The theme settings pages are built by system_theme_settings(). This function generates the form by running THEME_system_theme_settings_alter() for the theme you are trying to edit, as well as all of that theme's parents. In each case, $GLOBALS['theme_key'] is set to the name of the theme we're trying to edit.

After the form is initially built, we reset $GLOBALS['theme_key'] to the name of the administration theme, and finish processing the form by calling drupal_alter() which then calls THEME_system_theme_settings_alter() and overwrites the correct sub-theme values.

Proposed resolution

We need to move away from the drupal_alter() namespace in this case as form alter hooks are the main cause of this issue.

Remaining tasks

  1. There is a patch attached to comment #43 that needs help to get passing tests

User interface changes

N/A

API changes

@JohnAlbin provides a good explantion of the API changes in #34

As discussed in https://drupal.org/node/1164790 if we rename system_theme_settings() to system_theme_config() and THEME_form_system_theme_settings_alter() to THEME_theme_config() (That would also make the module's hook alter be hook_form_system_theme_config_alter().)

N/A

Files: 
CommentFileSizeAuthor
#43 core-943212-7446826.patch11.81 KBarknoll
FAILED: [[SimpleTest]]: [MySQL] 55,302 pass(es), 101 fail(s), and 5 exception(s).
[ View ]
#36 943212-36-admin-themes.patch14.53 KBJohnAlbin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 943212-36-admin-themes.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#34 943212-34-admin-themes.patch14.53 KBJohnAlbin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 943212-34-admin-themes.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#32 drupal_alter-943212-31.patch950 bytesryanissamson
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/module.inc.
[ View ]
#31 tests_only-943212-31.patch2.68 KBryanissamson
FAILED: [[SimpleTest]]: [MySQL] 46,397 pass(es), 35 fail(s), and 577 exception(s).
[ View ]
#13 943212-tests-only-but-tests-work.patch3.19 KBbfroehle
FAILED: [[SimpleTest]]: [MySQL] 31,561 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#12 943212-Tests-only.patch3.18 KBbfroehle
FAILED: [[SimpleTest]]: [MySQL] 30,371 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#7 943212-7-drupal_alter.patch804 bytesbfroehle
PASSED: [[SimpleTest]]: [MySQL] 27,463 pass(es).
[ View ]

Comments

Priority:Major» Critical

I haven't tried reproducing, but if the bug is as described, and does not happen in D6, I think it qualifies as critical.

The theme settings pages are built by system_theme_settings(). This function generates the form by running THEME_system_theme_settings_alter() for the theme you are trying to edit, as well as all of that theme's parents. In each case, $GLOBALS['theme_key'] is set to the name of the theme we're trying to edit.

After the form is initially built, we reset $GLOBALS['theme_key'] to the name of the administration theme, and finish processing the form by calling drupal_alter() which then calls THEME_system_theme_settings_alter() and overwrites the correct sub-theme values.

--- includes/form.inc
+++ includes/form.inc
@@ -968,7 +968,9 @@ function drupal_prepare_form($form_id, &$form, &$form_state) {
   if (isset($form_state['build_info']['base_form_id'])) {
     $hooks[] = 'form_' . $form_state['build_info']['base_form_id'];
   }
-  $hooks[] = 'form_' . $form_id;
+  if ($form_id != 'system_theme_settings') {
+    $hooks[] = 'form_' . $form_id;
+  }
   drupal_alter($hooks, $form, $form_state, $form_id);
}

Will certainly fix the problem, but I think it's not at all the correct solution.

Priority:Critical» Major

At this point, critical == if we don't fix it before RC, people will die. I don't see any reason this couldn't be fixed after RC1, or even in 7.2.

Title:Regression: Theme settings errors when admin theme and front end theme share the same base themeTHEME_form_system_theme_settings_alter called too many times.
Priority:Major» Critical
Issue tags:+theme hooks

I'm retitling to give a better description of the problem. I disagree a bit with @webchick here -- the cleanest solution might be to rename THEME_form_system_theme_settings_alter. Right now that function is serving two distinctly different uses:

  1. Generate the theme settings form. This generation procedure, while it is 'altering' the default theme settings form is not called from drupal_alter but rather directly from system_theme_settings.
  2. Alter the form of name system_theme_settings, via the standard drupal_alter / form generation framework.

Any solution to this bug, short of renaming THEME_form_system_theme_settings_alter, is likely going to involve a hack-ish "don't call theme hooks when altering form_system_theme_settings."

Feel free to bump it back down to major if you disagree.

Priority:Critical» Major

See http://drupal.org/node/974066 - webchick has already given her opinion on this issue.

Status:Active» Needs review
StatusFileSize
new804 bytes
PASSED: [[SimpleTest]]: [MySQL] 27,463 pass(es).
[ View ]

Here's a patch which changes drupal_alter to not call THEME_form_system_settings_form_alter. It fixes the problem locally for me.

Patch in #7 is working for me on latest DEV.

Status:Needs review» Needs work

Patch in #7 won't fly.

A test would help to resolve this issue + I'd like to see a test-only patch first that confirms the bug.

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

sun suggested we write a test for this first, tagging.

Status:Needs review» Needs work

cross post

Status:Needs work» Needs review
StatusFileSize
new3.18 KB
FAILED: [[SimpleTest]]: [MySQL] 30,371 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Okay. Here's a test which pretty much replicates the issue in the original post. Setting to Needs Review to trigger the bot.

Edit: Oops, this will never pass since I messed up the assertFieldChecked call.

StatusFileSize
new3.19 KB
FAILED: [[SimpleTest]]: [MySQL] 31,561 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

This set of tests should actually work properly. See the note in #12 to know what was wrong with the previous attempt.

Status:Needs review» Needs work

The last submitted patch, 943212-tests-only-but-tests-work.patch, failed testing.

Test will need to be re-rolled once #953336: Contributed modules are not able to test theme-related functionality goes in. (Also, this means we could put the test themes in modules/system/.... instead of in themes/tests.

Status:Needs work» Needs review
Issue tags:-Needs tests, -theme hooks

#13: 943212-tests-only-but-tests-work.patch queued for re-testing.

A test would help to resolve this issue + I'd like to see a test-only patch first that confirms the bug.

Well, here is your test-only patch.

Status:Needs review» Needs work
Issue tags:+Needs tests, +theme hooks

The last submitted patch, 943212-tests-only-but-tests-work.patch, failed testing.

Jeff Burnz pointed to this issue from my duplicate post. #1135794: Introduce hook_settings() for themes

Doing anything to drupal_alter to fix this specific issue should not be done. It was a mistake to rename hook_settings so it shares the same namespace as drupal_alter.

Title:THEME_form_system_theme_settings_alter called too many times.hook_form_system_theme_settings_alter() vs. THEME_form_system_theme_settings_alter() name-clash
Version:7.x-dev» 8.x-dev
Issue tags:+needs backport to D7

Sorry if I'm missing something, but I'm 99% sure that this issue can be 100% resolved by simply removing these lines from system_theme_settings():

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

100% sure including all edge-cases pertaining to themes.

Sun, note the $theme_key switching. That key is used to set focus on the theme specific form. The theme used to render the page and the theme settings form for given theme are not always the same.

Well, confirmed so subscribe. Running into this building my Seven sub theme (http://drupal.org/sandbox/yoroy/1131280), tweaking the styling of the toolbar. In admin I see my toolbar changes, in the front end I see default Seven styling.

It seems the 3 previous comments above need some more discussion still.

ouch, subscribing.

For D8, I think we should rename THEME_form_system_theme_settings_alter() to something not in the drupal_alter() namespace.

For D7, I think we're hosed. One very hackish option would be to require theme developers to change the design pattern to be:

<?php
function THEME_form_system_theme_settings_alter(&$form, &$form_state, $form_id = NULL) {
  if (
$form_id) {
   
// do drupal_alter()-type things.
 
}
  else {
   
// do system_theme_settings() generation things.
 
}
}
?>
to rely on the fact that the drupal_alter() call has $form_id set, but the direct call from system_theme_settings() does not.

Here’s another workaround. Not sure the comments make sense.

<?php
/**
* Implements hook_form_system_theme_settings_alter().
*
* Work around a core double invoke bug. This only needs to exist in the base
* theme. Use hook_form_theme_settings() for the real work.
*
* @see http://drupal.org/node/943212
* @see http://drupal.org/node/1135794
*/
function THEMENAME_form_system_theme_settings_alter(&$form, &$form_state, $form_id = NULL) {
 
// General "alters" use a form id. Settings should not be set here. The only
  // thing useful about this is if you need to alter the form for the running
  // theme and *not* the theme setting.
 
if (isset($form_id)) {
    return;
  }
 
// The following will be processed even if the theme is inactive.
  // If you are on a theme specific settings page but it is not an active
  // theme (example.com/admin/apearance/settings/THEME_NAME), it will
  // still be processed.
  // Build a list of themes related to the theme specific form. If the form
  // is specific to a sub-theme, all parent themes leading to it will have
  // hook_form_theme_settings invoked. For example, if a theme named
  // 'grandchild' has its settings form in focus, the following will be invoked.
  //
  // - parent_form_theme_settings()
  // - child_form_theme_settings()
  // - grandchild_form_theme_settings()
  //
  // If 'child' was in focus it will invoke:
  //
  // - parent_form_theme_settings()
  // - child_form_theme_settings()
 
$form_themes = array();
 
$themes = list_themes();
 
$_theme = $GLOBALS['theme_key'];
  while (isset(
$_theme)) {
   
$form_themes[$_theme] = $_theme;
   
$_theme = isset($themes[$_theme]->base_theme) ? $themes[$_theme]->base_theme : NULL;
  }
 
$form_themes = array_reverse($form_themes);
  foreach (
$form_themes as $theme_key) {
    if (
function_exists($form_settings = "{$theme_key}_form_theme_settings")) {
     
$form_settings($form, $form_state);
    }
  }
}
?>

@dvessel - that's working for me, absolutely awesome, for the first time I can actually use my own admin theme. Thank-you very much.

Nice work-around. Sorry about hosing all the admin themes in D7. :-p

subscribing. soon to try dvessel's workaround... thank you, sir

We need to agree on a fix for 8.x. Shall we rename THEME_form_system_theme_settings_alter() to something not in the drupal_alter namespace, say THEME_theme_settings() ?

There are some brief suggestions for a fix #1135794: Introduce hook_settings() for themes.

Status:Needs work» Active

Remove theme_form_FORMID_alter() and instuct to use the module hook instead? Or, like suggested above, rename theme_form_FORM_ID_alter() to theme_theme_form_FORMID_alter() or something.

Status:Active» Needs review
StatusFileSize
new2.68 KB
FAILED: [[SimpleTest]]: [MySQL] 46,397 pass(es), 35 fail(s), and 577 exception(s).
[ View ]

Alright so here is a reroll from #11. I warn you, this will likely be butchered.

StatusFileSize
new950 bytes
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/module.inc.
[ View ]

And here's a reroll of the patch from #7.

Status:Needs review» Needs work

The last submitted patch, drupal_alter-943212-31.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new14.53 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 943212-34-admin-themes.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Over in #1164790: “Settings” is not an immediately-expected term there is a discussion that the "settings" link is not clear. If we rename it to "configure" we can change system_theme_settings() to system_theme_config() and THEME_form_system_theme_settings_alter() to THEME_theme_config(). (That would also make the module's hook alter be hook_form_system_theme_config_alter().)

2 problem birds. 1 patchy stone.

Status:Needs review» Needs work

The last submitted patch, 943212-34-admin-themes.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new14.53 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 943212-36-admin-themes.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Wow. Didn't take long for the patch to stop applying. Retrying.

I was just bitten by this bug in Drupal 7. The key annoyance with this bug is that the form alter hook runs twice. Once from system_theme_settings() and again from drupal alter(). In my case, this behaviour caused it to attach form validate and submit alters twice, which then tried to save and delete custom CSS files twice. Super annoying.

Renaming the theme settings function to not conflict with the hook form_alter() namespace, as per patch #36, is a no brainer.

Drupal 7 hook_form_alter() documentation should also warn about using hook_form_system_theme_settings_alter() from your theme, and suggest the workaround in #24 to bail out if $form_id is not set.

Status:Needs review» Needs work
Issue tags:+Needs tests, +theme hooks, +needs backport to D7

The last submitted patch, 943212-36-admin-themes.patch, failed testing.

Issue tags:+Needs reroll

Tagging.

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

The summary may need to be updated with information from comments.

Assigned:Unassigned» arknoll

portland sprint, working on reroll

Status:Needs work» Needs review
StatusFileSize
new11.81 KB
FAILED: [[SimpleTest]]: [MySQL] 55,302 pass(es), 101 fail(s), and 5 exception(s).
[ View ]

Rerolled #36.

Status:Needs review» Needs work
Issue tags:-Needs tests, -Needs issue summary update, -theme hooks, -needs backport to D7, -Needs reroll

The last submitted patch, core-943212-7446826.patch, failed testing.

Status:Needs work» Needs review

#43: core-943212-7446826.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs tests, +Needs issue summary update, +theme hooks, +needs backport to D7, +Needs reroll

The last submitted patch, core-943212-7446826.patch, failed testing.

Issue summary updated.

Issue summary:View changes

Updated issue summary.

Issue summary looks very good to me, nice work @nathangervais!

Issue tags:-Needs reroll

#36 had 7 files changed, 39 insertions, 37 deletions.
#43 has 6 files changed, 32 insertions, 29 deletions.
I'm not sure
but removing the needs reroll tag.

Issue summary:View changes

Updated issue summary.