It would be nice if we can have alter() hooks to alter the working of existing components similar to CCK field, see (#417122: Allow drupal_alter() on Field and Widget Settings).

For example, have

HOOK__webform_edit_{COMPONENT-TYPE}_alter($form, $component)
to change or add edit fields

HOOK__webform_render_{COMPONENT-TYPE}_alter($element, $component, ...)
to alter how the form element is rendered

etc...

The reason I'm asking for this is so that I can add multi-column checkboxes/radios support for the 'Select options' component with my module Multi-column checkboxes radios

See issue #603900: Direct support for Multi-column checkboxes radios, #941234: Interface with webform in a more user-friendly way

As is now, the only way is to copy the select component source code, call it something else, change it a little to add the multi-column checkboxes radios parameters. The alter hook way seems like a much nicer way to go. Copy/paste for this kind of stuff is just wrong.

Since the invocation of the component callback is encapsulated in function webform_component_invoke(), the drupal_alter() can just be add there?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mattyoung’s picture

FileSize
782 bytes

I added the alter() hook stuff and was able to enhance the 'select' component to have multi-column option to display checkboxes/radio buttons in columns by simply adding two alter hooks in my module:

HOOK__webform_edit_select_alter(&$form, $component)
HOOK__webform_render_select_alter(&$element, $component, ...)

please consider adding this functionality

thanks

quicksketch’s picture

I'm not sure adding all these hooks is a good idea. We already have hooks for component insert, update, and presave. Other operations can already be reached through other hooks such as nodeapi $op == 'load' and hook_form_alter(). Adding 10 extra hooks would likely add a lot of redundancy (not to mention potential performance impact).

Dries Arnolds’s picture

Is it really that bad? According to Matt performance impact is almost none and it opens up possibilities to alter components (instead of writing new ones that overlap largely).

Would we need 10 hooks? Are you referring to all the form field types? Maybe not all components need to be extendable, but this is a real-world example that a lot of users could profit from.

Have you read his comment here?
http://drupal.org/node/941234#comment-3607158

quicksketch’s picture

Status: Active » Needs work

Would we need 10 hooks? Are you referring to all the form field types?

This patch adds hooks to every webform_component_invoke() call, including:

hook__webform_defaults_[component]_alter()
hook__webform_edit_[component]_alter()
hook__webform_render_[component]_alter()
hook__webform_display_[component]_alter()
hook__webform_submit_[component]_alter()
hook__webform_help_[component]_alter()
hook__webform_theme_[component]_alter()
hook__webform_analysis_[component]_alter()
hook__webform_table_[component]_alter()
hook__webform_csv_headers_[component]_alter()
hook__webform_csv_data_[component]_alter()

Considering the nature of this patch it actually adds 11 hooks * 12 out-of-box components = 132 differently named new hooks. True there, are only 10 actual new places to "hook-in", but this patch at the very least is sloppy in its implementation of these hooks. Imagine if some module added a new feature to every type of component, they'd have to implement the hook once for EVERY type of component. That's not at all effective programming.

Anyway, all told I really would like to have a way to solve #603900: Direct support for Multi-column checkboxes radios. And the fact that this patch would make Multicolumncheckboxes contain the webform-integration (instead of the other way around) is great. I just need some time to mull this over before adding a significant number of new hooks. This patch itself definitely needs work, my expectation is that each hook would exist as:

hook_webform_component_[op]_alter($data, $component, ...) (essentially drop the [component] type from the hook entirely) and replace ... with the remaining arguments.

Unfortunately even this approach causes a large change in the functionality of webform_component_invoke(), since some functions discriminate between array() being returned and values like FALSE and NULL. If this functional always returned an array (which is required when using drupal_alter()), this almost certainly will have repercussions to our existing APIs.

mattyoung’s picture

How about provide alter hook calls to only the callbacks that make sense? For me, I only need 'edit' and 'render'.

>Considering the nature of this patch it actually adds 11 hooks * 12 out-of-box components = 132 differently named new hooks.

This is a good thing... just like hook_nodeapi() in D7 become 23 separate hooks. So this is the style Drupal is evolving to.

>hook_webform_component_[op]_alter($data, $component, ...) (essentially drop the [component] type from the hook entirely) and replace ... with the remaining arguments.

I would strongly advice against this. a) this is not the new Drupal style. b) more important, it's not efficient: even if you only have alter hooks for just one component, the alter hook is called for every active component. hook_webform_[op]_[component]_alter(&$data, $component, ...) is better: it's the most efficient, and encapsulates the single purpose of the hook.

