Simple decimals fail to pass validation

I've added the following test data to NumberTest::providerTestValidStep() on my local machine:

array(20.12345678, 1e-8, TRUE),

and the test still passes. Similarly, executing

use Drupal\Component\Utility\Number;
dpm (Number::validStep(20.12345678, 1.0E-8, 0.0)? 'Yes' : 'No');

at /devel/php prints 'Yes'.

It looks like Number::validStep() is producing a non-zero remainder when it's called via form_validate_number() as opposed to being called directly, but I'm not sure why this is just yet.

I also can confirm this issue. Having a decimal field with precision: 14 and scale: 2, entering the value "3333333" is vaild, while "4444444" will fail Number::validStep vaildation in form_validate_number() in form.inc.

Raising this to major, as decimals are fundamentally broken at this time. Even simple basic precision/scale values seem to fail for reasonable values.

No interdiff provided, as the patch in #6 was basically a terrible hack (as acknowledged by @exlin when he submitted it), and wasn't used in the fix.

My bad.. The approach of not calling validStep for non-floats is wrong. Integers can have steps, so we should call validStep to verify them.

HOWEVER, it seems weird to use a function that is clearly aimed at checking floating-point step (and approximation errors), only for non-floating point numbers.. This because validStep is usually not called for float fields since these have step = any (integers have step=1, and decimals have step = 1^-scale).

So the newly added tests are correct, but I'm really having doubts on the usefulness of the current implementation of validStep. It should probably simply be checking if the value-min is a multiple of step.

@exlin was right.. The only way to do this correctly is to cast step to a string.

Detailed investigation on this function (which is only used once in Drupal), reveals that the first argument ($value) is received as a string, but $step and $offset are floats. PHP seems to slightly mangle the $step value on the first case above from 0.0001 to 0.00010000000000000001438. Passing the step parameter as a string in the case of decimal numbers maintains the correct precision, and allows a correct approximation calculation.

I had the issue described here...

https://www.drupal.org/node/2693467

