Improve theme_get_setting() and make custom theme settings a true form_alter

Subscribing for review

John,

I'll look at this patch today. Sounds promising!

Applied the patch to HEAD and tested the following functionality with the included themes.

- Verified that theme settings appear for both themes.
- Verified that theme settings from the base theme are inherited by the sub-theme.
- Verified that default values are retrieved from the .info file.
- Verified that, once changed, base and sub-theme theme settings are independent of each other.

- Verified adding my own theme settings using the THEME_form_theme_settings_alter() function, with default values set in the .info file.
- Verified that my own theme settings were correctly inherited.

That's what I can say with my initial testing. I'll try to do some more testing tomorrow.

As far as I can tell at this point, it's looking good for D7 code freeze. This is a critical step forward for true theme inheritance. Great work!

Patch applied with fuzz. Attached is simply chasing HEAD. I will follow it up with a code review and testing review.

Josh

Subscribing.com/org.net

User Interface Review

• The following notices happen everywhere...
  • Notice: Undefined index: search_theme_form in drupal_retrieve_form()
  • Warning: call_user_func_array(): First argument is expected to be a valid callback, 'search_theme_form' was given in drupal_retrieve_form()
• I think these notices are due to the form id not being passed correctly.
• Also, when I installed and enabled, Minelli, Liza, and Cabaret -- both options for the themes were evident on the Cabaret theme configuration page, but not on the Liza configuration page. I don't think that is what is intended.
• I love the themes default settings being set in theme.info

I did quickly look through the code... and found no coding style issues

I have grabbed a super fresh copy of HEAD and applied the patch posed in #6 and then installed. I did not get any notices or warnings that were mentioned above and things looked to be functioning perfectly.

As for my opinion, I like the way this works and is a definite improvement over 6's settings.

I have reviewed this new patch and have found no more problems. JohnAlbin wanted RobLoach to review, but I think he is not around today, so I will try and find more themers to review... if they are happy then RTBC.

Josh

Talked to mortendk and zarabadoo (sp?) in the hall and mortendk loves the idea of a form_alter() for theme form. Default values being set in .info was also a high point. zarabadoo plans to review and possibly RTBC it.

I have looked at this and it all seems to be functioning properly without errors. As said before, I approve completely and will mark this as a reviewed and tested patch.

Sorry for missing you guys at the code sprint, Josh and John. My flight left a bit earlier then I had thought. I got a chance to take a look at the patch though, and have to say that it really helps out the themer's experience when getting theme-specific settings. RTBC++.

Personally, I'm not a big fan of storing defaults in .info files. That seems to be a new feature? Why wouldn't we specify the defaults in _hook_alter() like we do elsewhere? If we want to aim for consistency, we should, IMO.

Also, should there be a theme.api.php where we keep such examples?

Dries,

I think being able to store theme defaults in .info files would be a big win for sub-themers. That way, a sub-theme could easily change the base theme defaults without having to add its own theme-settings.php file. It's a lot more accessible and flexible, and it should help encourage the creation of more themes in general.

And conceptually, .info already stores defaults for standard theme settings in the form of which Features to display (logo, name, slogan, etc.). The difference with this patch is that it would also include advanced (custom) theme settings as well. From a themer's point of view, it feels like a very "natural" place to put theme setting defaults for a particular theme or sub-theme.

This feature and the rest of this patch are important steps in cleaning up the theming/inheritance system for D7, so it can enable easier theme creation.

Um, I'm assuming that this patch is making it into D7 before the end of the code slush. Is that a correct assumption? Is there anything else that needs to be done in order to get this committed?

Looks like there was some kind of confusion regarding theme settings variable defaults in .info files. Apparently, that's a hidden "feature" we already have since D6 or earlier, because everything in .info is taken over into the registry.

This patch seems to bring theme_get_setting() more in line with variable_get(), so it returns a default value. That is good for themers, and that will also be good for the final variable defaults API change we all wait for and hopefully see in D8.

I'm still not a fan of abusing info files for settings -- I warned against that trend 3 years ago but was assured it wouldn't happen.

Either way, given that it was already a hidden feature in Drupal 6, I decided to commit this patch.

Is it really necessary that we document hook_form_system_theme_settings_alter()? We don't document any other hook_form_FORM_ID_alter() and it seems this would be more appropriate for the 'upgrading themes from D6 to D7' handbook.

Was not documented in the update guide.

Is this related? #591794: Allow themes to alter forms and page.

There is still a call to theme_get_settings() in search.test - I don't know how the test bot missed that.

Somehow, the patch doesn't apply. Requesting retest.

That's because I removed the 'toggle_search' setting, which is in the context of these lines.

Now that theme_get_settings() is gone, what is the preferred method for getting all of a theme's settings in one call?

