Remove #process pattern from taxo autocomplete widget

The last submitted patch failed testing.

subscribing

Nice usage of #element_validate! :)

Heh, bug when the ac field was left empty. Every test that uses article node type soon finds out...

Try this one.

The last submitted patch failed testing.

I'm here. I can't look at this for at least a day. I hope that's OK.

Sorry, not much time to review + test now, so just a quick one:

This class should be "taxonomy-autocomplete" instead of "text".

This review is powered by Dreditor.

bangpund: no worries. To clarify, my question is:
In the current taxonomy_autocomplete_elements_process():

what are the cases where branches 2 and 3 would be executed ?

re sun #6:
Hm, probably. Then again, I'm wondering if we *really* need the 'taxo autocomplete textfield' widget to have a 'custom width' setting (overriding node's default css, that puts a width: 95% style on all *node* forms, is the only reason for this specific class you mention - text and number widgets use the same workaround, and that class was copied from text.module)

Meanwhile, attached patch should fix the remaining failures.

Also, spin-off: #628642: Taxonomy field 'column' should be 'tid' instead of 'value'

You mean this?

Yes, it makes neither sense to limit the length of a taxonomy autocomplete textfield, nor does it make sense to specify a width (size) for it. The former is totally nuts, and the latter is a pure theming/stylesheet job - hence, it should use a unique CSS class name.

I mean more specifically

The line about '#maxlength' was wrongly copied from text.module, and is removed for good in the patch. Taxonomy fields don't even have a 'max_length' settings to begin with ;-)

This being said, custom 'width' property for text and number fields was a much welcome feature when it was added back in CCK D(5 ? 4.7 ?). So I'm not sure it's really a theme thing. Besides, if the FAPI 'textfield' #type supports the #size attribute, it precisely assumes it's not a theme thing ;-)

So, all in all, it probably makes sense to leave the 'size' setting since text and number fields have it, and they all should use the same class to override node.css style.
I'll modify this in the patch later today.

The last submitted patch failed testing.

Maybe we should really move this into a separate issue, but...

and they all should use the same class to override node.css style

Absolutely definitely not. All form items get the generic .form-item class in addition to generic .form-type-[type] and .form-item-[name] classes, and all Form API textfields automatically get the generic .form-text class.

In the #type 'text_format' issue, I was already inclined to remove the additional "text" CSS class from those widgets, because it's mainly duplication of classification that already exists.

Themers need and want unique CSS class names that have a meaning and provide classification. Applying the identical "text" class to all widgets to override a generic styling for .form-text that happens elsewhere is wrong and does not help anyone. If we want to apply custom class names, then the class names should be unique and have a meaning to provide classification.

Also, applying a class on an inner form element of a field widget is wrong, because themers want to style the entire widget, not only the form element. Looking into field_default_form() again, it looks like we do not wrap a widget with a widget-specific container. So we should open a separate issue that turns $form_element into a #type container, automatically applying proper .field-type-[type] and .field-widget-[widget] classes, which, in turn, allows us to remove those classes from the individual form elements in the widgets.

Applying the identical "text" class to all widgets to override a generic styling for .form-text that happens elsewhere is wrong and does not help anyone

No, I meant applying a class named 'custom-width' or something, with only "width: auto" property, to support the feature of 'enter the width you want for your [text|number|taxo autocomplete] widget'. This is not different than classes used to support the 'display label above or inline' feature.

This feature is specific to some widget types, therefore it has to be applied by the widget, and cannot be applied by field_default_form() to a wrapper element.

Besides, this class is only needed because node.css has some IMO dumb styling that de facto makes FAPI's #width property ineffective in node forms (width: 95% or something).

Let's move this to #629074: Field widgets are not themeable due to missing CSS class names ;)

I'll try to review + test this patch later on.

rerolled:
- removed the data massaging I wondered about in #7 - I'd still welcome bangpound's feedback before this is committed.
- removed the 'text' class as per #629074: Field widgets are not themeable due to missing CSS class names

I did some research and found out the reasoning behind the data massaging. This code was introduced in the patch for #557932: Taxonomy term field autocomplete widgets never validate, always lose values., and we had a heck of a time coping with language codes in the process and validate functions.

Wow. The autocomplete widget is no longer incomprehensible. I'm really thankful for this patch.

Since this is no longer a #process, we cannot override "any preset #element_validate handlers" ;)

Moved that into $element. No other changes, hence, RTBC.

Whether there are potentially more clean-ups possible here, we should move into a separate issue.

And, yeah, @bangpound, the same reply we see in each of these issues ;) Awesome, yched!

Good clean up except for one tiny little detail:

Small typo here: 'add create'

Agreed with sun's #18.
Fixed the typo mentioned in #19.

Committed to CVS HEAD. Thanks!

Followup for the @todo added in this patch: #631066: taxonomy_autocomplete() doesn't need to access $instance

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