Decimal field is being serialized as "null" instead of "0.0" when the value is empty. However, in other languages, such as Java or SQL, "null" is not allowed for decimal primitive.

This is because DecimalItem is being defined as "string" in core/lib/Drupal/Core/Field/Plugin/Field/FieldType/DecimalItem.php

 /**
   * {@inheritdoc}
   */
  public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
    $properties['value'] = DataDefinition::create('string')
      ->setLabel(t('Decimal value'))
      ->setRequired(TRUE);

    return $properties;
  }

Proposed solution:

change the "string" definition to "decimal"

Issue fork drupal-2888763

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

skyredwang created an issue. See original summary.

skyredwang’s picture

Status: Needs review » Needs work

The last submitted patch, 2: decimalitem_should_not-2888763-2.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 4: decimalitem_should_not-2888763-4.patch, failed testing. View results

Wim Leers’s picture

Wow, nice find!

Can you please do some issue queue/commit archeology to figure out why this is the case? It'd be great to have that in the issue summary.

skyredwang’s picture

I have already tried to search (used key word "DecimalItem") for reasons in the issue queue. I also asked @damiankloip on IRC. And, I traced the git commit back to

commit aed35d3a1fff040486646abc389051e55d12df00
Author: webchick <webchick@24967.no-reply.drupal.org>
Date:   Sun Mar 16 12:10:42 2014 -0700

It appears that this definition was made on the very first commit on this file. No more clues.

Wim Leers’s picture

skyredwang’s picture

No discussion in that issue, but

core/lib/Drupal/Core/Field/Plugin/Field/FieldType/DecimalItem.php was renamed from core/modules/number/lib/Drupal/number/Plugin/Field/FieldType/DecimalItem.php

Tracking the git commits on the older file path, which led to the very first commit to

commit 3ded2216b02adea6bfd50dd994fdbe1c12706cd9
Author: Alex Pott <alex.a.pott@googlemail.com>
Date:   Sat Oct 19 09:12:13 2013 +0100

That issue was #2115145: Move field type plugins to Plugin/Field/FieldType

skyredwang’s picture

core/modules/number/lib/Drupal/number/Plugin/Field/FieldType/DecimalItem.php was renamed from
core/modules/number/lib/Drupal/number/Plugin/field/field_type/DecimalItem.php, then the very first commit to that file was:

commit e30e80c560d1609e1b7ee386f86d424eb4970b80
Author: Alex Pott <alex.a.pott@googlemail.com>
Date:   Mon Jul 29 10:12:22 2013 +0200

The issue was #2015691: Convert field type to typed data plugin for number module

skyredwang’s picture

OK. I believe that the very first traceable reference that DecimalItem was defined as "string" is https://www.drupal.org/node/2015691#comment-7581845, however, there is still no discussion about why "string" is decided.

Wim Leers’s picture

So then it sounds like is indeed simply an oversight. Let's fix it.

skyredwang’s picture

I can't find a reference to the PHP data types, except this one https://www.w3schools.com/php/php_datatypes.asp. It seems that PHP doesn't support decimal as a primitive. I guess the implementation idea was that this field type was stored as "decimal" in MySQL, but displayed as a string.

Above patch is casting "decimal" to "float"/"double", this is tricky.

Another approach would be create a DecimalData, which extends StringData, and let getCastedValue() to "0.0" if the string is empty.

dawehner’s picture

Yeah treating it as float seems to be semantically and technically wrong. I think the suggestion made in #13 seems like a reasonable approach, at least for me.

tstoeckler’s picture

I think this patch makes sense. I was wondering what about NULL values, because in theory there is a distinction between an empty-ish value (in this case 0.0) and no value at all (which could be signified by NULL), but this is consistent with what we do otherwise, i.e. in IntegerData we just cast to (int), so any NULL, '', etc. that reaches this point will be returned as 0. So I guess typed data does not have the notion of empty values at this level, which kind of makes sense, I guess, because the storage does not come into play here yet, but only at the field (property) level.

Some notes on the actual patch:

  1. +++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/DecimalData.php
    @@ -0,0 +1,25 @@
    + * Decimal type is stored as "decimal" in the relational database. But because PHP does not have a primitive type decimal,
    + * it is implemented and displayed as string.
    

    This should break at 80 characters.

  2. +++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/DecimalData.php
    @@ -0,0 +1,25 @@
    +    return empty($this->getString()) ? '0.0' : $this->getString();
    

    This can be

    return $this->getString ?:
     '0.0';

