Support from Acquia helps fund testing for Drupal Acquia logo

Comments

oscnet’s picture

Status: Active » Needs review
FileSize
1.93 KB

I add a function load_theme , and had searched the drupal project has not used this function.
I had tested the patch , and it worked.

http://drupal.org/node/49703#comment-108426

Heine’s picture

I tested the patch against CVS, but it results in some bizarre behaviour.

For example, when bluemarine is the active theme & I configure goofy, part of the page is themed as bluemarine, part of the page chameleon. Similarly when configuring marvin or pushbutton; headers are bluemarine, blocks marvin or pushbutton.

Minor nitpicks:
1 indent of load_key doesn't match surrounding
2 can you remove the suffixes .inc6, inc7 from the patch?

oscnet’s picture

ok.
I will try to get a new patch.

oscnet’s picture

FileSize
1.82 KB

a new patch ,seems worked. someone pls test it.
>2 can you remove the suffixes .inc6, inc7 from the patch?
I use eclipse to create patch. muliti file patch has those line.

Heine’s picture

Sorry, these suffixes were somehow fused to the .inc extension; it's my fault.

The current patch doesn't solve the problem, because a theme's phptemplate_settings is now called on configuration pages of other phptemplate themes.

oscnet’s picture

so can't load two theme file (a active theme file and a needed settings theme file ) they may be all have phptemplate_settings function. this may case error.
but do not load the user settings theme file which phptemplate_settings exist, drupal can not access the function for their self configure form .
Heine: have you any suggestion?

oscnet’s picture

or simply use the setting's theme instead of use current active theme on setting page?

Heine’s picture

Suggestion:

I think the issue is with system_theme_settings which calls theme[$key]->prefix_settings on 1196. Since prefix is phptemplate on phptemplate themes it calls phptemplate_settings.

Implementing phptemplate_settings in phptemplate.engine that defers the call to the appropriate callback (template.php) should work.

Heine’s picture

Or simply remove the call to init_theme() from system_theme_settings

Heine’s picture

FileSize
1.75 KB

Similar to above patch, but call to init_theme() removed.

Heine’s picture

FileSize
1.76 KB

Patch as above with minor style issues fixed.

oscnet’s picture

Heine : patch doesn't solve problem , when I use bluemarine theme ,and setting my own esjing theme config which has
phptemplate_settings function on template.php, the self setting form on phptemplate_settings is not displayed .

oscnet’s picture

I was wrong write above on #12 , i had not tested the patch carefully.
wait to me test it more carefully.

Heine’s picture

Well, that works for me. Turns out in further testing that when a theme with a phptemplate_settings is active and you're configuring another theme with a phptemplate_settings you'll get a cannot redeclare fatal error. Clearly this isn't going to work. I'm going to implement the suggestion I made above.

Heine’s picture

Status: Needs review » Active

Changing status

Heine’s picture