Saving custom settings from admin panel does not work if form element is put inside fieldset. Am I doing something wrong here?

theme-settings.php:

function mytheme_form_system_theme_settings_alter(&$form, $form_state) {

  $form['sample_fieldset'] = array(
    '#title' => 'This is sample fieldset', 
    '#type' => 'fieldset', 
    '#tree' => TRUE,
  );

  $form['sample_fieldset']['sample_setting'] = array(
    '#title' => 'This is sample select form', 
    '#type' => 'select',
    '#default_value' => theme_get_setting('sample_setting'),
    '#options' => array(
      'option_1' => 'Option 1',
      'option_2' => 'Option 2',
    ),
  );

}

mytheme.info:

settings['sample_setting']   = 'option_2'

I just had to remove '#tree' => TRUE from fieldset definition. I don't know forms API very well yet, I'm guessing that #tree didn't make sense in this context anyway. So my issue probably was not related to reimplementation of theme_get_setting()

So far I have built quite big form with custom theme settings - works flawlessly.

The last theme_get_settings() call was removed in #550742: Remove Search box from theme system, default to block system instead, so I am setting this back to “fixed”.

What sort of documentation does this need -- can you be more specific?

Also, updating component etc. to how we indicate that a patch whose code has been fixed now needs documentation.

I've been converting themes to D7 and noticed that my base theme's settings weren't being inherited by sub-themes.

I also checked it out with a Garland sub-theme and found the same problem.

So, I traced it back to this line (1155) in theme.inc (HEAD 1.582) in the theme_get_setting() function which is supposed to get all the base themes, which are then used to loop through merging theme settings:

// Create a list which includes the current theme and all its base themes.
if (isset($theme_object->base_themes)) {

But the $theme_object doesn't have an entry for base_themes, just base_theme. So it fails the test, never gets the base themes, and the base themes are never checked for settings to inherit.

Following back to the list_themes() function in theme.inc that generates the $theme_object, it doesn't look like it ever generates the base_themes information.

I tried the following code to fix the problem in list_themes() and found that it allowed base theme settings to be inherited:

$theme->base_themes = system_find_base_themes(system_list('theme'), $theme->name);

Can anyone else replicate this behavior? Unless I'm missing something, this seems fairly major.

I've attached a patch for testing.

New patch chasing HEAD (theme.inc 1.583) that uses internal theme list (modified to use theme names as keys) and should remove dependency on db causing Drupal install failure.

#43: list_themes_base_themes_563708_2.patch queued for re-testing.

John,

Anything I can do to help out with this issue?

looking at the following code,

<?php
function foo_form_system_theme_settings_alter(&$form, $form_state) {
  $form['caberet_example'] = array(
    '#type'          => 'textfield',
    '#title'         => t('Widget'),
    '#default_value' => theme_get_setting('foo_example'),
    '#description'   => t("Place this text in the widget spot on your site."),
  );
}
?>

how exactly does theme_get_setting('foo example') here know which theme we are editting?
AFAICS, it does not.

It looks like no matter what theme you are editting, it will always save these settings in the current theme
Shouldn't theme settings files do

<?php
function foo_form_system_theme_settings_alter(&$form, $form_state, $theme) {
  $form['caberet_example'] = array(
    '#type'          => 'textfield',
    '#title'         => t('Widget'),
    '#default_value' => theme_get_setting('foo_example', $theme),
    '#description'   => t("Place this text in the widget spot on your site."),
  );
}
?>

and shouldn't this

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

be this

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

or perhaps more simply we could just have the theme settings files call

<?php
function foo_form_system_theme_settings_alter(&$form, $form_state) {
  $form['caberet_example'] = array(
    '#type'          => 'textfield',
    '#title'         => t('Widget'),
    '#default_value' => theme_get_setting('foo_example', 'foo'),
    '#description'   => t("Place this text in the widget spot on your site."),
  );
}
?>

also, should we not have also changed
$themes[$key]->prefix . '_engine_settings'
to
$themes[$key]->prefix . '_form_system_theme_settings_alter'

right now there is no way to update the themesettingsapi module because we can't tell whether we are dealing with phptemplate_engine_settings($settings) or phptemplate_engine_settings($form, $form_state)

it would be much clearer if we had phptemplate_engine_settings($settings) and phptemplate_form_system_theme_settings_alter($form, $form_state)

Tracking.

re: #40, I think we've identified where the base_themes array is being lost over at #761608: Missing theme settings values because list_themes() has inconsistent theme object data.

A very late realization but there's a bug related to this: #943212: hook_form_system_theme_settings_alter() vs. THEME_form_system_theme_settings_alter() name-clash

Unfortunate that with all these changes they missed allowing these theme variables being allowed to be set in the settings.php file.