Also this will need some tests. Not actually sure what kind of tests we want, but I know we want some kind of test ;-)

skyredwang’s picture

skyredwang’s picture

We might want a test in serialization that a decimal field with empty value will be serialized to '0.0'

e0ipso’s picture

I'm torn by this and I can't decide. On one hand I get that NULL is not a decimal. On the other hand empty is not 0.0, since 0.0 is an actually valid value.

I feel I'm having some cognitive bias in the form of "empty" is falsy, 0.0 is falsy. Therefore 0.0 is a good representation of "empty".

If forced to express in one way or the other I'd say NULL is a better "void" than 0.0. Application code should check the serialized value if NULL is not acceptable for the use case. However I'm not 100% happy about that either.

tstoeckler’s picture

Re #19: I had the same reservation in #16, but as far as I know Typed Data itself doesn't have the notion of falsey values. At the very least this makes decimals consistent with e.g. integers, that always return 0 for a falsey value.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

borisson_’s picture

Status: Needs review » Needs work

I share the reservations of #16 and #19 but I also agree with #20, consistency is good.

Can we write specific tests for this new data type or can we trust the implicit test for the other things we already have in place.

  1. +++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/DecimalData.php
    @@ -0,0 +1,26 @@
    + * Decimal type is stored as "decimal" in the relational database. ¶
    

    There's a stray space here.

  2. +++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/DecimalData.php
    @@ -0,0 +1,26 @@
    + * But because PHP does not have a primitive type decimal, it is
    + * implemented and displayed as string.
    

    /s/But because/Because/

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Graber’s picture

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

Piotr Pakulski’s picture

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

Graber’s picture

Status: Needs work » Needs review
FileSize
1.58 KB

Ok, here goes then, let's see if testbot is ok with this after so many years. Also, I don't expect any regressions here ever so I don't think we need additional test coverage for this.

borisson_’s picture

nitpicks in #22 are still relevant.

Wim Leers’s picture

Status: Needs review » Needs work

For #36.

Let's land this one at last! 🤓 🤝

Graber’s picture

Status: Needs work » Needs review
FileSize
1.57 KB

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

Here goes.

borisson_’s picture

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.

Graber’s picture

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?

Graber’s picture

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

borisson_’s picture

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

Graber’s picture

Aaah, gotya :) Thanks!

Graber’s picture

FileSize
1.57 KB

Fingers crossed ;)

I'll stop the test for this one.
EDIT: I can't actually. Testbot hates us now.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

Looks great!

Status: Reviewed & tested by the community » Needs work

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

Graber’s picture

Status: Needs work » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

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

Graber’s picture

Status: Needs work » Reviewed & tested by the community

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Status: Reviewed & tested by the community » Needs work

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

borisson_’s picture

Status: Needs work » Reviewed & tested by the community

random fail

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs subsystem maintainer review, +Needs tests

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 storage config.


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.

Edit: I realized that I read the patch too quickly and confused \Drupal\Core\Field\FieldItemInterface::schema and \Drupal\Core\Field\FieldItemInterface::propertyDefinitions. Thank you @borisson_ for pointing that out on Slack 🙏 This makes most of the feedback I posted earlier incorrect and irrevelant. I still think we should add additional test coverage to \Drupal\KernelTests\Core\TypedData\TypedDataTest to cover the new data type.

lauriii’s picture

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

Graber’s picture

@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?

lauriii’s picture

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 👍

Graber’s picture

Status: Needs work » Needs review

Ok, I admit this had missing pieces.

Graber’s picture

Hiding the last patch as outdated.

Graber’s picture

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.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

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.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work

Posted few comments in the MR

Graber’s picture

Status: Needs work » Needs review

All threads resolved, back to review.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

Both of the remarks by @lauriii have been resolved.

  • lauriii committed da8c8f98 on 11.x
    Issue #2888763 by Graber, skyredwang, borisson_, Wim Leers, tstoeckler,...

lauriii’s picture

Status: Reviewed & tested by the community » Fixed

Committed da8c8f9 and pushed to 11.x. Thanks!

Piotr Pakulski’s picture

Status: Fixed » Closed (fixed)

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