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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

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.

bfroehle’s picture

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.

bfroehle’s picture

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

webchick’s picture

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.

bfroehle’s picture

Title: Regression: Theme settings errors when admin theme and front end theme share the same base theme » THEME_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.

marcingy’s picture

Priority: Critical » Major

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

bfroehle’s picture

Status: Active » Needs review
FileSize
804 bytes

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

Jeff Burnz’s picture

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

sun’s picture

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.

Jeff Burnz’s picture

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

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

Jeff Burnz’s picture

Status: Needs review » Needs work

cross post

bfroehle’s picture

Status: Needs work » Needs review
FileSize
3.18 KB
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.

bfroehle’s picture

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.

bfroehle’s picture

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.

bfroehle’s picture

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.

dvessel’s picture

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.

sun’s picture

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.

dvessel’s picture

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.

yoroy’s picture

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.

catch’s picture

ouch, subscribing.

bfroehle’s picture

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:

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.

dvessel’s picture

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


/**
 * 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);
    }
  }
}
Jeff Burnz’s picture

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

JohnAlbin’s picture

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

aquariumtap’s picture

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

bfroehle’s picture

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.

bfroehle’s picture

Status: Needs work » Active
RobLoach’s picture

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.

ryan.gibson’s picture

Status: Active » Needs review
FileSize
2.68 KB

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

ryan.gibson’s picture

FileSize
950 bytes

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.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
14.53 KB

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.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
14.53 KB

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

treksler’s picture

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.

star-szr’s picture

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.

star-szr’s picture

Issue tags: +Needs reroll

Tagging.

dcam’s picture

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.

arknoll’s picture

Assigned: Unassigned » arknoll

portland sprint, working on reroll

arknoll’s picture

Status: Needs work » Needs review
FileSize
11.81 KB

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.

star-szr’s picture

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.

nathangervais’s picture

Issue summary updated.

nathangervais’s picture

Issue summary: View changes

Updated issue summary.

star-szr’s picture

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

YesCT’s picture

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.

YesCT’s picture

Issue summary: View changes

Updated issue summary.

steinmb’s picture

Assigned: arknoll » Unassigned
cilefen’s picture

I realized this may be the situation in Drupal 8 now. Bartik is a sub-theme of Classy, and so is Seven. I tried to reproduce the issue in the summary in HEAD but could not. Can someone more knowledgable of the theme system check if this is still an issue?

lauriii’s picture

Issue tags:

After seeing the code behind this, I wouldn't be amazed if this would still happen. Anyway this API is very fragile so we should at least investigate it has test coverage.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Issue tags:

I'd agree with #52 -- probably move this issue back to D7 only if it is not reproducible in D8, and create a normal followup in D8 for expanding the test coverage (using test themes, not Classy and its children).

JohnAlbin’s picture

Version: 8.1.x-dev » 8.2.x-dev

The only way to fix this fragile code is to change the API. The theme settings need to be added to the form _before_ the form finishes building so that modules and themes can _alter() that form.

That means the theme that owns the settings should be using a custom hook name (and not a pseudo- _alter() hook name). That kind of change is a backwards-incompatible change.

Which means we would need to write it in such a way that the old API is deprecated without being removed.

While the bug described may not happen any more due to Forms API changes, it is DEFINITELY still true, that a theme's THEME_form_system_theme_settings_alter() gets run _twice_. That could cause all sorts of unpleasantness if you aren't careful. I'm still using dvessel's work-around (#24) in my Drupal 8 themes.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

JurriaanRoelofs’s picture

I'm updating my theme to D8 and I can confirm this problem still exists. I make extensive use of theme settings and inside my hook_form_system_theme_settings_alter implementation I run a plugin system that includes files and those files contain functions within the hook_form_system_theme_settings_alter namespace.

Due to double loading that still exists in D8 this produces fatal errors due to redeclaring of the same functions. If I change my plugin system from this:

  foreach (file_scan_directory(drupal_get_path('theme', 'example') . '/features', '/settings.inc/i') as $file) {
    $theme = 'example';
    include($file->uri);
  }

To use require_once like this:

  foreach (file_scan_directory(drupal_get_path('example', 'glazed') . '/features', '/settings.inc/i') as $file) {
    $theme = 'example';
    require_once($file->uri);
  }

..the custom theme settings and functions in the included files are not loaded at all.

I use a similar workaround to #24, which I now sadly have to copy into my Drupal 8 theme-settings.php file:

function example_form_system_theme_settings_alter(&$form, &$form_state) {
  global $example_altered, $base_path, $theme_chain;
  if ($example_altered) return;
  $example_altered = TRUE;

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

savkaviktor16@gmail.com’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
12.93 KB

Re-rolled (redone according to current structure) #43's patch. I'm not sure that's done properly, but still.

markhalliwell’s picture

Status: Needs review » Needs work
Related issues: +#2869859: [PP-1] Refactor theme hooks/registry into plugin managers

I'm not entirely sure why the above patch is changing all the paths. IMO, this seems like overkill and a massive BC break, if only for all the [external] documentation purposes.

Also, renaming a hook, whether it clashes or not doesn't seem like the most appropriate thing to do either.

This has been a well known, and well documented, issue in contrib projects for years now.

Changing this now seems like it may cause more damage than its worth.

I'd rather we implement something like #2869859: [PP-1] Refactor theme hooks/registry into plugin managers, where themes would have the ability to create their own settings inside an OO class, thus bypassing the whole "hook clashing" issue altogether.

I'm tempted to just mark this issue as won't fix but will set it back to CNW in case someone else can come up with a better, more non-disruptive fix.

markhalliwell’s picture

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

tyler36’s picture

Status: Needs work » Closed (won't fix)

The video in the description requires flash to play.

Marking it as "wont fix" based on:
- was original open in 2010
- has had no activity for 5 year
- @markhalliwell comment in #63

Should anyone decided to reopen, please note the current (5 year old) patch has multiple references to "bartik", a theme that is no longer shipped with Drupal.