Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pcambra’s picture

Title: Implement the taxonomy_autocomplete widget as a Plugin » Convert taxonomy module widgets to Plugin system

One widget here: 'taxonomy_autocomplete'

swentel’s picture

Status: Postponed » Active

Widget plugins patch is in, we can convert. The way that we have been working for now is creating a separate branch in the sandbox, but I'm not sure if that's the 'right' way now. Attaching patches is fine as well I guess, once they're ok, we can move this to the core queue.

pcambra’s picture

Assigned: Unassigned » pcambra

I'm giving this one a try tonight

pcambra’s picture

Status: Active » Needs review
FileSize
4.39 KB

Here's the conversion, some comments

I haven't converted the info alter as the option widgets haven't been converted yet #1751234: Convert option widgets to Plugin system

/**
 * Implements hook_field_widget_info_alter().
 */
function taxonomy_field_widget_info_alter(&$info) {
  $info['options_select']['field types'][] = 'taxonomy_term_reference';
  $info['options_buttons']['field types'][] = 'taxonomy_term_reference';
}

Also I've converted

      'behaviors' => array(
        'multiple values' => FIELD_BEHAVIOR_CUSTOM,
      ),

To multiple_values = TRUE as the testfields do.

yched’s picture

Status: Needs review » Needs work

Right, taxonomy_field_widget_info_alter() should stay untouched for now.

All references to $instance['widget']['settings'][$some_setting] should move to $this->getSetting($some_setting);
Which means we don't need

+    $instance = $this->instance;

in formElement().

Also, errorElement() just replicates WidgetBase::errorElement(), meaning it could be discarded.

pcambra’s picture

Status: Needs work » Needs review
FileSize
4.11 KB

Right, here's a new patch with the changes applied.

yched’s picture

Seems to work fine !

As an additional nicety, taxonomy_autocomplete_validate() could be replaced with the new Widgetinterface::massageFormValues() method.

pcambra’s picture

Status: Needs review » Needs work

Let's see how that works :)

yched’s picture

Oops, sorry - completely removing the #element_validate won't work, because WidgetBase::submit() expects to receive form values as an array, and transforming the comma-separated string into an array happens in taxonomy_autocomplete_validate().

Then I guess taxonomy_autocomplete_validate() could be simplified to just explode the string to an array, and the rest of the logic could go in massageFormValues().

Getting rid of taxonomy_autocomplete_validate() completely would probably require some upstream work on how WidgetBase deals with 'multiple' widgets, that would be a followup.

pcambra’s picture

Project: D8 Field API » Drupal core
Version: » 8.x-dev
Component: Code » field system
Status: Needs work » Needs review
FileSize
6.99 KB

Applying the change suggested in #9 and moving to core to see what the TestBot thinks :)

yched’s picture

Status: Needs review » Reviewed & tested by the community

Yup, this looks great. Thanks !

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome. Confirmed this is basically just moving code around. This is actually a great patch for demonstrating how to convert to the new widget system, and it's nice to see how this more elegantly groups like code together. Well done!

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

nils.destoop’s picture

Status: Closed (fixed) » Needs review
FileSize
673 bytes

There was 1 thing that was not converted yet. The field types key from hook_field_widget_info_alter() should now be field_types. Don't know if we need a test for it? Test would be almost the same like the test for #1817180: Tests: hook_widget_info_alter() is not called anymore.

webflo’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

catch’s picture

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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