Currently the theme_options function conditionally adds a bunch of classes and calls drupal_render on the conditional settings form items. A better implementation would be to do these organizational changes in a pre_render function so that an overriding theme function does not need to also implement these needed alterations.

This patch move most of that work to a pre_render function so that the theme function can be as simplistic as possible.

CommentFileSizeAuthor
#7 options_element_pre_render-1009104-7.patch3.59 KBmstef
#6 options_element_pre_render.patch4.01 KBeffulgentsia
options_element_pre_render.patch3.4 KBjames.elliott

Comments

quicksketch’s picture

quicksketch
he/him
commented
Status: Needs review » Patch (to be ported)

Thanks, any way this could be backported to the D6 version too?

james.elliott’s picture

james.elliott commented

Shouldn't take very long. I'll roll a D6 patch as soon as I get a chance.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

What's your thinking of getting this into Drupal 7 before a Drupal 6 port is ready?

quicksketch’s picture

quicksketch
he/him
commented

I'd like to keep the two versions in sync as much as possible. Since there's no reason why this can't be backported I won't be adding it to the D7 version until we can put it into D6.

quicksketch’s picture

quicksketch
he/him
commented

When I reviewed this patch to start a backport I found that I'm not sure I'd want to include these changes at all. The theme function was specifically designed so that you could change the markup of the manual element. With this new patch you get a rather neutered theme function:

function theme_options($variables) {
  $element = $variables['element'];

  $output = '';
  $output .= '<div class="' . implode(' ', $element['#attributes']['class']) .'">';
  $output .= drupal_render_children($element);
  $output .= '</div>';
 
  return $output;
}

There's so little that's actually changeable in this theme function it's purpose is limited.

effulgentsia’s picture

effulgentsia commented
Version: 7.x-1.3 » 7.x-1.4
Status: Patch (to be ported) » Needs work
StatusFileSize
new options_element_pre_render.patch4.01 KB

Rerolled with a couple minor changes. Doesn't address #4 or #5. CNW for #5.

mstef’s picture

mstef commented
StatusFileSize
new options_element_pre_render-1009104-7.patch3.59 KB

Rerolled #6 with git - module root.

quicksketch’s picture

quicksketch
he/him
commented

While I understand the desire: "the theme function can be as simplistic as possible". I don't think this is actually a beneficial change. While preprocess functions can be helpful to simplify things, they also are immutable (other than the option to kill preprocess functions entirely). I'd prefer to close this issue and not include the changes, unless I can get a demonstrable benefit.

quicksketch’s picture

quicksketch
he/him
commented
Status: Needs work » Closed (works as designed)

Please reopen if you think differently and can help explain the benefit here.