Convert field type to typed data plugin for number module

Taking this one...

@plopesc. mind you that swentel already did the job in a sandbox for a couple field types, including number.
For those field types, it's just about extracting his code and posting a patch.

Hello

@yched: I know, but based on text module implementation I've made some minor changes in sandbox code.

Attaching first approach patch.
It works fine and number field tests passes but, preSave() function does not work at the moment, given that it depends on #1868004: Improve the TypedData API usage of EntityNG.

Regards

Changing status

Thanks (again) @plopesc !

+++ b/core/modules/number/lib/Drupal/number/Plugin/field/field_type/DecimalItem.phpundefined
@@ -0,0 +1,102 @@
+      '#options' => drupal_map_assoc(range(10, 32)),

While we're here, let's use \Drupal\Component\Utility\MapArray::copyValuesToKeys() instead of drupal_map_assoc() (see the @deprecated doc in the phpdoc for drupal_map_assoc())

Same a couple lines below.

+++ b/core/modules/number/lib/Drupal/number/Plugin/field/field_type/DecimalItem.phpundefined
@@ -0,0 +1,102 @@
+      '#disabled' => $field->hasData(),
 ...
+      '#disabled' => $field->hasData(),
+    );

Let's extract $field->hasData() in a variable to avoid computing it several times.

+++ b/core/modules/number/lib/Drupal/number/Plugin/field/field_type/DecimalItem.phpundefined
@@ -0,0 +1,102 @@
+  public function preSave() {
+    $this->getInstance()->getField()->setValue(round($this->getInstance()->getField()->getValue(), $this->getInstance()->getField()->settings['scale']));
+  }

Too long, this should be split in several lines.

+ this looks seriously wrong :-)
Should be something like
$this->value = round($this->value, $field->settings['scale'])
Don't forget that in a FieldItem class, $this is the value object itself. No need to go through getInstance() / getField() to access it.

+++ b/core/modules/number/lib/Drupal/number/Plugin/field/field_type/FloatItem.phpundefined
@@ -0,0 +1,62 @@
+
+    if (!isset(static::$propertyDefinitions)) {

Nitpick: I know this is just moving some existing code around, but let's remove the empty line at the beginning of the function. (also applies to the other classes)

+++ b/core/modules/number/lib/Drupal/number/Plugin/field/field_type/NumberItemBase.phpundefined
@@ -0,0 +1,109 @@
+use Drupal\Core\Entity\Field\PrepareCacheInterface;

Not needed, should be removed.

+++ b/core/modules/number/lib/Drupal/number/Plugin/field/field_type/NumberItemBase.phpundefined
@@ -0,0 +1,109 @@
+    if (!empty($this->getInstance()->getField()->settings['min'])) {
+      $min = $this->getInstance()->getField()->settings['min'];
+      $constraints[] = $constraint_manager->create('ComplexData', array(

This looks wrong, 'min' and 'max' are instance-level settings, so this should be $this->getInstance()->settings['min'];
(same applies for max)

Also, better extract $this->getInstance()->settings to its own variable at the beginning of the function, like you have done in other places.

+++ b/core/modules/number/lib/Drupal/number/Plugin/field/field_type/NumberItemBase.phpundefined
@@ -0,0 +1,109 @@
+            'minMessage' => t('%name: the value may be no greater than %max.', array('%name' => $this->getInstance()->label, '%max' => $max)),

copy/paste - should be 'maxMessage' :-)

Seeing number.module empty aside from hook_help() : +++++++ :-)

Hi!

Attaching patch following your advices.
Thanks for your feedback!

Regards.

Length / Range : right, missed that.

+    $this->setValue(
+      round($this->get('value')->getValue(),
+      $this->getInstance()->getField()->settings['scale'])
+    );

I really vote for the short syntax :-)
$this->value = round($this->value, $this->getInstance()->getField()->settings['scale'])

No problem, using the short way ;)

Given that this is the preferred approach, I changed it in isEmpty() method too.

Regards.

Thanks!
RTBC if green.

Er - sorry, I should have realized that earlier, my bad.

As I just wrote in #2015693: Convert field type to typed data plugin for link module :
"The goal is to make our FieldItem classes (like LinkItem here) be usable for configurable fields (that can be added through the UI) as well as base entity fields (that are hardcoded in the Entity definition).
This means relying as little as possible on the Field / FieldInstance definition structures that are specific to configurable fields, and instead use the FieldDefinitionInterface."

This means that everywhere we access settings (hm, except probably the schema() method for now, this one is pretty tied to configurable fields), this should be done through FieldDefinitionInterface::getFieldSettings() / getFieldSetting() :

$setting = $this->getFieldDefinition()->getFieldSetting('foo_bar');
or, if you're going to use several settings :
$settings = $this->getFieldDefinition()->getFieldSettings();
And just don't bother whether the setting is field-level or instance-level (this will get both indistinctly)

For the field label : $this->getFieldDefinition()->getFieldLabel();

Hello

I did it without any problem for field settings or field label. However, the hasData() property necessary to enable/disable the settings form elements. I think it's not accessible through getFieldDefinition.

I don't know if we could add it to FieldDefinitionInterface, or if you have planned that this property will be accessed in other way in the future.

Regards.

Yeah, hasData() is not in the interface. Same as for schema(), we need to polish that a bit.
In practice, it's not too important since hasData() is typically called in settingsForm(). When this method is called, this necessarily implies a configurable field.

I guess the best solution would be to let the caller pass the result of hasData() as a param to settingsForm(). That's how it worked in D7.

I'll open an issue for that. This patch is good to go.

Thanks !

Opened #2051157: pass $has_data as a param to ConfigFieldItem::settingsForm()

Committed e30e80c and pushed to 8.x. Thanks!

The patch needed a small update after #2041423: Rely on 'provider' instead of 'module' for Field plugin types, here it is :)

Yup.

I should have caught that :(

Committed 3454b34 and pushed to 8.x. Thanks!