Configuring theme specific settings with another theme active is not going to work for themes that both declare phptemplate_something (eg. http://drupal.org/node/11811) in their template.php. The easiest workaround is to simply switch theme on the settings page.

Heine’s picture

FileSize
758 bytes

Attached patch, switches theme on the themes/settings pages, so you can add theme specific settings using the function phptemplate_settings() in template.php

I wonder if this shouldn't be a feature request.

The message of line 1205 ('Engine-specific settings') still has to be adapted.

Heine’s picture

Category: bug » feature
Status: Active » Needs review
FileSize
1.99 KB

Here's a 'better' patch, including the message.

From the original code, it appears that only engine-specific and true phptheme-specific settings are supported: I've changed this to a feature request. This means that http://drupal.org/node/56713 may need to be reverted as well.

forngren’s picture

Trying to keep up with head, but not tested.

forngren’s picture

Title: Couldn't setting theme Adding Extra Configuration Options when it is not active » Can't change settings of unactive themes + custom settings in phptemplate
pimpmastahq’s picture

Version: x.y.z » 5.1

It seems the patch submitted by Heine on April 7, 2006 works the best; switching the theme to successfully load template.php without theme hook functions colliding between multiple themes. However, I like to use garland as the Administration Theme for the back-end and my own themes for the front-end. This solution would force me to include the Drupal module css stylesheets and assumes that the content area in my theme is wide enough to accommodate the color.module and the rest of those form elements in admin/build/themes/settings/mytheme ... which simply isn't the case, and it shouldn't have to be.

For now, I'll build a module to include the phptemplate_settings() I need, and maybe it will be worthwhile enough that other themes will begin to use it, but ideally there should be a solution that can include this hook in each theme's template.php, as it appears to have been intended, without switching to that theme in the Administration page.

forngren’s picture

Version: 5.1 » 6.x-dev

Look at block_admin_display, it uses a somewhat similar approach. Perhaps we should borrow some code?

(http://drupal.org/node/54990 was marked dupe?

JohnAlbin’s picture

FileSize
3.14 KB

A couple of points:

  • It’s overkill to call init_theme() just to display a fieldset for a non-active theme.
  • The previous patches remove the Engine-specific settings feature, which may be in use for other theme engines.

I think the best way to go about this is to put a phptemplate_settings() call in the phptemplete.engine. And have it check for the existence of a settings.php file in the selected theme. That way you can have phptemplate_settings() return the contents of the settings.php file to system_theme_settings().

And you don’t have to worry about which theme is active when getting the settings.

The attached patch is nearly functional (read: non-functional) and needs some work. I won’t have time for a couple days, but I thought I would let someone else play with it in the interim.

JohnAlbin’s picture

Title: Can't change settings of unactive themes + custom settings in phptemplate » Custom theme settings in phptemplate / Can't change settings of unactive themes
JohnAlbin’s picture

FileSize
3.47 KB

Here’s the updated, functional patch that implements this feature.

To co-exist with the PHPTemplate’s goals of allowing easy modification by end-users, no function name or file name includes the theme name.

Instead, the $form elements are added via a settings.php file that resides in the theme’s directory. PHPTemplate engine’s phptemplate_settings() is called to include that file and to return the resulting $form array.

Since settings.php is separate from template.php, one can easily modify a non-active theme’s settings without worrying about function name collisions with the active theme’s template.php.

JohnAlbin’s picture

FileSize
3.47 KB

Here’s the updated, functional patch that implements this feature.

To co-exist with the PHPTemplate’s goals of allowing easy modification by end-users, no function name or file name includes the theme name.

Instead, the $form elements are added via a settings.php file that resides in the theme’s directory. PHPTemplate engine’s phptemplate_settings() is called to include that file and to return the resulting $form array.

Since settings.php is separate from template.php, one can easily modify a non-active theme’s settings without worrying about function name collisions with the active theme’s template.php.

JohnAlbin’s picture

Status: Needs review » Needs work

There are significant changes between D5 and D6 in the objects returned by list_themes(). Need to re-roll patch to reflect those changes. D6 reports: “notice: Undefined property: description.”

JohnAlbin’s picture

Title: Custom theme settings in phptemplate / Can't change settings of unactive themes » Custom theme settings improvements (+ PHPTemplate theme settings)
Status: Needs work » Needs review
FileSize
3.67 KB

This patch allows separate theme engine settings and theme settings.

And implements theme-specific settings in PHPTemplate.

It is also theme.info file aware (which the un-patched code was not.)

hass’s picture

@JohnAlbin: i'd like to test this importend feature with my themes... do you have an example settings.php?

forngren’s picture

I suppose that this should work as settings.php

  $form['dummy'] = array(
    '#type' => 'checkbox',
    '#title' => t('Dummy setting'),
    '#default_value' => variable_get('dummy', FALSE),
    '#description' => t('Dummy setting to test JohnAlbins patch.'),
  );

However, I have not tested it. So if something is broken it's either the patch or my crappy dummy code.

JohnAlbin’s picture

Hass, I meant to post this earlier!

I’ve been adding a file at themes/garland/settings.php with the following content:

<?php

// Set the default values for the theme variables
$defaults = array(
  'garland_shoes' => 1,
);

// Merge the variables and their default values ($variables will contain the currently saved settings)
$variables = array_merge($defaults, $variables);

// Create the form
$form['garland_shoes'] = array(
  '#type' => 'checkbox',
  '#title' => t('Use ruby red slippers'),
  '#default_value' => $variables['garland_shoes'],
);

Johan’s code won’t work since a theme’s settings are stored as an array named theme_[theme name]_settings, rather than as individual variables.

hass’s picture

John, let me give some feedback, please.

1. i'm not sure if the core developers will accept code that is not a hook and will not integrate 100% in current logic's. This patch will change some well known development experience and code structure. maybe somebody from core can say more?

2. i read about the discussion about init_theme or switching theme for the settings page. i don't know why this is overkill, while this happen very rare! Additional to this, it will solve some problems i'm going to run in. I'd like to a integrate setting that allows to switch the type of navigation. For e.g. i deliver ~20 themes with Sliding Doors 1, Sliding Doors 2 and Shiny Buttons. It will be a very nice feature to save this setting and have the theme settings page switched to the changed design. Additional i'd like to implement a page width setting and such things. I think such features are better to administer if we switch the theme like we do in blocks, where i see the changes in real time, too.

3. thinking about the above i'd like more to have a hook_settings implemented inside template.php

JohnAlbin’s picture

Status: Needs review » Needs work

Okay, Hass. You’ve convinced me. I can see where init_theme() would be useful when changing theme settings.

I’m working on a new patch that adds that to system_theme_settings() and creates a phptemplate_theme_settings() hook to go in the template.php files. (We can’t use the phptemplate_settings() name since that function has to go in the phptemplate.engine.)

hass’s picture

sounds good. why not using mythemename_settings() like the mythemename_regions()?

JohnAlbin’s picture

Hass, all of PHPTemplate’s hooks can be called either as [theme_name]_hook() or as phptemplate_hook(). So if we used [theme_name]_settings(), we would have to support using phptemplate_settings() to get theme-specific settings.

But, phptemplate_settings() is already in use as an existing [theme_engine]_settings() hook to get theme-engine-specific settings. So I don’t feel right removing that existing hook without investigating on whether/how it is being used by other theme engines.

Thus, the [theme_name]_theme_settings() name.

JohnAlbin’s picture

FileSize
3.71 KB

Here’s an updated patch.

To test the functionality, you can add the following function to garland’s template.php file:

function phptemplate_theme_settings($key) {
  // Set the default values for the theme variables
  $defaults = array(
    'garland_shoes' => 1,
  );

  // Merge the saved variables and their default values
  $settings = array_merge($defaults, variable_get(str_replace('/', '_', 'theme_'. $key .'_settings'), array()));

  // Create the form
  $form['garland_shoes'] = array(
    '#type' => 'checkbox',
    '#title' => t('Use ruby red slippers'),
    '#default_value' => $settings['garland_shoes'],
  );

  return $form;
}

When testing this patch, please note bug 144162 (Theme settings not saved.)

hass’s picture

hm - i'd like more to see this hook named without "_theme_"... if required other theme engines should update and fix their problems... phptemplate is drupal core since 4.7.x and should therefor work perfectly. We shouldn't spend much time on backward compatibility for something outdated and nobody is using any more...

JohnAlbin’s picture

FileSize
2.68 KB

Fourth time’s the charm!

After reading some comments by Earl Miles about Theming changes in Drupal 6, I realized that the custom theme settings API needs to be re-jiggerred to be inline with those Theme changes.

Drupal 5’s API called ENGINE_settings() for themes that were based on theme engines OR it called THEME_settings() for plain (.theme) themes. Note that the _settings functions had to figure out what the saved settings were on their own.

This patch:

  • Calls ENGINE_engine_settings($settings) to get theme-engine-specific settings (instead of ENGINE_settings()).
  • Then gets the theme-specific settings by calling THEME_settings($settings) OR ENGINE_settings($settings).

Passing $settings to each of the _settings functions makes it really easy for them to merge the defaults with the saved settings.

JohnAlbin’s picture

FileSize
468 bytes

To test this patch, you can add the attached phptemplate_settings() function to garland’s phptemplate.php file.

JohnAlbin’s picture

Status: Needs work » Needs review

Please review, of course!

hass’s picture

Status: Needs review » Reviewed & tested by the community

Works like a charm, patch apply's cleanly, theme settings are saved and read (i tested my own settings). Let's commit. THX

moshe weitzman’s picture

yup - works well.

if themes could use hook_form_alter, they could inject own form fields but they can't so this looks good.

Dries’s picture

Before I commit this, I'd like to know two things:

1. What is the best place to document this? It's not really documented in the code, and there is no example in core.

2. Are there core themes/modules that could take advantage of this patch to clean up some stuff?

(A hook_form_alter approach would probably be better, but might be difficult. With namespaces et al.)

JohnAlbin’s picture

1. The existing custom theme settings API is already NOT documented (I stumbled upon the code while using a debugger.) But having said that, I would be more than happy to write docs for the updated API. I would think this is one good place: http://api.drupal.org/api/HEAD/group/themeable (using Angie’s API update method) And also in the Theme developer’s handbook, there should be a “Creating customizable themes (colors, etc.)” section (and move “Integrating with Color.module” under that umbrella.) I’ve just applied for docs maintainer status today.

2. Possibly. But since I don’t use any core themes on a regular basis, I can’t answer this without further investigation. Anyone else know a good target? A core theme using this API would certainly make the API a lot more visible. If no one else replies, I could investigate this before June 1, but can we commit first? :-D

hass’s picture

Dries

commit this patch, please.

#1: Let is write a handbook page - no problem.

#2: I looked inside core themes and haven't found anything that must be turned into a setting. We can think about an example setting to change "fixed page width" for minelli, if an example *must* be added to core themes... but i feel it's better to add a handbook page as first step. however this shouldn't hold up the commit...

I'm using themesettingsapi 5.x in over 20 theme with different settings (different primary/vertical navigations, fixed and fluid page width settings, footer link settings and some more) and this works very well. I see this a little bit as a "hack" and i'd like to get this removed and well integrated to wipe out the hacks an extra files required by themesettingsapi module...

Regards
Alex

JohnAlbin’s picture

2. I’ve looked carefully at all the core themes and the only item I see that could definitely be cleaned up is pushbutton theme’s duplicate navigation. It display primary and secondary navigation in both the header and the footer. We could add a custom setting that asks the user where to display the navigation (header, footer, both). Does that sound like a good candidate? That would be a separate patch and/or issue, yes?

forngren’s picture

Yes, that should be a separate patch if we want to do it.

hass’s picture

well, sounds good. someone watching is??? RTBC... only one day...

forngren’s picture

FileSize
2.63 KB

The same patch with some offset/fuzz fixed.

JohnAlbin’s picture

I have docs maintainer status now, but it seems presumptuous to add pages of docs for an API that hasn't been committed. ;-) Dries?

To review, we need (and I’ll write) docs in the following places:

Steven’s picture

Status: Reviewed & tested by the community » Needs review

Forcing more admin pages to display under a particular theme is a step back IMO and defeats the whole point of having an admin theme setting. Especially because those pages (theme config, block config) are table- or fieldset-heavy.

The theme system calls have always been intended only for a single active template. Removing dependencies on theme activation was also a major factor in moving regions and features to theme's .info files.

I'd much rather prefer a solution with a separate settings include file, referenced from the theme's info file (but with a sane default for 99% of all cases, just like the other fields). This will lead to a more consistent admin experience IMO.

merlinofchaos’s picture

subscribing

hass’s picture

Status: Needs review » Reviewed & tested by the community

@Steven: please read #2 and #3 in http://drupal.org/node/57676#comment-242698

it is nearly a "no go", not to switch theme on the settings page.

JohnAlbin’s picture

For block settings, It looks like the switching of themes has indeed been removed from 6.x. (As a side note, there’s a bug #153905 such that only the active theme’s region show up for every theme’s block settings.)

In 5.x, the theme was switched for purely technical reasons (there was no other way to get the regions since they were defined in template.php.) But as a consequence, administrators were able to get a live view of the region arrangement (and also the block arrangement) for non-active themes. IMO, this was a huge usability improvement. In 6.x, just seeing a list of region names doesn’t explain how a region is used.

With theme settings, the benefit of a seeing the effects of settings is even more useful than seeing the block region arrangement. Imagine a theme that allows the admin to change the widths of content and sidebar regions. Or one that changes the placement of the primary links.

It would be impossible to sanely change theme settings without seeing the live theme. So if we remove the theme-switching functionality from this patch, we are effectively telling admins they have to go to /user/1 to switch their theme and then go back to /admin/build/themes/settings/example to make the changes.

Also, the alternative, using a separate settings.php file, is less elegant because you lose the ability to have a well defined hook; you can’t put any function definitions in a settings.php file, since the active theme’s settings.php will also be included.

forngren’s picture

I agree with John, switching theme is an usability improvement. Not doing so is like sending a friend to shop/try clothes for you ;)

Steven’s picture

Status: Reviewed & tested by the community » Needs review

While I see the value of switching the theme, I think it is overrated, especially for theme settings. You're much better off just making your settings themselves more WYSIWYG rather than the page. Layout toggles and navigation bar styles can just as well come with images to pick from, for example, and this would let you compare options side by side rather than having to try each out and reloading the entire page.

It is only in comparison to a bare list of checkboxes or dropdowns that switching the entire theme looks interesting.

You're also making the assumption that the settings page will be representative for the rest of the theme, whereas high quality themes have many different design elements and styles, which are rarely all used at once. The settings page is just a big form.

So I would propose that we only switch themes as a courtesy, and only when no separate admin theme is set. (same for the block region markers). My beef is not with the switching per se, but that it is hardcoded into the theme system.

dmitrig01’s picture

Status: Needs review » Needs work
JohnAlbin’s picture

Assigned: Unassigned » JohnAlbin
Status: Needs work » Needs review
FileSize
3.95 KB

I very much like Steven’s proposal; this patch now only switches themes if the admin theme is not set.

Since functions in the non-active theme’s template.php will likely conflict with those in the admin theme’s template.php, we’ll have to use a separate file for custom theme settings.

The API in this patch is still the same as described in comment #38 above. Except you have to put the THEME_settings() or ENGINE_settings() in a separate settings.php file.

To test the patch, create a settings.php file in garland and add the phptemplate_settings() function from comment #39 above. Don’t forget the <?php!

hass’s picture

Status: Needs review » Needs work

why does the patch change RCS file: /cvs/drupal/drupal/.htaccess,v?

hass’s picture

First time i heard about this blocking admin theme in D6... From usability side i'm not happy about this if (variable_get('admin_theme', '0') == '0') {. It will for sure result in many surprised users - asking, why the theme is not switching on box one, but not on box two. Most people will not understand this internal logic and it is not documented yet!

We should at minimum extend the text in "admin/settings/admin" about the bad side effect that prohibits dynamic theme switching for theme settings, blocks and other sections, if an specific admin theme is configured. Additional the theme settings page should dynamically display, if admin theme overwrite is done and dynamic theme switching is blocked and why! best in big red letters - like aggressive cache mode module warnings!

hass’s picture

Status: Needs work » Needs review
FileSize
3.33 KB

maybe like this patch. it fixes:

1. removed .htaccess from patch
2. outputs a drupal_message if theme overwrite is active

JohnAlbin’s picture

FileSize
4.49 KB

why does the patch change RCS file .htaccess?

Oh, ****. That’s what I get for rolling a patch after midnight. I’m glad 6.x no longer includes /sites/default/settings.php; otherwise, the patch would have shown you my database username and password! :-)

We should at minimum extend the text in "admin/settings/admin" about the bad side effect that prohibits dynamic theme switching for theme settings, blocks and other sections, if an specific admin theme is configured.

Actually, continuing to use the admin theme for the theme settings page is the expected behavior. Doing the theme switching is unexpected and disconcerting to new users, but extremely useful once you realize why it’s switching themes.

To optionally allow theme switching for sites with an admin theme, I’ve added a “Use admin theme when configuring theme settings” checkbox (enabled by default) on the /admin/settings/admin page.

hass’s picture

Well, this last patch is much better for usability, but not very logical. The new checkbox in "admin/settings/admin" is active and "System default" selected and the theme is switching, but it shouldn't...

JohnAlbin’s picture

The new checkbox says:

Use administration theme when configuring theme settings.
Use the administration theme rather than switching to the theme being configured.

The wording was chosen to be consistent with the other checkbox on that page:

Use administration theme for content editing
Use the administration theme when editing existing nodes or creating new ones.

The default setting for “administration theme” is to not use one. So the theme switching is enabled.

If the user wants an administration theme for all the admin pages, all they need to do is select a theme from the pull-down menu. This disables theme switching on the configure theme page and is the expected behavior. If the user now wants to enable theme switching while still using an administration theme for the rest of the admin pages, they can un-check the new checkbox.

It seems quite logical to me, but I wrote it. :-) The new checkbox is just a bonus, but should be removed if it‘s confusing.

