Move number formatters to the new API

Comments

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
Assigned: Unassigned » pcambra
pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
Status: Active » Needs review

Here's the commit in my branch: 89abbac

I was doubting when transforming this if we should implement just one Default formatter for integer and decimal as the code is almost equivalent, but that would mean more changes.

yched’s picture

yched commented
Status: Needs review » Needs work

Thanks @pcambra !

There does need to be different classes for integer and decimal 'default' formatters, because they have different default settings, but given the code in the methods, it seems they would really benefit from sharing a common base class.

- At least for the code about prefixes/suffixes in viewElements() should be shared in a method in the parent
(also, beware, in your code this code is wrapped in an if ($this->getSetting('thousand_separator')) { that should be about setting 'prefix_suffix' instead)

- settingsForm() could probably be in the parent as well :
something like :

public function settingsForm(array $form, array &$form_state) {
  $settings = $this->getDefaultSettings();
  $settings_names = drupal_map_assoc(array_keys($settings));

  if (isset($settings_names['decimal_separator']) {
    $element['thousand_separator'] = ...
  }

  if (isset($settings_names['scale']) {
    $element['scale'] = ...
  }
}

(IMO we do want to get rid of the "dummy" settings on number_integer, that are only used for their default values : scale, decimal_separator)

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
Status: Needs work » Needs review

Here's a new version: commit
I had some stupid merge problems, but here's a good interdiff.

I couldn't implement the common settings form as getDefaultSettings is returning all of them for both of the formatters.

Back to CNR

yched’s picture

yched commented
Status: Needs review » Fixed

Adjusted the code organisation and bit, and merged into field-plugins-formatters-1785748.
Thanks !

Status: Fixed » Closed (fixed)

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