This patch moves most of theme_webform_components_form into a preprocess function. This makes it possible to change the components form table's header and rows if necessary.

I'm sure not many people need to change that form, but I do for webform_entity.module fun. I think this is a better approach regardless to get more of the logic into a preprocess function.

Files: 

Comments

quicksketch’s picture

I usually try to avoid putting too much logic into preprocess functions, since unlike theme functions you can't easily override them. If they do something you don't like, you don't really have a choice in preventing their behavior (though you can of course discard all that work and do it over again in a different preprocess function, which is often what ends up happening). On the flip side, this particular theme function is so complicated I don't think many people override it anyway (though it's happened a few times: #1162560: Cannot redeclare _webform_components_form_rows(), #1291996: WSOD /node/%/webform/components).

As an alternative to this approach, perhaps we could make the theme function return back a renderable instead of a string? Then you could override theme_webform_components_form() with your own version, but call the original function to get back an array that you can modify.

tim.plunkett’s picture

You can't return a renderable array from a theme function. It has to be a string.

Something I've taken to doing is building out an entire render array in template_preprocess, and putting it in $variables['element'], while still storing any useful contextual information in other $variables keys

Then in the theme function:

function theme_easy_to_change($variables) {
  return drupal_render($variables['element']);
}

That way they have access to the the full render array, the context used to derive it, and they can use preprocess to add any tweaks, or less often, override the theme function to do something completely different.

While that's a pretty radical shift, I think the current code is optimized for people who want to throw away all of the functionality of the theme function, while the patch is more suited to those needing small changes or additions.

quicksketch’s picture

You can't return a renderable array from a theme function. It has to be a string.

Right, right. I was thinking of menu callbacks.

Something I've taken to doing is building out an entire render array in template_preprocess, and putting it in $variables['element'], while still storing any useful contextual information in other $variables keys

Yeah that approach sounds pretty similar to Jody's original patch. While you're right that it's better for minor changes and probably better overall, I still feel a bit dirty putting all that display logic in a preprocess function. That's Drupal for you though.

So, thinking about this actual patch, I don't think it would cause any issues for upgrades, other than the logic being done twice right?

tim.plunkett’s picture

StatusFileSize
new1.57 KB

Oh, that is kinda what she did. Go figure.

In case someone had previously implemented their own theme_webform_components_form override, clobbering $variables['form'] is probably worth avoiding, how about using $variables['element'] instead?

quicksketch’s picture

Version:7.x-3.x-dev» 7.x-4.x-dev
Status:Needs review» Fixed

Ah, what the heck. Let's just use the same variable as before. If they did some theming on this form (unlikely) it's going to cause a problem either way. Committed the original patch. However to avoid any inter-3.x issues, this is only going in the 4.x branch of the module.

quicksketch’s picture

Status:Fixed» Closed (fixed)

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

colemanw’s picture

FWIW Webform CiviCRM was overriding that theme function, but it was dirty. Looking forward to trying out this cleaner approach in the 4.x branch.
https://drupal.org/node/2044771

colemanw’s picture

I'd call the new solution almost nice and clean. The only trouble is that when adding my own preprocess function it's impossible to find the component id for each table row without resorting to hackish string manipulation of that row's contents.
Here is a fix to make people's theming experience much cleaner: #2047905: Make component id easily retrievable on components form