If we are going to do the way you propose, we basically would have switch/case inside it. But if we use separate hook, there would be no need for that. Cleaner code and more efficient.

quicksketch’s picture

How about provide alter hook calls to only the callbacks that make sense? For me, I only need 'edit' and 'render'.

In actually, you really only need render, since you can already modify the form through hook_form_alter().

I would strongly advice against this. a) this is not the new Drupal style. b) more important, it's not efficient

Hrm. I don't know how 132 new hook names would ever be considered "clean". The new style for hooks is separated out so that $op is no longer passed in, that's true. In Webform terms this would be things like "edit", "submit", "csv_data", etc. However Drupal core doesn't provide separate hooks for every node type or field type, which would be equivalent of a webform component type. Think how silly it would be to have to implement a function for every node type you want to modify.

hook_node_story_insert()
hook_node_page_insert()
hook_node_image_insert()
etc...

Drupal core eliminate the $op parameter, not the $type parameter. I think the same thing makes sense for Webform.

mattyoung’s picture

>In actually, you really only need render, since you can already modify the form through hook_form_alter().

Don't make people go through hook_form_alter(). Why force such a blunt method when a elegant/simple alternative is so simple to implement? If it's difficult to implement, then I see your point. But it's not and if you don't allow the 'edit' alter just because hook_form_alter() can be used, then the code will look very odd. Think about how the code will look:

function hook_form_webform_component_edit_form_alter(&$form, ... ) {
  if (SELECT_COMPONENT) {
     blah;
     blah;
  }
  elseif (TEXTFIELD_COMPONENT) {
    blah;
    blah;
  }
}

vs.

function hook_webform_edit_select_alter(&$form, ...) {
   blah;
   blah;
}

function hook_webform_edit_textfield_alter(&$form, ...) {
   blah;
   blah;
}

which one is easier to read and understand? Also hook_webform_edit_select_alter(&$form, ...) is called only when that specific component is being edited, whereas hook_form_webform_component_edit_form_alter() is called for every component and then you will have to figure out what to do inside it.

>I don't know how 132 new hook names would ever be considered "clean".

Don't think of it as 132 hooks but a naming convention. You will never do the same thing in 'edit' or 'render' alter for two difference components, so why lump together? Again, that will only make code less understandable and less efficient.

Let's compare what the code looks like:

your way:

function hook_webform_edit_alter(&$form, ...) {
  if (SELECT_COMPONENT) {
    blah;
    blah;
  }
  elesif (TEXTFIELD_COMPONENT) {
    blah;
    blah;
  }
}

function hook_webform_render_alter(&$element, ...) {
  if (SELECT_COMPONENT) {
    blah;
    blah;
  }
  elesif (TEXTFIELD_COMPONENT) {
    blah;
    blah;
  }
}

my way:

function hook_webform_edit_select_alter(&$form, ...) {
  blah;
  blah;
}

function hook_webform_edit_textfield_alter(&$form, ...) {
  blah;
  blah;
}

function hook_webform_render_select_alter(&$element, ...) {
  blah;
  blah;
}

function hook_webform_render_textfield_alter(&$element, ...) {
  blah;
  blah;
}

To me, my way is much easier to read and understand: every hook function does one specific thing; the hooks are only fired when they are actually used. Your way, the hooks are fired for every component.

quicksketch’s picture

I want to add an option to EVERY component type, something like "Label position" or "Show description" as popup. How are you going to do that when you have to implement a separate hook for every component type?

Your approach is not typical Drupal convention, though you are arguing it is. I'm not going to add the hooks as you've described, however I'm open to adding well thought out individual hooks as necessary. I don't want to firehose hooks blindly, changing our expected return values and breaking our APIs at the same time.

quicksketch’s picture

Similar examples of hooks that do not exist:

hook_node_[node-type]_[op]
hook_block_[module]_[delta]_[op]

In these situations (and many others), you always have to do switches or if statements based on $node->type or $block->module and $block->delta.

Though I have to concede that in D7, an individual hook does exist for form and base forms:

hook_form_alter()
hook_form_[base-form-id]_alter()
hook_form_[form-id]_alter()

Which would suggest that a valid solution is adding 11 generic hooks and 11 specific ones based on naming convention (11 + 132). Though this doesn't help my general anxiety around adding a huge number of hooks.

Dries Arnolds’s picture

And what is your stance on adding support for multi-columnn checkboxes/radios directly in webform? That is what this discussion actually started.

imclean’s picture

I'm also interested in modifying and extending existing components. The "Options Elements" module looked like a good place to start but it seems support for that specific module has been built in to webform.

