DecimalItem should not use String as its DataDefinition

+1 for that, it caused some confusion for me why should I use string data type for a decimal field property..

Also I think worth adding to the DecimalData class comment that float should not be used due to rounding issues.

also +1 from me, the patch looks ok. Voting to push this further.

Ok, here goes then, let's see if testbot is ok with this after so many years.

nitpicks in #22 are still relevant.

For #36.

Let's land this one at last! 🤓 🤝

Aaargh, forgot to add before diff --cached ;)

Here goes.

The comment can be reflowed closer to 80 chars, I know that this is an ubernitpick, but that is the only change needed still, after that I can rtbc this change.

Not sure what you mean, thought we only have a line length limit of 80 according to standards and here the info is split to lines and the first line has a empty line following so my phpcs doesn't complain. Can you please propose a specific change?

Also see ItemList.php in the same folder. What's the difference?

+++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/DecimalData.php
@@ -0,0 +1,27 @@
+ * Because PHP does not have a primitive type decimal and using
+ * float can result in unexpected rounding behavior, it is

* Because PHP does not have a primitive type decimal and using float can
* result in unexpected rounding behavior, it is implemented and displayed 
* as string.

This is also still < 80 chars and makes it a little bit easier to read. Like I said, super tiny change.

Aaah, gotya :) Thanks!

Fingers crossed ;)

I'll stop the test for this one.

Looks great!

The last submitted patch, 44: core-2888763-44.patch, failed testing. View results →

Re-ran and passed. It was quite obvious the fail was not related. Moving back to RTBC.

The last submitted patch, 44: core-2888763-44.patch, failed testing. View results →

The last submitted patch, 44: core-2888763-44.patch, failed testing. View results →

random fail

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/DecimalItem.php
@@ -35,7 +35,7 @@ public static function defaultStorageSettings() {
+    $properties['value'] = DataDefinition::create('decimal')

At the moment most of the decimal field logic is on the UI layer, i.e. \Drupal\Core\Field\Plugin\Field\FieldWidget\NumberWidget. It looks like this issue wants to change this so that \Drupal\Core\Field\Plugin\Field\FieldType\DecimalItem is storing the data using the decimal data type, allowing us to remove some of the handling from the UI layer.

Unfortunately I don't think this is working as it should at the moment because this configures the column with the Mysql default precision and scale. This could lead to really confusing behavior because this would completely bypass the precision and scale configured in field config.

If we are going to move to using the decimal data type in Mysql, the configuration should be moved from field config to field storage. We also need to asses what changes are needed in \Drupal\Core\Field\Plugin\Field\FieldWidget\NumberWidget. I think we need to write additional test coverage too because it doesn't look like the current test coverage was able to catch this.

Removing the subsystem maintainer review tag as well because I don't think it's needed anymore.

@lauriii what do you think this test should exactly do? The only thing I see different from the previous logic is that zero will not evaluate to NULL as in case of int currently. Is that it or you have any other ideas so if someone starts working on it, they'll not land at nw again?

We could use the tests for integer as starting point for decimals and adjust the assertions to what we expect them to be with the new decimal type. This would already cover the NULL use case and the other basic use cases 👍

Ok, I admit this had missing pieces.

Hiding the last patch as outdated.

So now we have double validation and we can remove the field level one. Just one question: should we leave the type one with is_numeric or go for the regex instead? I’d go for is_numeric.

I agree with #60, keeping this is the is_numeric is perfectly fine and probably somewhat faster than the regex. Back to rtbc now that we have test coverage.

Posted few comments in the MR

All threads resolved, back to review.

Both of the remarks by @lauriii have been resolved.

lauriii → closed merge request !4047

Committed da8c8f9 and pushed to 11.x. Thanks!

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