When we took out #base en masse, we added a #theme property to system_settings_form(). Because we use system_settings_form() in many places, hard coding this #theme value causes problems when you want your module's settings form to have a different #theme. Since there isn't a theme_system_settings_form() function anyway, we can just remove the line entirely and avoid the problem.

This bug was introduced in the Remove #base entirely issue, and is very similar to the System settings are not validated bug.

Previously this sort of thing was common:

function my_module_settings() {
  $form = array();
  // Setup form fields.
  return system_settings_form($form);
}

But if you want to theme that form you'll need to do this currently:

function my_module_settings() {
  $form = array();
  // Setup form fields.
  $form = system_settings_form($form);
  $form['#theme'] = 'my_module_settings';
}

This sort of doubling-up on the #theme function isn't necessary since we're never using #theme = 'system_settings_form' anyway.

CommentFileSizeAuthor
#10 drupal.system-settings-form-theme.patch2.57 KBsun
#6 drupal.confirm-form-theme.patch872 bytessun
system_settings_theme.patch591 bytesquicksketch

Comments

quicksketch’s picture

quicksketch
he/him
commented
Status: Active » Needs review
ppmax’s picture

ppmax commented

I just tried theming system_settings_form using the technique above and didnt achieve success. here's a summary of my code:

function myform_settings() {
//bunch of form fields;
$form = system_settings_form($form);
$form['#theme'] = 'myform_settings';
}

function theme_myform_settings() {
//bunch of $output .= 'blaa';
return $output . drupal_render($form);
}

should this work?

thx
PP

quicksketch’s picture

quicksketch
he/him
commented

You also need to register your theme function in the hook_theme() of your module. It won't work without it. http://api.drupal.org/api/function/hook_theme

ppmax’s picture

ppmax commented

Thanks for the reply--much appreciated.

>>You also need to register your theme function in the hook_theme() of your module. It won't work without it.
I have a mymodele_theme() function:

function pptaxonomy_menu_theme() {
  return array(
    'pptaxonomy_menu_admin_settings' => array(
      'arguments' => array('form' => NULL),
    ),
  );
}

I'll try it again--it was late and I may have messed something up along the way. I'll report back after I get a chance to give it another go. Any idea when the patch above will be accepted for release?

thx
pp

mattyoung’s picture

mattyoung commented

.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Version: 6.x-dev » 7.x-dev
StatusFileSize
new drupal.confirm-form-theme.patch872 bytes

Holy cow. How can this fine patch languish in the queue for so long?

confirm_form() itself is a nightmare from Drupal 3. We can't fix that now, but we can and must at least make it properly alterable and themeable!

Attached patch fixes this issue in a 100% flexible way. (yes, there is a theme_confirm_form() now)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

And, btw, if you want to help to eliminate that horrible confirm_form() construct for D8, then welcome to #588550: Allow the user edit form to only ask for the current password when necessary (in a followup confirmation step). ;)

quicksketch’s picture

quicksketch
he/him
commented
Status: Needs review » Reviewed & tested by the community

Nice sun, a better approach than my original patch. Ready to go.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Postponed (maintainer needs more info)

I committed #6 to HEAD; however, I don't see how it helps with the title of the original issue? It seems that it's only for confirm forms..?

Have no idea what to mark this issue, so I'll go with this. :)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new drupal.system-settings-form-theme.patch2.57 KB

LOL! hahahahah.... - well, seems like I was working on too many confirm_form() issues ;)

Right, system_settings_form() has the very same issue. So now we do the same for it.

This one is a bit larger, because I additionally realized that we're loading system.admin.inc for all system_settings_forms, which is totally insane.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

ok, uhm, since the original patch was accepted, and this patch does _exactly_ the same, I'm feeling a little bit bold and mark that RTBC. Feel free to slap me! ;)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Cool, committed to HEAD. Thanks. :)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.