Similar as #626354: Remove #process pattern from number field, but for taxonomy 'autocomplete' widget.
Seems to work OK according to my manual tests, let's see what the bot has to say.

The patch has @todo notes about a snippet I don't understand - bangpound, if you're around ?

CommentFileSizeAuthor
#20 taxo_ac_widget_simplify-628188-20.patch9.48 KByched
#18 drupal.taxonomy-widget.18.patch9.44 KBsun
#16 taxo_ac_widget_simplify.patch9.47 KByched
#8 taxo_ac_widget_simplify.patch9.62 KByched
#3 taxo_ac_widget_simplify.patch7.43 KByched
taxo_ac_widget_simplify.patch7.28 KByched

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

subscribing

Nice usage of #element_validate! :)

yched’s picture

yched commented
Status: Needs work » Needs review
StatusFileSize
new taxo_ac_widget_simplify.patch7.43 KB

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

Try this one.

Status: Needs review » Needs work

The last submitted patch failed testing.

Anonymous’s picture

Anonymous (not verified) commented

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

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

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

+++ modules/taxonomy/taxonomy.module	10 Nov 2009 02:33:50 -0000
@@ -1211,80 +1208,38 @@ function taxonomy_term_title($term) {
     '#attributes' => array('class' => array('text')),

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

This review is powered by Dreditor.

yched’s picture

yched commented

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

  // See if this element is in the database format or the transformed format,
  // and transform it if necessary.
  if (is_array($element['#value'])) {
    if (!array_key_exists($field_key, $element['#value'])) {
      // (branch 1)
    }
    else {
      // (branch 2)
    }
  }
  else {
    // (branch 3)
  }

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

yched’s picture

yched commented
Status: Needs review » Needs work
StatusFileSize
new taxo_ac_widget_simplify.patch9.62 KB

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'

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

You mean this?

  $element[$field_key] = array(
    '#type' => 'textfield',
...
    '#size' => $instance['widget']['settings']['size'],
...
  );
  $element[$field_key]['#maxlength'] = !empty($field['settings']['max_length']) ? $field['settings']['max_length'] : NULL;

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.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
yched’s picture

yched commented
Status: Needs work » Needs review

I mean more specifically 

'#size' => $instance['widget']['settings']['size'],

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

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.

yched’s picture

yched commented

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).

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue tags: +D7 API clean-up

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.

yched’s picture

yched commented
Status: Needs work » Needs review
StatusFileSize
new taxo_ac_widget_simplify.patch9.47 KB

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

Anonymous’s picture

Anonymous (not verified) commented

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.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new drupal.taxonomy-widget.18.patch9.44 KB

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!

dries’s picture

dries commented

Good clean up except for one tiny little detail:

+++ modules/taxonomy/taxonomy.module	11 Nov 2009 07:06:56 -0000
@@ -1211,148 +1208,76 @@ function taxonomy_term_title($term) {
+      // otherwise, add create a new term.

Small typo here: 'add create'

yched’s picture

yched commented
StatusFileSize
new taxo_ac_widget_simplify-628188-20.patch9.48 KB

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

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

yched’s picture

yched commented

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

Status: Fixed » Closed (fixed)
Issue tags: -D7 API clean-up

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

Issue tags: +

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.