Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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"
Comment | File | Size | Author |
---|
Issue fork drupal-2888763
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:
- 2888763-decimalitem-should-not changes, plain diff MR !4047
Comments
Comment #2
skyredwangLet's see how many tests this change will break.
Comment #4
skyredwangTrying again with "float"
Comment #6
Wim LeersWow, 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.
Comment #7
skyredwangI 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
It appears that this definition was made on the very first commit on this file. No more clues.
Comment #8
Wim LeersSo that was #2218199: Move email and number field types to \Drupal\Core\Field, remove modules.
Have you gone through that issue and verified it wasn't discussed there?
Comment #9
skyredwangNo discussion in that issue, but
core/lib/Drupal/Core/Field/Plugin/Field/FieldType/DecimalItem.php
was renamed fromcore/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
That issue was #2115145: Move field type plugins to Plugin/Field/FieldType
Comment #10
skyredwangcore/modules/number/lib/Drupal/number/Plugin/Field/FieldType/DecimalItem.php
was renamed fromcore/modules/number/lib/Drupal/number/Plugin/field/field_type/DecimalItem.php
, then the very first commit to that file was:The issue was #2015691: Convert field type to typed data plugin for number module
Comment #11
skyredwangOK. 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.
Comment #12
Wim LeersSo then it sounds like is indeed simply an oversight. Let's fix it.
Comment #13
skyredwangI 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 extendsStringData
, and letgetCastedValue()
to "0.0" if the string is empty.Comment #14
dawehnerYeah 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.
Comment #15
skyredwangHere is the patch implementing #13
Comment #16
tstoecklerI 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. inIntegerData
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:
This should break at 80 characters.
This can be
Also this will need some tests. Not actually sure what kind of tests we want, but I know we want some kind of test ;-)
Comment #17
skyredwangaddressing #16
Comment #18
skyredwangWe might want a test in serialization that a decimal field with empty value will be serialized to
'0.0'
Comment #19
e0ipsoI'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
, since0.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" than0.0
. Application code should check the serialized value ifNULL
is not acceptable for the use case. However I'm not 100% happy about that either.Comment #20
tstoecklerRe #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.
Comment #22
borisson_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.
There's a stray space here.
/s/But because/Because/
Comment #33
Graber CreditAttribution: Graber as a volunteer commented+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.
Comment #34
Piotr Pakulskialso +1 from me, the patch looks ok. Voting to push this further.
Comment #35
Graber CreditAttribution: Graber as a volunteer commentedOk, 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.
Comment #36
borisson_nitpicks in #22 are still relevant.
Comment #37
Wim LeersFor #36.
Let's land this one at last! 🤓 🤝
Comment #38
Graber CreditAttribution: Graber as a volunteer commentedAaargh, forgot to add before diff --cached ;)
Here goes.
Comment #39
borisson_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.
Comment #40
Graber CreditAttribution: Graber as a volunteer commentedNot 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?
Comment #41
Graber CreditAttribution: Graber as a volunteer commentedAlso see ItemList.php in the same folder. What's the difference?
Comment #42
borisson_This is also still < 80 chars and makes it a little bit easier to read. Like I said, super tiny change.
Comment #43
Graber CreditAttribution: Graber as a volunteer commentedAaah, gotya :) Thanks!
Comment #44
Graber CreditAttribution: Graber as a volunteer commentedFingers crossed ;)
I'll stop the test for this one.
EDIT: I can't actually. Testbot hates us now.
Comment #45
borisson_Looks great!
Comment #47
Graber CreditAttribution: Graber as a volunteer commentedRe-ran and passed. It was quite obvious the fail was not related. Moving back to RTBC.
Comment #49
Graber CreditAttribution: Graber as a volunteer commentedComment #52
borisson_random fail
Comment #53
lauriiiAt 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.Comment #54
lauriiiRemoving the subsystem maintainer review tag as well because I don't think it's needed anymore.
Comment #55
Graber CreditAttribution: Graber as a volunteer commented@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?
Comment #56
lauriiiWe 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 👍Comment #58
Graber CreditAttribution: Graber as a volunteer commentedOk, I admit this had missing pieces.
Comment #59
Graber CreditAttribution: Graber as a volunteer commentedHiding the last patch as outdated.
Comment #60
Graber CreditAttribution: Graber as a volunteer commentedSo 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.
Comment #61
borisson_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.Comment #62
lauriiiPosted few comments in the MR
Comment #63
Graber CreditAttribution: Graber as a volunteer commentedAll threads resolved, back to review.
Comment #64
borisson_Both of the remarks by @lauriii have been resolved.
Comment #67
lauriiiCommitted da8c8f9 and pushed to 11.x. Thanks!
Comment #68
Piotr Pakulski