...which was marked as a duplicate of this one. I applied patch 16 and it fixed my issue.

  /**
   * Verifies that a number is a multiple of a given step.
   *
   * The implementation assumes it is dealing with IEEE 754 double precision
   * floating point numbers that are used by PHP on most systems.
   *
   * This is based on the number/range verification methods of webkit.
   *
   * @param float $value
   *   The value that needs to be checked.
   * @param float $step
   *   The step scale factor. Must be positive.
   * @param float $offset
   *   (optional) An offset, to which the difference must be a multiple of the
   *   given step.
   *
   * @return bool
   *   TRUE if no step mismatch has occurred, or FALSE otherwise.
   *
   * @see http://opensource.apple.com/source/WebCore/WebCore-1298/html/NumberInputType.cpp
   */
  public static function validStep($value, $step, $offset = 0.0) {

The code here seems to suggest this documentation is wrong.

I'd also expect some changes to Drupal\Tests\Component\Utility\NumberTest

Here's a failing test for Number::validStep added into the testonly patch from above.

The fix supplied above doesn't help with validStep not working.

Let's use #22 as the testonly patch.

This patch hopefully addresses the documentation request from @alexpott on #19.

It includes @bc's expanded test from #22, and some minor coding style changes.

Something deep is going on here, and we may have to redo this validStep function from scratch.. But for now, let me just save my current work. Some failing tests are commented out, and others are actually configured to report FALSE when they should be returning TRUE.

What are you doing testbot?

jcventura, the tests that i added should be failing. Once the bug in Number::validStep is fixed, the tests will pass.

I think we should just parse the numbers as strings, rather than dealing with weird float issues. I can rewrite it if there are no objections.

Here's a somewhat complete fix for validStep -- i've adapted code from zend framework's Zend\Validator\Step, but tweaked it to work with very small floats.

I removed a couple tests that don't pass -- if anyone knows how to make them pass, please modify :)

I'll work on it more tonight to try & get them to pass.

oops, the last patch i created was total trash; here is one that only has changes to validStep & the tests.

i added back the original (8.2.x) potentially useless tests that check for things like 100/7 -- will drupal ever be saving the floating point result of integer division into a field value?

tested patch #27 in drupal 8.1.8...fail

can't save/create a field decimal value like 9'999'999.99 or above.

is there any other solution?

i need values up to e.g. $ 999'999'999.99

thanks in advance

tested patch #27 in drupal 8.1.8...fail

can't save/create a field decimal value like 9'999'999.99 or above.

is there any other solution?

i need values up to e.g. $ 999'999'999.99

thanks in advance

@NerdyCrowd have you tried my patch from #37? if it fails can you add the tests that should pass to core/tests/Drupal/Tests/Component/Utility/NumberTest.php?

tested patch #27 on fresh drupal 8.1.8 localhost php7= pass

@bc...i will test your patch #37 within next days

Patch #27 solved a problem I was having on Drupal 8.1.8. I was getting an error with a valid number when submitting a decimal somewhere greater than 3,560,000.00. So this issue doesn't just effect small numbers.

Are there any know down sides to the 8.1.8 patch? One thing in particular I'm seeing is that validation no longer prevents entering a value larger than the DB permits. For example, @NerdyCrowd - I can submit a value as great as 999,999,999.99. I've got my field set to a precision of 12 and a scale of 2. However, I can't submit a value of 12,999,999,999.99 as that gives me an error like this:

Drupal\Core\Entity\EntityStorageException: SQLSTATE[22003]: Numeric value out of range: 1264 Out of range value for column

@inversed
patch #27 tested again in drupal 8.1.10...
I've got my field set to a precision of 16 and a scale of 2...and the biggest value is 90.000.000.000,00. i need values max. 9.999.999.999,99...so this patch does it for me.

Patch #27 fails for me with the following setup:
- Decimal field, precision: 10, scale: 7
- Trying to save this number fails: -3.1933172

The same number saves successfully without the patch.

How is this coming along? I'm running 8.2.4 and tried #37 to fix entry of large numbers like 4,031,239,412.52 on a 20/2 field. While that worked, small numbers like 1109.87 became invalid. For the time being I'm running #16 as it appears to be working in both of these instances.

@pianomansam when I add those numbers to `core/tests/Drupal/Tests/Component/Utility/NumberTest.php` they pass with my patch -- i wonder what is causing the validation failure...

array(4031239412.52, 0.01, TRUE),
array(1109.87, 0.01, TRUE),

also i've just noticed that for whatever reason, when i apply my patch in #37 to 8.2.x latest, these fractional test cases fail:

array(1, 1 / 3, TRUE),
array(-100, 100 / 7, TRUE),
array(2 / 7 + 5 / 9, 1 / 7, 5 / 9, TRUE),

actually i think i know what is causing the validation failure -- the casting issue that jcventura's patch deals with.

maybe the best approach is to combine jcventura's patch in #16 and mine in #37.

@bc Yes, as long as you're passing the value as a float/double to validStep, you've already lost a lot of precision.. Just pass it as a string like I did in #27, and then you can try to optimize the validStep function.

I know it's awful to pass it as a string, but once you look at the values you get at the start of validStep if you don't do that, you'll also concede defeat and simply pass it a string.. Believe me, I tried hard not to have to do it.

Quick work around in case people are stuck without a solution until this is resolved: https://gist.github.com/ummdorian/eaece93165df0bdd6546b07614e955e4

Patch in #16 worked for me with the following scenarios, that were failing without this patch:

- Decimal field, precision: 10, scale: 2
- Trying to save this number: 70000000
- Trying to save this number: 70000000.00

Here's a straight re-roll of #27 for 8.4.x

The test fail is reproducible but somewhat bogus since it's complaining about a class mentioned in comment.

Quick work around, do not validate #step

<?php

  function MY_MODULE_form_alter (array &$form, FormStateInterface $form_state) {
    $form['field_your_field']['widget'][0]['value']['#step']='any';
  }

Not sure if this is a Chrome browser bug, but the with a high precision, using the up/down arrows in the field will sometimes give you a number in scientific notation like 3e-8.

This fails validation even with this patch:

This value is not valid.

The attribute in the HTML is:
step="1.0E-8"

full element:
<input data-drupal-selector="edit-field-chemical-amount-0-value" type="number" id="edit-field-chemical-amount-0-value" name="field_chemical_amount[0][value]" value="3e-8" step="1.0E-8" min="0" placeholder="" class="form-number error" aria-invalid="true" />

I've manually tested and confirmed the use cases in the issue summary and #3, #46, #47, #53 work using the patch in #54. The test fail due to a class mentioned in a comment still exists.

I am updating credit for the triage work. Going forward, you should know that we like to see documented triage steps (even if brief). It is the only way to know if the triage has actually been completed. Here are some made-up examples of documented triage steps:

• I tested the steps to reproduce and they did (or did not) work (so I am tagging it "Needs issue summary update").
• I searched for duplicate issues but could not find any.
• I checked the issue summary and it is accurate and up-to-date.
• Etc...

Thank you!

I'm using #54 patch. For some numbers it does work, but for some not.

13517282.20 - this fails
13517282.21 - this passes

tell me why? :)

The storage is using the default decimal field settings e.g.
precision: 10
scale: 2

I second @Nor4a that those values fails and not as he said. Very odd. I tried looking but the math on validStep is a bit over my head, at least currently :).

And also second the workaround on #56. I ended up doing the same

Adding a similar issue that @larowlan located.

This was discussed on the major triage call by @cottser, @catch, @xjm, @effulgentsia, @cilifen and myself and agreed it was major on the following basis:

• Interfere with normal site visitors' use of the site - validation errors for regular form submissions
• Cause a significant admin- or developer-facing bug with no workaround.