forngren’s picture

The patch seems RTBC, but there is one minor issue that I'm concerned about. The new setting is inconsistent with block theme switching. Either this setting should affect both or another one should be introduced, IMO.

JohnAlbin’s picture

If theme switching for block regions was still in 6.x, I would agree that this checkbox should control both behaviors. But theme switching for block regions has been completely removed from 6.x. If we want it back in, we should open a new issue.

hass’s picture

The checkbox is not confusing... this is a very good idea. I'm only coming from the text side. I've extended the text to make clear what i'm talking about.

Use administration theme when configuring theme settings. Use the administration theme rather than switching to the theme being configured. This setting have no effect if "System default" is selected as Administration theme.

Nevertheless, this patch works for me and looks RTBC except the minor text change.

forngren’s picture

FileSize
4.54 KB

Oops, didn't notice that block switching was dropped. #?

Re-rolled with Hass's suggested text change.

JohnAlbin’s picture

FileSize
4.54 KB

Hass, I agree the text is lacking. How about the shorter sentence below?

Use administration theme when configuring theme settings.
If this setting is disabled or if using the "System default" theme, the theme settings pages will be switched to the theme being configured.

Re-rolled with updated checkbox description.

JohnAlbin’s picture

Whoops! Johan, I guess we were re-rolling at the same time; I didn’t see your patch. Anyway, please review the proposed checkbox description.