quicksketch, I've looked at hook_form_alter() but I'm not sure where to start. For example, how would I use this to add config settings to Webform's select component? Although I suspect this request isn't quite what the topic is about. Is this even possible?

#10 Pixelstyle, this issue is still going: #603900: Direct support for Multi-column checkboxes radios

timlie’s picture

subscribe

robertom’s picture

Version: 6.x-3.x-dev » 7.x-3.x-dev
Status: Needs work » Needs review
FileSize
2.07 KB

I try to find a path for implement #536236: Custom CSS classes for form components functionality with external module.

with this patch I've added 2 hooks:

hook_webform_render_component_alter()
hook_webform_theme_component_alter()

This is acceptable?

robertom’s picture

re-rolled on current git version (4cca9de)

quicksketch’s picture

What's the necessity of the "theme" alter? Can't hook_theme_alter() be used already with the exact same result?

Bensbury’s picture

Hi,

We've been following this thread to see if this functionality is going into webforms.
We are using the patch to allow the multi-column checkboxes to work for a client who requires the ability to create their own webforms which they direct their clients to.

To allow this we have had to patch webforms but are concerned about this when it comes to updates and security updates.

So is this patch likely to go into webforms or be rejected as there has been a webforms update recently?

Thanks.

Dries Arnolds’s picture

I'm still curious about my comment in #10. If all the hooks are too much, can multi-column checkboxes and radios be supported directly as an internal component? It is a valid use case for all longer lists of choices in forms. Something I personally come across quite often.

quicksketch’s picture

If all the hooks are too much, can multi-column checkboxes and radios be supported directly as an internal component?

Heya Pixelstyle. I looked into doing this once but the combination of needing to support both normal radios/checkboxes in addition to "Select or Other..." radios/checkboxes seems a bit daunting (and prone to break).

After re-reading this issue what I'd like to introduce as a toe-in-the-water exercise of including three hooks (the ones you just so happen to need for multi-column radios):

hook_webform_component_defaults_alter(&$component)
hook_webform_component_render_alter(&$element, $component, $value, $filter)
hook_webform_component_edit_alter(&$form, $component)

These hooks would also be enough for other popular modules like Webform Validation which currently loop recursively through the form to add the necessary validation to elements. mattyoung also said as much in #5:

How about provide alter hook calls to only the callbacks that make sense? For me, I only need 'edit' and 'render'.

But oddly none of the patches provided have done exactly this. I've argued that really "render" is the only one necessary, since you can just hook_form_alter() the component form. But providing it as a convenience would be acceptable for me. The addition to the "defaults" isn't truly necessary either, but it can save a lot of hassle when trying to be E_ALL compliant by fixing a lot of situations that would cause notices.

The sticking points in this issue have been my complaints about implementation, not that these hooks shouldn't be added. The problems: 1) Adding hooks for more operations than is needed for this problem and 2) Providing per-component type hooks instead of a single hook for each operation for all components.

mattyoung’s picture

Version: 7.x-3.x-dev » 6.x-3.x-dev
FileSize
663 bytes

patch to add

hook_webform_component_defaults_alter(&$component)
hook_webform_component_render_alter(&$element, $component, $value, $filter)
hook_webform_component_edit_alter(&$form, $component)

However, I think these hooks should be component type specific instead of generic. So that if I want to alter the select component, I do:

hook_webform_defaults_select_alter(&$component) { }
hook_webform_render_select_alter(&$element, $component, $value, $filter) { }
hook_webform_edit_select_alter(&$form, $component) { }

Not

hook_webform_component_defaults_alter(&$component) { if ($component['type'] == 'select' { ... } }
hook_webform_component_render_alter(&$element, $component, $value, $filter) { if ($component['type'] == 'select' { ... } }
hook_webform_component_edit_alter(&$form, $component) { if ($component['type'] == 'select' { ... } }
quicksketch’s picture

Thanks mattyoung. I still have to disagree that individual hooks by component type is the way to go, any more than hooks by node type is used in Drupal. Just like you'd regularly only affect one node type, the ability to affect all nodes equality is still tremendously helpful, the same goes for modification of components in Webform.

The patch looks good (almost too trivially simple ;), but we also need docs in webform_hooks.php to document these new hooks.

quicksketch’s picture

Status: Needs review » Fixed

#245424: Make Webform multilingual (i18n) aware through contributed modules added hooks for render and display altering, making it so that these patches are no longer needed. The new hooks are included and documented in the 3.16 release.

Note that it still does not include a defaults_alter(), so we may look into that individual hook as a separate issue.

Status: Fixed » Closed (fixed)

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