Re-rolling from #27 as #54 ate the $wrong_entries = [] part of the test, picked a comment from #54 and fix of misclassified test from #37.
This patch will fail.

@Nor4a:
To see why that is try setting ini_set('precision', 32);, then it turns out:
0.01 is actually 0.010000000000000000`208166817117217
13517282.20 is 13517282.199999999`254941940307617
13517282.21 is 13517282.210000000`894069671630859
after the 17th significant digit (marked with `) illusion falls apart (not enough bits in double).

The culprit is mentioned in #2, inside Number::validStep() there is:
$remainder = (double) abs($double_value - $step * round($double_value / $step));
This is the trouble maker, especially $step * round($double_value / $step)
because by dividing $double_value by $step you effectively increase its order by the precision of the $step so it becomes 1351728220,
which when multiplied by $step is enough orders of magnitude to bring the rounding errors into play,
and after subtraction it makes enough of a difference (0.00000000186264514923095703125) to go over $computed_acceptable_error (0.00000000059604644775390626240770918829542)

13517282.21 passes because appoximate 13517282.21 is more closely divided by approximate 0.01 than approximate 13517282.20 is.

The code all of this is based on moved to https://github.com/WebKit/webkit/blob/master/Source/WebCore/html/StepRange.cpp but hasn't improved much.

By not reaching into the abyss deeper than is necessary it should be possible to mitigate most of the problems, but you can only do so much with doubles, that's why Zend based fmod patch in #37 has its own problems. Added more test cases mentioned in the issue.

This patch should pass.

Edit: Also the comments on the original make me suspect the author might have been a bit confused about the difference between fixed-point and floating-point representations of the floats.

Fixing comment, Drupal\Component should know nothing about Drupal\Core, even comments.

Test only, should fail.

Note: Number::validStep() states:
The step scale factor. Must be positive.
but it is tested in NumberTest::providerTestValidStep() with
[1000, -10, TRUE],

@tacituseu thank you for your effort in consolidating all of the previous discussion & coming up with a really great fix for this issue! even though the project i'd originally been working on this bug has come & gone, it still haunts me from time to time. in addition to being a comprehensive solution for the bug, you explained your thought process, and educated all of us. i really appreciate your work here.

I'm taking a look at this.

The patch from #72 does seem to fix the issue, but the patch was no longer applying cleanly to 8.5.x branch. Rerolled with fix for the non-applying chunk.

Since I only rerolled the patch to get it applying cleanly, manual testing showed it fixing the issues at hand and the automatic tests are passing:

RTBC'd

Still needs review