forngren’s picture

Status: Needs review » Reviewed & tested by the community

I like the new wording. Great patch, heavily tested and polished.

Nice work, John!

hass’s picture

Now, what are we waiting for? It's ready! READY...

JohnAlbin’s picture

Version: 6.x-dev » 7.x-dev
FileSize
4.2 KB

Re-rolled against HEAD.

See the comments (including my own) in this drupal-devel thread for why this patch isn’t going into 6.x.

hass’s picture

Version: 7.x-dev » 6.x-dev

The code freeze has been extended by another two weeks... so we need someone to look on this very small code part...

Let's try to get this in now. It's a bug or broken feature since 4.7.x, not really a new feature! post-freeze should be an option for bugs, what this is. If we cannot get this in now we should move this to a bug, what it is.

JohnAlbin’s picture

Category: feature » bug

When Heine changed this issue from a bug report to a feature request he stated “From the original code, it appears that only engine-specific and true phptheme-specific settings are supported.” Actually, when Custom Theme Settings were introduced in Drupal 4.2 the idea was that it was supporting engine-specific and default-theme-engine-specific settings. When the default theme engine was changed to PHPTemplate for 4.7, the Custom Theme Settings became a broken API.

I’m setting this back to a bug report.

So, the Custom Theme Settings API needs to either be fixed to reflect the new default theme engine (as this patch does) or it should be removed completely (issue #156655). It’s nonsensical to have a theme-related API that does not support the default theme engine.

dvessel’s picture

I would love this feature. Too bad I discovered it too late. :(

Please tell me it's not too late. Still applies with 2 hunk offsets.

hass’s picture

Let's find someone able to commit bugfixes :-)

Gábor Hojtsy’s picture

The patch seems to mix two things together:

- admin config option on what theme to use for theme settings (which I don't yet understand why is related, or needed, I did not have time to study all comments, and did not find a summary in any of the comments)
- fixing the API calls to settings forms with engine and theme specific settings

Could someone provide me with an overview of what is happening here?

hass’s picture

Yep,

- the API fixes allows "theme settings" in general and fixes the currently broken api.
- the admin setting is to allow optional theme switching with the configured settings. take a look in http://drupal.org/node/57676#comment-242698 point #2 to read about some examples.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

1. I don't think the theme switching is a good idea to add now (it is so late). It also makes for a hard to understand conditional on theme configurators: "if not using an admin theme, or if I specifically ask for theme settings page switching" seems to be overkill. Save this for Drupal 7.

2. The theme settings.php part seems to be another new feature for themes, but admittedly one to fix a broken feature. If I remember right, themes used to be able to have a settings hook defined in their *template.php*, and the *settings.php* naming is introduced in this patch, and would be a new file. Am I right? How did this feature work before (when it did work)?

hass’s picture

I think it's best to start reading on top of this thread... :-)

There was some discussion here about a pro and contra of a settings hook inside template.php and the move to settings.php. In past there was a hook_settings() in .theme engine. But this old variant will not work - see http://drupal.org/node/57676#comment-87685.

Maybe we are able to wipe out the admin setting, but repair the hook_settings bug?

JohnAlbin’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3.08 KB

Gábor: No need to start reading at the top. Let me summarize for you.

The way it works in 5.x: If you had a plain php theme, you could add a THEME_settings() function to your THEME.theme file. If you had a phptemplate theme, you were hosed. You could add a phptemplate_settings() function to your template.php file and you could edit your custom settings while the theme was an active theme, but the custom settings would be erased if you went to the admin/build/themes/settings/THEME page while the theme was an inactive theme because the API wouldn’t show the custom theme settings form fields for an inactive phptemplate theme.

In order to fix the API, we have to be able to edit the settings when the theme is inactive.

If we were to try to include an inactive theme’s template.php, it would fail with function re-declaration errors. There are 2 ways to fix that problem:

  • Force a theme switch, so that template.php file of the admin theme is NOT included; only the template.php file of the theme being configured is included. This option was nixed by Steven in #51 and #56 above.
  • Use a separate settings.php file. The settings.php file of the admin theme is not included, so there’s no conflict with the settings.php file of theme being configured.

Hopefully that explains why we have to use a separate settings.php file.

This new patch is the same as before, but with the theme switching removed. Since this new patch only removes lines that were added by the previous patch, I’m setting this back to RTBC.

webchick’s picture

I've read JohnAlbin's summary (thanks!), and also tried to grok everything in the thread. What a hairy problem, ugh.

I really don't like the idea of adding settings.php files to themes. :( I think this further muddies the already muddy waters for Drupal newbies. There are already at least 2 different places settings.php files might be (sites/default/settings.php, sites/XXX/settings.php), now we're adding another, but for a file that will contain something TOTALLY DIFFERENT than what's in sites/X/settings.php (if I understand it correctly, the theme's settings.php will contain just a simple form declaration to use as the settings form). If we must solve the problem this way, let's name the file something like theme.settings or theme_settings.php or whatever so it's less confusing.

I still think the most straight-forward way to fix this is when configure is clicked, switch the theme for that one configure page *only*, which I believe was the approach taken by one of the earlier patches. I know this was nixed by Steven above (and there are some silly themes out there that are useless for admin pages), but the solution outlined here seems to be muddying up the API.

Ideally, all of this business would be right in the themes' .info files, which are already pulled into memory for non-enabled themes in order to parse the theme name/description. This is also much easier to modify than PHP. I'm not sure how to represent FAPI in INI-ish format, though... maybe like:

settings[] = (
  name: "themename_fixed",
  type: "checkbox",
  title: "Fixed width",
  description: "Constrain theme to fixed-width?"
  default: 0
)
settings[] = (
  name: "themename_fixed_width"
  type: "textfield",
  title: "Fixed width size",
  description: "Width of page in pixels, if fixed-width."
  default: 800

(that syntax is likely wrong, but you hopefully get the idea...)

I looked through maybe half a dozen theme settings hooks, and all seemed to only be adding their own form settings.

JohnAlbin’s picture

Angie, I thought about the .ini files, but the problem is that it severely limits what you can do in the form.

For example, I attach javascript via drupal_add_js() in my theme’s call to phptemplate_settings(). This allows all kinds of nifty usability improvements. Like hiding advanced options until a specific standard option is chosen. Or having a theme preview that dynamically changes depending on the selected options. (Steven perfers this option over theme switching in #56 above.) You could also have options that depend on installed modules via module_exists().

Having real PHP code is essential, IMO.

The solution outlined here seems to be muddying up the API.

Muddying it how? By having an extra theme file? Or by having new theme settings hooks like THEME_settings(), ENGINE_settings(), and ENGINE_engine_settings()?

If it’s the new hooks, I think it’s important to have the theme settings be consistent with the other new template hooks.

If it’s the new file, there’s no other solution since we can’t force theme switching per Steven’s comments.

If it’s just the name of the new file, I do think it is a good idea to rename settings.php to something else to lessen any confusion with the site’s settings.php file.

webchick’s picture

Right. By muddying the API, I meant both adding a new file to the theme that stores only PHP code for settings pages, when logically it makes more sense to keep this with template.php, along with all the rest of the "code stuff." (I do understand all the reasons why this won't work.) I also understand why .info files are not the ideal format for this, but it seemed like it might be an interim solution for D6 to allow us to re-think this for D7.

However, regarding the patch in specific, I *do* feel strongly though that it should be renamed from settings.php. I don't particular care to what, but let's *not* further overload a file name that already confuses the crap out of people new to Drupal.

JohnAlbin’s picture

FileSize
3.1 KB

Agreed. New patch replaces “settings.php” with “theme_settings.php”; no other code changes.

dvessel’s picture

I previously glanced at this thread, and agree it should be fixed (I just didn't look close enough at how it did so). But I like webchicks idea of using .info files. Might not be as flexible but as she mentioned, how about we hold off till we find a better fix for d7.

I think it would have a better chance of getting committed since .info files will be standard part of core.

hass’s picture

.info files are more static. That's doesn't sound like a good idea, while i'm unable to add extra HTML in a way i'd like. It may work for simple checkboxes and such things, but cannot be complex. Additional i cannot build SQL inside .info files to get some special fields and so on and so on... that's too limited for many cases.

JohnAlbin’s picture

Joon, see my comment in #84 above about why there is no way you can shoehorn FAPI (which is a full PHP API) into .info files (which are ini-style files and can’t hold PHP.) Any form that isn’t built with FAPI is a step backwards.

I know that some of you following this thread are hesitant to support this patch, but the problem is that there are several existing Drupal features that directly conflict with each other when trying to fix this bug.

  1. The site admin can edit the settings of non-active themes. (Introduced in Drupal 4.2.)
  2. PHPTemplate themes can implement theme hooks as phptemplate_hook() so that it’s easier to copy and modify themes. (Introduced in Drupal 4.7.)
  3. An “administration theme” can be used to over-ride the default theme while in the administration section. (Introduced in Drupal 5.0.)

We can implement a cleaner API, but we have to break one of the above features.

  • Because of #2, trying to include a non-active theme’s template.php will conflict with functions in the active theme’s template.php.
  • Because of #3, you can’t force a theme switch to the non-active theme; the admin theme should always take precedence.
  • And, of course, #1 is what is broken.

From my perspective, the only solution that respects all three features is putting the theme’s custom settings into a file that is separate from template.php. It’s the only solution. Does anyone disagree?

I’m more than happy to debate/modify the details of the patch, but I see no alternative to a separate settings file.

To make it easier to debate the details, here’s a recap of how the patch works:

  • Calls ENGINE_engine_settings($settings) to get theme-engine-specific settings (instead of ENGINE_settings() as in 5.x.)
  • Includes a theme_settings.php file that resides in the theme’s directory. Then gets the theme-specific settings by calling THEME_settings($settings) OR ENGINE_settings($settings).
  • Passing $settings to each of the _settings() functions makes it possible for the hooks to set the default values of the custom settings.

I would love to make the API better by 1.) allowing optional theme switching, 2.) allowing themes to edit the entire form and not just add a fieldset to the form (ala THEME_settings(&$form, $settings)), and 3.) storing the default values of custom fields in THEME.ini. But those are all feature requests (ones I will work on) that don’t belong in a bug fix patch.

Apologies for the long post. :-)

Gábor Hojtsy’s picture

IMHO it was awkward and keeps being awkward, that themes are supposed to implement conflicting named functions. An ideal fix for this bug would change that, but this is definitely not possible in Drupal 6, in its frozen state. Even this settings.php change might be too late. I wrote to Dries yesterday to ask him to review this bug exactly for this reason.

dvessel’s picture

John, I'm beginning to understand a bit more about this.

As Steven said before, how about using the .info file to create the file name and location. That idea should be extended to color.module also for including color.inc.

Dries’s picture

I spent a good 40 minutes analyzing this problem, and I agree with John's assesment in #89. I'm OK with the proposed patch, except that I don't like the name theme_settings.php. I actually prefer settings.php but I could also live with theme-settings.php. All other files use a dash, not an underscore.

From a user-point of view, it would be great if users weren't exposed to the difference between a theme and an engine. So far, this has been an implementation detail, but looking at the proposed patch, it seems like that this difference is being exposed to the user now. Do we really need per-engine settings?

Because this patch fixes an important problem, I'm OK with committing this.

webchick’s picture

One of the main arguments against re-using the name 'settings.php', beyond the confusion of an overloaded name, is in searchability. If I'm a theme developer, and want to know how to add custom settings with my theme, if I search for "settings.php" I'll get a bunch of users' support requests who can't figure out how the one under the site directory's supposed to work.

+1 to theme-settings.php.

JohnAlbin’s picture

FileSize
3.1 KB

Dries, per-engine theme settings have been a part of the API since 4.2. This patch just brings along that feature for the ride, but I have no idea if any engine actually uses it.

The fieldset description that says “These settings only exist for all the templates and styles based on the %engine theme engine.” is verbatim from the old code.

New patch changes theme_settings.php to theme-settings.php and applies to the new system.admin.inc file.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

As Dries was fine with this approach, I reviewed the patch once more, and went ahead with committing it. A long standing problem now solved. Thanks for your efforts.

Note, that this needs to be documented, so I am setting it to "code needs work" in tradition. Once you have this documented in the upgrade guide, come back and set it to fixed.

dvessel’s picture

John, could you please document this inside the upgrade guide. If you don't have proper permission then do it inside the comment and I'll move it into the main page.

Thanks!

http://drupal.org/node/132442

JohnAlbin’s picture

Status: Needs work » Fixed

I've added a new “Custom theme settings” page to the Theme developer's handbook and updated the Converting 5.x themes to 6.x page.

Anonymous’s picture

Status: Fixed » Closed (fixed)