theme_webform_element needs a preprocess function

Like it'spossible with all core form elements.

How is it possible with core form elements that is different from Webform? theme_webform_element() is largely based on theme_form_element() and is in most cases more flexible, rather than less.

And I can't see why this wouldn't work:

function mymodule_preprocess_webform_element(&$variables) {
  $variables['element']['#attributes']['class'][] = 'another-class';
}

And in the 4.x branch we've "back-ported" support for #wrapper_attributes like Drupal 8, so you can add a class on the wrapper too:

function mymodule_preprocess_webform_element(&$variables) {
  $variables['element']['#wrapper_attributes']['class'][] = 'another-wrapper-class';
}

However since we don't like to change theme_* functions within a branch, so that functionality doesn't exist within 3.x. So overall I'm puzzled by this request, but in any case this would be a feature rather than a bug, and certainly not major.

Webform does not use mymodule_preprocess_element().

It doesn't use the core preprocessing function because the theme function itself is different. Webform doesn't want to inherit the preprocessed variables of a theme function for which it wasn't intended.

I cannot add for every module extra form theming just tor the reason that existing apis are not reused.

Webform is a special case that deals extensively with forms, much more than your typical module. Webform needs to provide it's own theme function to support functionality core does not inherently provide, such as inline labels. Broadly re-applying the same preprocessing that isn't made for Webform's theme functions is an approach that could result in unintentional side-effects.

Rephrasing title. There's no limitation on adding classes to Webform elements via preprocessing right now. This request is asking that Webform automatically reuse the same preprocessing that is done for core and apply it to its own elements.

I marked your other issue #2025583: All form elements need to use core form fields theming duplicate, as it's essentially the same issue rephrased differently.