1. +++ b/core/modules/field/src/Tests/Number/NumberFieldTest.php
   @@ -72,16 +74,29 @@ public function testNumberDecimalField() {
   +    // Submit a few signed decimal value within the allowed precision and scale.
   +    $valid_entries = [
   ...
   +    foreach ($valid_entries as $valid_entry) {
   

   I wonder why we're not doing this with a dataprovider.

   --
   Oh, because of browser tests, okay - nevermind.

   I still think this looks weird but in the interest of testspeed I understand why this is happening.

2. +++ b/core/tests/Drupal/Tests/Component/Utility/NumberTest.php
   @@ -88,7 +89,28 @@ public static function providerTestValidStep() {
   +      // These floats are valid, but might trigger FP math idiosyncrasies.
   

   Can we rewrite this comment? I understand this sentence up until "valid".

   How about something like this? Not sure if this is even correct.

   These floating numbers are valid, but since floating point numbers are not precise in computing, we are treating these as strings. This is especially so when using small steps.
   
   These testcases test that parsing them as a string resolves that, @see ...
   

Tested on Drupal 8.5.0 and works.

Oops ... Didn't notice this was back to needs work, should probably leave it like that as explained in #81

This updates the comment @borisson_ pointed out and *should* make it clearer.

Since the precision computation code is one of the most complex pieces, I split if off to its own method. This allows for more dedicated documentation and testing, as well as reusability of this generic code which is easy to get wrong.

This does make it a lot easier to read, good work @Xano! I really like the additional testcoverage.

I feel really bad for doing this but:

+++ b/core/tests/Drupal/Tests/Component/Utility/NumberTest.php
@@ -34,6 +34,52 @@ public function testValidStep($value, $step, $expected) {
+  public function testGetPrecision($expected, $number)
+  {

bracket position is incorrect.

Also comment from the part mentioned in #81.1

+    // Submit a few signed decimal value within the allowed precision and scale.
+    $valid_entries = [

they're no longer only signed.

Thanks for the reviews! :)

I'm pretty sure that patch contains too many things @Xano. It went from 10kb to 91kb. I guess you were also working on something else? The interdiff looks great though.

Ah, meh :D I make interdiffs on the same branch, but patches between branches. I guess I diffed against the wrong branch. New patch coming soon!

It helps if you rebase before generating diffs, Xano...

All the nitpicks that were posted since #80 have been resolved, and this was RTBC before.

1.    * The implementation assumes it is dealing with IEEE 754 double precision
      * floating point numbers that are used by PHP on most systems.
      *
      * This is based on the number/range verification methods of webkit.
      *
      * Besides integers and floating numbers, we also support decimal numbers
      * which are not stored in IEEE 754 format. In somewhat higher precisions for
      * these numbers, the $step value cannot accurately represent the desired
      * precision, when it is passed as a float. Passing it as a string bypasses
      * this loss of precision and enables a correct calculation of the step
      * validity.
   

   These docs contradict themselves. "The implementation assumes it is dealing with IEEE 754 double precision" and "which are not stored in IEEE 754 format".

2. +++ b/core/lib/Drupal/Component/Utility/Number.php
   @@ -9,6 +9,34 @@
   +    // Convert non-strings to strings, for consistent and lossless processing.
   +    if (is_float($number)) {
   +      $number = rtrim(number_format($number, 13, '.', ''), '0');
   +    }
   

   This needs to explain why 13 is chosen here.

3. +++ b/core/lib/Drupal/Component/Utility/Number.php
   @@ -33,6 +68,19 @@ class Number {
   +    // If the value is of higher precision than desired it isn't divisible by
   +    // step.
   +    $value_precision = static::getPrecision((string) $double_value);
   

   This documentation is not easy to read. I suggest changing it to something like:
   The step is not valid if the value has a higher precision than the desired precision. This is because the value will not be divisible by the step.

4. +++ b/core/lib/Drupal/Component/Utility/Number.php
   @@ -43,8 +91,9 @@ public static function validStep($value, $step, $offset = 0.0) {
   +    $expected = (double) round($step * round($double_value / $step), $desired_precision);
        // Now compute that remainder of a division by $step.
   

   What is $expected? Also the documentation below the line seems to belong above.

5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/NumberWidget.php
   @@ -73,6 +73,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      '#number_type' => $this->fieldDefinition->getType(),
   
   +++ b/core/lib/Drupal/Core/Render/Element/Number.php
   @@ -88,8 +88,14 @@ public static function validateNumber(&$element, FormStateInterface $form_state,
   +      $step = $element['#step'];
   +      if (isset($element['#number_type']) && ($element['#number_type'] == 'decimal')) {
   +        // PHP mangles the precision of floating-point arguments, so convert
   +        // the step to string for non-floating-point numbers.
   +        $step = (string) $element['#step'];
   +      }
   

   As far as I can see this could is not tested.

6. The amount of type juggling in \Drupal\Component\Utility\Number::validStep() concerns me. It also concerns me that inputs that look the same about from type give different answers. This seems a recipe for head scratching. For example:

         [13, '0.0000000000000'],
         [0, -0.0000000000000],
   

   Perhaps rather than trying to merge all the fix into \Drupal\Component\Utility\Number::validStep we could introduce a new method for decimals aka non IEEE 754 numbers.

I brushed off my math (read: Wikipedia) skills and dug into this a little more.

This needs to explain why 13 is chosen here.

This looks intended to chop off any insignificant digits, but I don't know where 13 comes from. IEEE 754 double precision floats have 53-bit significands, leading to floor(53 * log(2)) = 15 guaranteed significant decimal digits (Kahan, 1997). I briefly tested this manually against a float with 25 decimals and after 15 decimals rounding errors started to show up after conversions. This leads me to believe 13 must instead be 15.

However, PHP does not guarantee it uses IEEE 754 for floats:

Although it depends on the system, PHP typically uses the IEEE 754 double precision format

(emphasis mine, source)

Re #93:
2. This is from #34 but is not part of Zend\Validator\Step, it seems to be a 'test passing constant' ;) as Drupal\Tests\Component\Utility\NumberTest::providerTestValidStep() has this:
[3.9999999999999, 1e-13, TRUE],
4. It is the closest evenly divisible by $step number, or a number it would expect to see if it was a valid step

#34 seems to indicate the 13 comes from zendframework/zend-validator, but it's never been a part of \Zend\Validator\Step.

Also, what's preventing us from adding this Zend package as a dependency?

@Xano: it has its own/similar problems, so it won't pass.

I used the patch at #85.

However, saving data (latitude, longitude) is not working for me.

I created a content entity using Drupal Console, I modified and added the following.

  public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {

    // ... All other fields here.
    // Latitude and Longitude fields.
    foreach (['latitude' => t('Latitude'), 'longitude' => t('Longitude')] as $field_name => $field_label) {

      $fields[$field_name] = BaseFieldDefinition::create('decimal')
        ->setLabel($field_label)
        ->setDescription(t("Clients @field_label.", ['@field_label' => $field_label]))
        ->setSettings([
          'precision' => 18,
          'scale' => 12,
        ])
        ->setDefaultValue(0.00)
        ->setDisplayOptions('view', [
          'label' => 'above',
          'type' => 'decimal',
          'weight' => -4,
        ])
        ->setDisplayOptions('form', [
          'type' => 'decimal',
          'weight' => -4,
        ])
        ->setDisplayConfigurable('form', TRUE)
        ->setDisplayConfigurable('view', TRUE)
        ->setRequired(TRUE);

    }
    
    return $fields;

  }

During data submission, the form structure I get is:

    [#required] => 1
    [#delta] => 0
    [#weight] => 0
    [#type] => number
    [#default_value] => 0.000000000000
    [#placeholder] => 
    [#number_type] => decimal
    [#step] => 1.0E-12
    [#input] => 1

So, when validStep is executed, the precision I always get is 6.

1.0E-12 represents the precision I want, 12, however

    $desired_precision = static::getPrecision($step) + 1;

Always returns 6, causing an error stating the the number is not valid.

Any ideas on how can I enforce the full representation to be used so that it properly calculates the precision?

So, I discovered that if I do this change manually during the buildForm() I can get it to work, even without the patch.

  public function buildForm(array $form, FormStateInterface $form_state) {

    // Other code here

    foreach (['latitude', 'longitude'] as $field) {
      $form[$field]['widget'][0]['value']['#step'] = "0.000000000001";
    }
    return $form;
  }

@alemadlei: 6 is because static::getPrecision() fails to do the expansion if the step is a string

var_dump(is_float('1.0E-12')); // => bool(false)
var_dump(is_float(1.0E-12)); // => bool(true)

and it ends up this way in the form because of:

+        // PHP mangles the precision of floating-point arguments, so convert
+        // the step to string for non-floating-point numbers.
+        $step = (string) $element['#step'];

and

var_dump((string)0.000000000001); // => string(7) "1.0E-12"

Also, 18 digits of precision is too much to ask of a double.

1.0E-12 represents the precision I want, 12, however

That is becauseyour 1.0E-12 is actually a string, and not a numeric value, and currently the only string formats supported are integer and float values. I don't think adding support for all those cases is a good idea. Instead, this API should take a few simple, predictable input types, and user input parsing should be done elsewhere. However, this API cannot be cleaned up without breaking BC, so I suspect the core committers will see adding this support as the only realistic way forward, but it won't be particularly clean.

This should clear up a few things :)

Expecting the same issue at my project.
I have Decimal field with 12 digits and scale=2.
Patch at the #102 fixes this issue.
Thanks @Xano for this fix.

Code at the patch looks good for me.
Moving to Needs Review

1. Testing undesirable behavior using both provided examples.
2. Creating a decimal field on the article content type, with precision 10, scale 4.
3. Trying to create a new article with 99999.0000 in that field: fail with message "test1 is not a valid number."
4. Creating another decimal field on the article content type, with precision 16, scale 8.
5. Trying to create a new article with 20.12345678 in that field: fail with message "test2 is not a valid number."
6. Applying patch: $ curl -l https://www.drupal.org/files/issues/2018-04-18/drupal_2230909_102.patch | git apply -v
7. Applied cleanly.
8. drush cr
9. Retesting step 3: success
10. Retesting step 5: fail with message "test2 is not a valid number."

If the second use case in the description is no longer valid, then this is RTBC. Otherwise, it needs work. I'm not sure, so leaving it as "Needs review".

I've found that existed code doesent check properly out of range numbers.
I have Decimal field with 12 digits and 2 for digits after dot.
I'm trying to enter 10 000 000 000 and getting mysql error: Numeric value out of range:
It means that high border validation doesent checked.

@anpolimus: That would be #1003692: PDOException: SQLSTATE[22003] after entering large value in integer and decimal field.

Re #101: The thing with 1.0E-12 is it looks like it is the patch itself that introduces the problem (see #100), and then tries to undo it with number_format(), maybe replacing the casting with number_format() would make it cleaner

Re #102:

-    $double_value = (double) abs($value - $offset);
+    $float_value = (float) abs($value - $offset);

why change in casting ? they might be somewhat synonymous in PHP but it still feels wrong.

Re #104: about the second case, it does feel wrong to fail like that, either:
1. there should be enforced upper limit on precision (15)
2. do as #93.6 proposed and use some arbitrary precision math library for that

why change in casting ? they might be somewhat synonymous in PHP but it still feels wrong.

They are identical, but "float(ing point number)" is the official name, so the change was made for consistency's and clarity's sake.

This patch introduces a new method strictly for normalizing different numeric values into the simple string format we use in core. It's also a format used by BCMath, so I documented that as well. Furthermore, it addresses lack of test coverage reported in #93 by reverting the change, because the code normalizes all values under the hood now. Additionally, I fixed some documentation.

After discussion with @alexpott on IRC, we concluded that strings using scientific notation will not be supported. Scientific notation is for use in code only (after all, it is syntax sugar for ordinary floats), and string-based numeric values, such as from form input, will have to remain in an integer or plain float (digits with a decimal period) format.

1.

       '#type' => 'number',
       '#title' => t('Scale', [], ['context' => 'decimal places']),
       '#min' => 0,
-      '#max' => 10,
+      '#max' => Number::IEEE_754_GUARANTEED_PRECISION,
       '#default_value' => $settings['scale'],
       '#description' => t('The number of digits to the right of the decimal.'),

If there's to be a limit it should be on $element['precision'] as floating point representation guarantees a specific number of significant decimal digits, and not amount of 'digits to the right of the decimal'.

2.

+  /**
+   * Gets a number's precision.
+   *
+   * @param int|float|string
+   *   The number as a numeric type or a decimal string.
+   *
+   * @return int
+   */
+  public static function getPrecision($number) {

This method name might be a bit confusing with FieldType defining 'precision' as:
'#description' => t('The total number of digits to store in the database, including those to the right of the decimal.'),

Thanks! What about this?

This fixes a typo introduced by the previous patch, and improves some additional variable names, as well as documentation.

This patch adds test coverage to ensure the minimum number of significant digits is indeed enforced as an upper bound when counting a numbers significant decimals, as well as coverage to ensure this bound is not enforced for float-formatted strings, as well as documentation why.

1. Can't help much with naming, as a non-native speaker I've no idea what is the common way to refer to it, the least ambiguous to me seems to be 'decimal places'.
Looked around a bit and at least MySQL, SQL Server and BCMath seem to refer to it as scale though.

2,
+ public function testGetPrecision($expected, $number) {
+ public function provideGetPrecision() {
nitpick, but it still references old function name, also 'provider'.

Edit:
3.

+    $actual_significant_decimals = static::countSignificantDecimals((string) $float_value);

Didn't notice this earlier, but casting to string results in scientific notation in some cases (as shown in #100), which will make it not work as intended, will try to add a test case for that later.

I'm seeing a problem with the value populated into the HTML5 validation attribute which stops a form from submitting. This is using Drupal 8.5.2 and PHP 7.2.

The markup is:

<input data-drupal-selector="edit-amount-0-value" type="number" id="edit-amount-0-value" name="amount[0][value]" value="100.00000000" step="1.0000000000000005E-8" placeholder="" class="form-number">

Note the trailing 5 in the step attribute.

The base field definition is:

    $fields['amount'] = BaseFieldDefinition::create('decimal')
      ->setLabel(t('Threshold Amount'))
      ->setDescription('')
      ->setSetting('precision', '21')
      ->setSetting('scale', '8')
      ->setRequired(FALSE)
      ->addPropertyConstraints('value', ['Range' => ['min' => 0.0]])
      ->setDisplayOptions('view', [
        'label' => 'above',
        'weight' => 5,
      ])
      ->setDisplayOptions('form', [
        'type' => 'string_textfield',
        'weight' => 5,
      ])
      ->setDisplayConfigurable('form', TRUE)
      ->setDisplayConfigurable('view', TRUE);

A quick and dirty fix in \Drupal\Core\Field\Plugin\Field\FieldWidget\NumberWidget::formElement looks roughly like this where the 1st changed line is already part of this patch.

I'm really not sure where in the render flow it's getting mangled, but this is roughly the same solution (handle as string)

--- a/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/NumberWidget.php
+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/NumberWidget.php
@@ -73,12 +73,13 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
       '#type' => 'number',
       '#default_value' => $value,
       '#placeholder' => $this->getSetting('placeholder'),
+      '#number_type' => $this->fieldDefinition->getType(),
     ];
 
     // Set the step for floating point and decimal numbers.
     switch ($this->fieldDefinition->getType()) {
       case 'decimal':
-        $element['#step'] = pow(0.1, $field_settings['scale']);
+        $element['#step'] = $field_settings['scale'] > 0 ? str_pad('0.', $field_settings['scale'] + 1, '0') . '1' : '1';
         break;
 
       case 'float':

Here's a patch with a fix included as above (but using number_format()) plus in \Drupal\Core\Field\Plugin\Field\FieldType\DecimalItem::fieldSettingsForm

Does there need to be added validation for base fields that the scale is not more than Number::IEEE_754_DOUBLE_GUARANTEED_SIGNIFICANT_DECIMALS ?

@pwolanin Thanks!

Didn't notice this earlier, but casting to string results in scientific notation in some cases (as shown in #100), which will make it not work as intended, will try to add a test case for that later.

@tacituseu Good catch! The attached patch should take care of this. It also converts a few code comments from # to //, because I realized this is not Python... ;-)

This adds some documentation, and adds the field-level validation @pwolanin brought up in #117.

Added test case for #114.3 and also one for the other way it could go wrong.

Somehow for #119 only functional tests ran (results), so either I rolled the patch wrong or it introduced new failures, retesting.

@tacituseu: #118 contains a fix (the preg_match() call) for the bug you tried to expose in #122. It basically checks if the string cast results in a number of decimals within the minimum guaranteed range of significant decimals (1-15). If not (because too much precision or scientific notation), it uses the existing number_format() solution). This handles PHP's unreliable float -> string cast, but also floats that are cast reliably, but have less than 15 significant decimals, and applying our number_format() approach to such numbers would result in the numbers being padded to the right until there are 15 decimal digits, which will automatically result in a false positive precision of 15 digits (due to the padding, even though the original float had perhaps only 3 significant decimal digits).

@Xano: looks like this part:

+    $constraints[] = $constraint_manager->create('Range', [
+      'min' => 0,
+      'max' => Number::IEEE_754_DOUBLE_GUARANTEED_SIGNIFICANT_DECIMALS,
+    ]);

is responsible for the failures.

This fixes the test failure by removing the field value constraint I added for unknown reasons (constraints cannot be applied to field settings), and it applies the jargon changes to the tests (thanks to @tacituseu for pointing that out).

The patch in #125 (drupal_2230909_124.patch) fails to apply on Drupal 8.6.0

Reroll for 8.6 and 8.7

I jacked up my previous patch

Attempting this again since this prevents us from editing nodes migrated from D7

I applied this path and it's ok, for me. I review the test and I think that it's ok.

1. This is looking really good. I read all the docs and they look good. I wondered about the language significant decimals and whether it should be significant decimal digits but I was wrong - significant decimals is used on the web so no change necessary for that.

2. +++ b/core/lib/Drupal/Component/Utility/Number.php
   @@ -9,6 +9,72 @@
   +   * @param $number int|float|string
   +   *   The value to normalize. If this is a string, it must be formatted as an
   +   *   integer or a float.
   

   I think it is important to mention that floats with a higher number of significant decimals than IEEE_754_DOUBLE_GUARANTEED_SIGNIFICANT_DECIMALS will lose the additional precision as it is not guaranteed by PHP.

3. +++ b/core/lib/Drupal/Component/Utility/Number.php
   @@ -9,6 +9,72 @@
   +   * @return string
   +   *   The normalized string containing integers with an optional decimal
   +   *   separator (.).
   

   This reads a bit odd. How about The normalised numeric string.? It can contain a minus sign as well for example.

4. +++ b/core/lib/Drupal/Component/Utility/Number.php
   @@ -9,6 +9,72 @@
   +  public static function normalize($number) {
   

   There is no explicit testing of this method. As we're making it public there should be.

5. +++ b/core/lib/Drupal/Component/Utility/Number.php
   @@ -9,6 +9,72 @@
   +  /**
   +   * Counts a number's significant decimals.
   +   *
   +   * @param int|float|string
   +   *   The number whose decimals to count. If this is a string, it must be
   +   *   formatted as an integer or a float.
   +   *
   +   * @return int
   +   */
   +  public static function countSignificantDecimals($number) {
   

   It feels important here to mention that floats are limited to the precision guaranteed by PHP - ie. 15. Hence you get things like

   Number::countSignificantDecimals(-0.000000009000000900009) // equals 15
   

Today tested, @lapek's patch from #131 applies cleanly on 8.8.x HEAD.

To address @alexpott's observations:

1. Grepped in the patch file, "significant decimal digits": zero occurrences; "significant decimals": 10 occurrences.
2. Suggestion appended with a slightly modified wording regarding Grammarly.
3. The term "numeric" inserted into the sentence in question.
4. Adding a unit test for this method is still an outstanding task.
5. Suggestion appended with a slightly modified wording regarding Grammarly.

Will it ever be fixed?
This bug prevents us from every second project for the sale of real estate, when we try to save prices in rubles

@APolitsin could you please provide a unit test to address this last remaining point from my previous list?

"4. Adding a unit test for this method is still an outstanding task."

AFAICT this seems the last remaining todo from @alexpott's comment #135. So if someone writes a unit test, then it can be reviewed and set to RTBC again.

This isn't fixed in core while it exists for 5 years because of a missing test?

1. I think we should add a separate issue to work on the method to count significant decimals. New method Number::countSignificantDecimals() does not affect the current HEAD so it can be reviewed and commited faster.

2. I created #3082919: Add to \Drupal\Component\Utility\Number a method to count significant decimals. It has a new test and changes according to #135 that needs review.

Just confirming that #138 fixes the issue for us.

Postponed due to #142. This issue is not trivial and requires more attention during the review. IMO we should finish #3082919: Add to \Drupal\Component\Utility\Number a method to count significant decimals first.

Thank you! The patch save my time

Seems like this was close to being accepted. Not sure if splitting off Number countSignificantDecimals() is the best idea considering? Maybe it is, but in the case that it isn't, here is the patch from #138 with the Unit test from #3082919: Add to \Drupal\Component\Utility\Number a method to count significant decimals.

+++ b/core/lib/Drupal/Component/Utility/Number.php
@@ -9,6 +9,82 @@
+   * @param int|float|string

Missing parameter name

- @param int|float|string
+ @param $number int|float|string

#138 and #147 patches are failing to apply for 8.9.x and 9.1.x.

Re-roll for 9.1.x-dev.

#151 works well for me on 8.8.x, 8.9.x, and 9.1.x

#151 drupal_2230909_151.patch works well for me on 8.8.5 (php 7.4) where I applied the patch without problems and large numbers seem to work at first glance.

BUT:

I have a 20/2 decimal field - not a default 10/2:

When I enter 123456789123456789.12 it is saving without problems but I get: 123456789123460000.00

When I enter 12345678912346.01 I get the old error "is not a valid number."
Same for 1234567891234.01
When I enter 123456789123.01 it gets saved correctly. No error. And so do apparently do all shorter numbers.

So there is still some inconsistency with the patch here.

Maybe there should be a warning in the interface that remarks the platform's max. of precision on the system (PHP_FLOAT_MAX).

It does not make sense to allow field settings for a larger number than technically possible by the underlying system.

@maxilein can you please check this patch the #154 has been resolved
This is due to the presave in decimal DecimalItem.php and and numberFormat function in core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/DecimalFormatter.php

I don't know why we again change this value in numberFormat while we are saving the correct value in DB.

Working on #156
@maxilein this is due to the number_format function.
The bast solution is to add validation in field settings

@maxilein I have changed the #max value 32 to 16 it's working fine.
The max value supported is 99999999999999.99 which i have checked .
It's meet the #156

Hi,
thank you. For testing I created a new field "testdecimal". Selection of a maximum 16 precision upon creation is ok.
So I chose 16 with scale 2.

But when I enter your maximum:
99999999999999.99 ->1 error has been found: testdecimal is not a valid number.
9999999999999.99 ->1 error has been found: testdecimal is not a valid number.
999999999999.99 -> saves ok. (Thats 14 digits 16-2?)

The behaviour is consistent for the new and also already existing fields.
Maybe the max precision value is misleading? Does it include the scale?

@maxilein yes it's including the scale

Then it is wrong. It is missing 2 digits.
You can see that the working example above in #163 has only 14 digits (12+2), when it should have 16(14+2) like the number you tested.

@maxilein it was working fine for me .but no problem I will test again and will attach a video.

Did your latest patch, patch your patch from #151?
I just used 8.8.6 and applied #162. Maybe thats what I do wrong?

@maxilein i used drupal 9.1 and #162

Ok. I have 8.8.6 on php 7.4 ubuntu 20.04

So then #163 is still a problem. I can only save 14 digits (12+2) - when according to your test it should be 16 (14+2).

It applies to new and existing fields after applying #163

Hi @maxilein yes you are correct .

This is working fine with 14 digits (12+2) so i have changed the patch accordingly .

Looks good to me!

While testing the new patch when i changed the scale of the decimal field, I got the following exception:

Attempt to update field dec failed: Exception thrown while performing a schema update. SQLSTATE[42000]: Syntax error or access violation: 1427 For float(M,D), double(M,D) or decimal(M,D), M must be >= D (column 'field_dec_value').: CREATE TABLE {node__field_dec} ( `bundle` VARCHAR(128) CHARACTER SET ascii COLLATE ascii_general_ci NOT NULL DEFAULT '' COMMENT 'The field instance bundle to which this row belongs, used when deleting a field instance', `deleted` TINYINT NOT NULL DEFAULT 0 COMMENT 'A boolean indicating whether this data item has been deleted', `entity_id` INT unsigned NOT NULL COMMENT 'The entity id this data is attached to', `revision_id` INT unsigned NOT NULL COMMENT 'The entity revision id this data is attached to', `langcode` VARCHAR(32) CHARACTER SET ascii COLLATE ascii_general_ci NOT NULL DEFAULT '' COMMENT 'The language code for this data item.', `delta` INT unsigned NOT NULL COMMENT 'The sequence number for this data item, used for multi-value fields', `field_dec_value` DECIMAL(14, 15) NOT NULL, PRIMARY KEY (`entity_id`, `deleted`, `delta`, `langcode`), INDEX `bundle` (`bundle`), INDEX `revision_id` (`revision_id`) ) ENGINE = InnoDB DEFAULT CHARACTER SET utf8mb4 COMMENT 'Data storage for node field field_dec.'; Array ( ) .

@samiullah Thanks for the checking.
This is due to the max value of scale is 15 and according to the patch the max value of Precision is 14.
The value of scale must be under Precision value.

Thanks
Pavnish

Works for me.

Looks to me like everything else has been addressed but the comment clarification in #135.2.

I changed the comment as proposed in #135, I agree there is no need to further qualify the normalization but it rather can make the docs confusing. Also slight rerolling was needed.