Webform needs to provide it's own theming for form elements because it provides functionality not provided by core. Things like our conditional logic also depend on the webform-specific styling. New features in the 4.x branch like being able to set wrapper classes from the UI depends on theme_webform_element() and cannot be done with core's theming. Drupal 8.x includes some enhancements to theme_form_element() (#wrapper_attributes was added in #1838114: Change node form’s vertical tabs into details elements), but even then in 8.x we have differences that Webform requires.

Out of the box it works fine. You have either installed or written a module which breaks its functionality. It looks like either the CSS Webform provides is not being loaded on the page, or the class "webform-container-inline" is not being added to the elements, resulting in inline elements being displayed on independent lines.

In that case, some CSS on your site might be overriding the CSS provided by Webform. Here's what it typically looks like (this is with the Bartik theme):

I understand the need for a separate webform_element theme function as there is (can be) too much going on that core elements just doesn't do/provide.

However, I would surmise that the real issue is that there is too much "preprocessing" going directly into theme_webform_element instead of an actual template_preprocess_webform_element function.

While a [base/sub-]theme can add classes, it cannot however replace them. I'll submit a patch shortly.

This looks good to me, thanks Mark! How does this affect sites that have already overridden theme_webform_element()? Will the now-duplicated code (between preprocess and the overridden version) cause any problems?

I suspect it will duplicate the classes added to the output. This shouldn't be an issue though as most browsers will simply ignore this. It really only becomes an issue as valid markup (ie: shouldn't duplicate identical classes). It is an easy fix though and should be easily remedied with a change notice (which I'd be happy to create).

I would still like to classify this as a bug though because the current implementation does not allow themes to remove classes (which is major IMO, but I'll leave that one alone).

I have created a draft change notice here: https://www.drupal.org/node/2302983

Sorry, but removing classes is necessary in some cases and it cannot be done without this patch.

Setting meta data according to the Issue queue handbook.

If you read it, but whatever... how about we just fix this instead of playin issue status wars please....

The fact of the matter is that this theme hook is not using the theme system API appropriately (bug) and bypassing preprocessing altogether (major). This is apparent by the fact that a theme cannot remove the classes that are injected via the theme function, which is the last step before it's output is rendered (ie: pretty major bug).

I want quicksketch to weigh in. Given that we are already in RC for 7.x-4.x, I'm not sure if this should be 7.x-5.x as it is an API change. I'm concerned about introducing a change like this between two RC releases. It would be great to have someone else review this patch, too.

As for meta data "wars", please just follow the guidelines. Undoing the meta data corrections of the primary maintainer isn't helpful as we struggle to cope with 500+ open issues.

I understand how RC's work. There hasn't been an official release. While an RC is indeed usually deemed relatively stable, it isn't considered set in stone... especially when it's fixing fundamentally flawed code like this. If a stable release has this kind of bug... man, what a shame that it got hung up on something as trivial as squabbling over semantics.

Undoing the meta data corrections of the primary maintainer isn't helpful as we struggle to cope with 500+ open issues.

This alone suggests that this project is, in fact, not using the issue queues correctly then. Using different categories and priorities just to help not show them as "open bugs" is wrong. A bug, is a bug, is a bug. Saying otherwise causes confusion (like this) between projects, especially when cross-projects utilize the same type of core APIs/[theme] hooks.

The fact that this project doesn't actually implement this particular theme hook properly is what deems this particular issue major. It prevents other contrib projects (ie: Bootstrap, which I maintain and provide integration support for this project) cannot properly interface with this project.

Please do not lecture me about how issue queues work... I am very well versed in cross-project issue queues, especially ones involving the theme system (core, themes, js, etc).

I am not trying to be hostile, attack anyone or bully this issue. I came over here (from Bootstrap) to help fix a pretty serious flaw because I have been getting issues in Bootstrap about this. I provided a patch and there hasn't been any progress in a couple weeks. I re-evaluated everything and realized that it was actually a major issue because I cannot do anything on my end until this is fixed.

The fact that I have to say all this is what is frustrating. Please just fix this instead of provoking unnecessary rebuttals through trivial insults.

@Mark Carver -- I am very sorry you took offense when none was intended. I do understand that you really want this patch committed. I do understand that you consider this a bug. I do understand that your work can't continue until this is committed. And I infer that you are frustrated. It was quicksketch's opinion that the existing implementation wasn't broken (not a bug) and not meeting the criteria for major , and I concurred. I even re-read the criteria before restoring quicksketch's metadata.

We all agree that your change is better and more flexible. Let's just kiss and make up, setting the meta-data aside as it is not productive at this point. We don't have to agree. We just have to feel that the other person wasn't acting maliciously, which I'm sure was true in all cases.

I think there are only two issues:
- Review of the patch.
- What branch this can be committed to.

I will try to find some time to review the patch. I'd like some guidance from quicksketch re the branch. I personally would be upset if my working site broke because of a theme implementation change at rc4 to rc5. Reading the criteria for beta, the recommendation is to change an API at the beta stage only if the bug is critical.

We just have to feel that the other person wasn't acting maliciously, which I'm sure was true in all cases.

Yes, neither of us were acting maliciously. I understand that. That wasn't the part I was upset about. Inferring that I am somehow unaware of or not following the Issue queue handbook is what was. Comments like this is what I find offensive, counter productive and completely unnecessary between established community members. It takes away from the issue at hand: fixing the broken code.

I'd like some guidance from quicksketch re the branch.

Yes, Nate should make the call here.

I personally would be upset if my working site broke because of a theme implementation change at rc4 to rc5.

This is why there are release notes. There are some things, like this, that change between RCs. This is why I drafted a change notice to go along with whatever release notes are to be made: https://www.drupal.org/node/2302983

Reading the criteria for beta, the recommendation is to change an API at the beta stage only if the bug is critical.

Webform does not provide the theme system/hook API, core does. Webform is not changing anything itself has created (new APIs specific to webform only), thus this issue is not subject to this type of release gate.

If I could interject as Webform's biggest fan (seriously)...

I am not qualified to speak to the larger implications such as whether this should be added to the current RC or a new version. Nor can I say if something was done right or wrong in the first place. BUT theming Webform in a Bootstrap subtheme is currently absurd. This might be the single most frustrating thing I've done in 7 years of Drupal (even when I started out as a never-wrote-code-before noob).

Of course it's upsetting if an update breaks your webform or theme, but that's a risk you sign up for when using a beta or RC release. Could we not add a check whether the site's default theme has implemented the theme_webform_element()? (Honest question!)

While I do agree that this probably isn't "Major" since you can technically run webform in a Bootstrap theme, the idea of pushing it out to 7.x-5.x is disheartening given the lack of support for 7.x-4.x in Webform-related modules.

Just a perspective... I'm going apply the patch to my live site now. :)

Hi guys, thanks for all the input and my apologies I didn't see this happening. Mark is a super-active in Twig in D8 and Dan is the most active maintainer of Webform module; it sucks to see online communications break down.

My primary concern here is what happens to sites that have already overridden theme_webform_form_element(), since I don't want to break sites that have already overridden this function. I test this out to be absolutely sure of the effect, and indeed the result it causes is double-output of classes:

Wrapper with current code, overriding theme_webform_element():

<div class="form-item webform-component webform-component-email webform-component--new-1398236195818">

Wrapper after applying patch:

<div class="form-item webform-component webform-component-email webform-component--new-element form-item webform-component webform-component-email webform-component--new-element">

But other than that, I don't think this will cause harm to existing sites who have overridden theme_webform_element(). It could do the frustrating thing of *re-adding* classes that were explicitly removed (say if a theme like Mothership already did the work of removing them). But even then I think the overall impact would be minimal.

I don't like changes like this in RCs or minor point releases, and I don't really think this is a bug (as we're doing pretty much the exact same thing as theme_form_element() in core, where this limitation also exists), but I think this new approach is better and may save us if we need to make changes to the output in the future. So I say let's go for it and put a notice in the release notes, and prepare for a few (rightfully) grumpy themers storming the issue queue.

Since I just tested it, I suppose I should mark this RTBC. I know there are reservations about this patch (I have my own as well here), but I'm for putting it in.

Committed this to 7.x-4.x. Thanks all!

I added change notes to the upgrade documentation at https://www.drupal.org/node/1609324#form-element-preprocess.

This needs porting to 8.x-4x.

Done.