Simple decimals fail to pass validation

Created on 1 April 2014, over 10 years ago
Updated 17 August 2024, 3 months ago

Problem/Motivation

- Create decimal field, set precision to 10 (minimum in the UI and scale to 4
- Saving new node with value 19999.0000 succeeds (precision is 5+4 = 9).
- Saving new node with value 99999.0000 fails (same precision as above).

or

- Create decimal field, set scale to anything over 6 (need 8 to store bitcoin values for example). I used 16 as precission.
- Saving new node with value 20.12345678 fails validation while 0.1234567 succeeds.

This is because Drupal\Component\Utility\Number::validStep() returns false. 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.

Proposed resolution

Bypass weak PHP floating-point handling by passing the step as a string.

Remaining tasks

Merge. Commit. Decimals FTW!

User interface changes

None.

API changes

None.

Data model changes

None.

Possible workaround if you need big decimals

Disable this validation by setting the render element #step to 'any'

i.e.

$element['#step'] = 'any';

In the case of fields, you can do

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

And if you want to target all decimal fields:

/**
 * Prevents validation of decimal numbers
 * @see https://www.drupal.org/node/2230909
 */
function MYMODULE_field_widget_form_alter(&$element, \Drupal\Core\Form\FormStateInterface $form_state, $context) {
  $field_definition = $context['items']->getFieldDefinition();
  if ($field_definition->getType() == 'decimal') {
    $element['value']['#step'] = 'any';
  }
}
šŸ› Bug report
Status

Needs work

Version

11.0 šŸ”„

Component
NumberĀ  ā†’

Last updated 3 months ago

No maintainer
Created by

šŸ‡«šŸ‡®Finland exlin

Live updates comments and jobs are added and updated live.
  • Triaged core major

    There is consensus among core maintainers that this is a major issue. Only core committers should add this tag.

  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • šŸ‡ŗšŸ‡øUnited States smustgrave

    Seems points in #291 have been addressed.

  • Status changed to Needs work over 1 year ago
  • šŸ‡¬šŸ‡§United Kingdom alexpott šŸ‡ŖšŸ‡ŗšŸŒ
    +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/DecimalItem.php
    @@ -69,7 +69,7 @@ public function storageSettingsForm(array &$form, FormStateInterface $form_state
    -      '#max' => 32,
    +      '#max' => ini_get('precision') <= 10 ? 10 : ini_get('precision'),
    
    @@ -79,7 +79,7 @@ public function storageSettingsForm(array &$form, FormStateInterface $form_state
    -      '#max' => 10,
    +      '#max' => ini_get('precision'),
    

    I think we need a update function here for existing fields as we're changing the maximum so current configuration can be invalid. Not sure what we should be doing with existing decimal fields...

  • šŸ‡¬šŸ‡§United Kingdom kiwimind

    Having come across this issue on 9.5.8, applying the patch from https://www.drupal.org/project/drupal/issues/2230909#comment-14626332 šŸ› Simple decimals fail to pass validation Needs work has solved this issue.

    Prior to the patch we were unable to set a default value of a trillion (1 with 12 zeroes) on a 15,2 decimal field. After the patch it has saved as expected.

  • šŸ‡«šŸ‡·France andypost
    +++ b/core/lib/Drupal/Component/Utility/Number.php
    @@ -9,6 +9,71 @@
    +      // If the float has less significant decimals than the number we can
    +      // guarantee, convert it to a string directly.
    +      if (preg_match(sprintf('/^\d+\.\d{1,%d}$/', ini_get('precision')), (string) $number)) {
    +        return (string) $number;
    +      }
    +      // For floats with more significant decimals than the number we can
    +      // guarantee, discard those that are not guaranteed.
    +      return rtrim(number_format($number, ini_get('precision'), '.', ''), '0');
    
    +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/DecimalItem.php
    @@ -69,7 +69,7 @@ public function storageSettingsForm(array &$form, FormStateInterface $form_state
           '#title' => $this->t('Precision'),
           '#min' => 10,
    -      '#max' => 32,
    +      '#max' => ini_get('precision') <= 10 ? 10 : ini_get('precision'),
    
    @@ -79,7 +79,7 @@ public function storageSettingsForm(array &$form, FormStateInterface $form_state
           '#title' => $this->t('Scale', [], ['context' => 'decimal places']),
           '#min' => 0,
    -      '#max' => 10,
    +      '#max' => ini_get('precision'),
    
    +++ b/core/tests/Drupal/Tests/Component/Utility/NumberTest.php
    @@ -34,17 +34,107 @@ public function testValidStep($value, $step, $expected) {
    +   * @covers \Drupal\Component\Utility\Number::normalize
    ...
    +  public function testNormalize($number, $expected) {
    +    $this->assertEquals($expected, Number::normalize($number));
    ...
    +   * Provides data to self::testNormalize().
    

    Checking requirements should go to report/status instead of runtime

    If you need properly configured server no reason to scatter this checks all over codebase

    Also a test should use permutations for precision configured in field and via ini, I mean missing case when ini_get returns wrong data or it <10

  • šŸ‡«šŸ‡·France andypost

    Also this code should be overridable from contrib to allow override math via PHP bc, gmp, decial extensions

  • šŸ‡ŗšŸ‡øUnited States pwolanin

    @anypost - this important patch has been waiting for 7+ years to get committed. we should not expand the scope. If you want to e.g. be able to use BC math, please open a follow-up issue.

  • šŸ‡øšŸ‡¦Saudi Arabia samaphp Riyadh, SA šŸ‡øšŸ‡¦
  • Status changed to Needs review over 1 year ago
  • šŸ‡®šŸ‡³India vinaymahale

    Any more work need to be done here?

  • Status changed to Needs work over 1 year ago
  • šŸ‡ŗšŸ‡øUnited States smustgrave

    #294

    and #296 or follow up least opened.

  • šŸ‡®šŸ‡¹Italy kopeboy Milan

    I'm not an expert developer and couldn't read all the comments and patches, but I hope this can help.

    I was appalled to see I couldn't save web3 wallet balances to my Drupal site (there a lot of decimals places) even if I could easily define my "balance" field (in my custom entity's BaseFieldDefinition) with precision = 65 and scale = 30 (which correctly creates a column in the database with Type: DECIMAL, Lenght: 65,30)...
    These values will come from APIs, but while testing with a manual form input a value of 0.123456789012345678901234567890 would be magically converted to 0.123456789012350000000000000000 in the UI AND database (without validation warnings).
    Strangely, the same input given programmatically (I tested with ECA model, ie. plugin, both in plain text and in YAML format) would become 0.123456789012350000000000000000 (last non-zero digit = 6 instead of 5 šŸ§)

    So I checked core/lib/Drupal/Core/Field/Plugin/Field/FieldType/DecimalItem.php myself and I found that I could fix the problem just by commenting lines 122 to 127:

      /**
       * {@inheritdoc}
       */
      public function preSave() {
        $this->value = round($this->value, $this->getSetting('scale'));
      }

    Note that scale should be already defined by this (right?):

      public static function baseFieldDefinitions(EntityTypeInterface $entity_type): array {
    
        $fields = parent::baseFieldDefinitions($entity_type);
    
        $fields['balance'] = BaseFieldDefinition::create('decimal')
          ->setSettings(array(
            'type' => 'numeric',
            'unsigned' => TRUE,
            'precision' => 65,
            'scale' => 30,
            'not null' => FALSE,
            'description' => t('the decimal integer amount'),
          ))
          ->setLabel(t('Balance'))
          ->setDisplayConfigurable('form', TRUE)
          ->setDisplayConfigurable('view', TRUE);

    So why do we want to round at all when we are already forcing the precision and scale in the field (storage) definition?
    From https://www.php.net/manual/en/function.round I immediately read

    round ā€” Rounds a float

    ,
    but this is a decimal!!

    Then shouldn't that be just an optional Manage Display's setting for decimal fields?
    Also, why isn't the round function called for float number fields (FloatItems.php file in the same folder) instead?

  • šŸ‡¦šŸ‡¹Austria tgoeg

    Also not a dev, but I think I can at least clear up things a little.

    First, this is not a decimal (as strange as it may sound). Your number is a float (mathematically) and SQL stores it as an exact data type DECIMAL with a given precision (which in fact is a float). There is an explicit FLOAT data type in SQL, but that is not guaranteed to be exact.

    The basic underlying problem is that computers "think" binary and not decimally. Representing an arbitrary number with lots of places after the decimal point will inherently produce errors (depending on which number it is, as some ("machine numbers") map cleanly to binary base and some do not. That's why people here see this only with select numbers). See https://en.wikipedia.org/wiki/Round-off_error for details.

    The internal representation of a number 0.123456 might be 0.123456789 because of rounding errors. That's why rounding to the exact given scale does make sense. It's no actual rounding of the given number in the human sense. It just makes sure the number is exactly the one entered and has exactly that many decimal places as configured.

    Does the patch in #2230909-292: Simple decimals fail to pass validation ā†’ not fix it for you? That would be an important information.

  • šŸ‡®šŸ‡³India asad_ahmed

    I have made the changes as per comment #294. And write a update function in Field.install file but i am not sure where to place it, currently i have placed Field.install here core/lib/Drupal/Core/Field/Field.install. Please review it and give feedback if any. Thanks

  • Open in Jenkins ā†’ Open on Drupal.org ā†’
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update 11 months ago
    Custom Commands Failed
  • šŸ‡¦šŸ‡¹Austria maxilein

    patch #305 applies to D10.2 but it does not work.

    I tested a default decimal field (10,2) and a large decimal field (14,2)

    Problem 1: Both act like a standard field with 10 digits.
    Problem 2: the decimal separator is counted as one digit!
    Problem 3: There used to be larger numbers possible than 14.

    Please see attached screenshots.

  • Open in Jenkins ā†’ Open on Drupal.org ā†’
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update 11 months ago
    Patch Failed to Apply
  • šŸ‡¦šŸ‡¹Austria maxilein

    I just realized that #305 is just an addition.

    #292 does not apply to 10.2

  • šŸ‡·šŸ‡ŗRussia APolitsin

    Š¼Š¾Š¹ Š»ŃŽŠ±ŠøŠ¼Ń‹Š¹ тŠøŠŗŠµŃ‚ Š½Š° Š“руŠæŠ°Š»Š¾Ń€Š³Šµ

  • šŸ‡©šŸ‡ŖGermany szeidler Berlin

    Rerolling #292 for Drupal 10.2

  • Open in Jenkins ā†’ Open on Drupal.org ā†’
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update 11 months ago
    Build Successful
  • Open in Jenkins ā†’ Open on Drupal.org ā†’
    Environment: PHP 8.1 & MySQL 5.7 updated deps
    last update 11 months ago
    Build Successful
  • Open in Jenkins ā†’ Open on Drupal.org ā†’
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update 11 months ago
    25,970 pass, 1,784 fail
  • Open in Jenkins ā†’ Open on Drupal.org ā†’
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update 11 months ago
    25,849 pass, 1,787 fail
  • last update 11 months ago
    Build Successful
  • last update 11 months ago
    25,826 pass, 1,797 fail
  • šŸ‡¦šŸ‡¹Austria maxilein

    #309 helps to use large numbers. Thank you very much!!!! We can upgrade to 10.2!

    If we get out of bounds there is just a nasty crash error. Not a warning in the gui.

    1,123,123,123.00 - just ok.
    123,123,123,123 -> crash

    
    The website encountered an unexpected error. Try again later.
    
    Drupal\Core\Entity\EntityStorageException: SQLSTATE[22003]: Numeric value out of range: 1264 Out of range value for column 'field_kostenschaetzung_value' at row 1: INSERT INTO "node__field_kostenschaetzung" ("entity_id", "revision_id", "bundle", "delta", "langcode", "field_kostenschaetzung_value") VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5); Array ( [:db_insert_placeholder_0] => 6479 [:db_insert_placeholder_1] => 19995 [:db_insert_placeholder_2] => fue_projekt_kosten [:db_insert_placeholder_3] => 0 [:db_insert_placeholder_4] => en [:db_insert_placeholder_5] => 123123123123 ) in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 817 of core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).
    
    Drupal\Core\Database\StatementWrapperIterator->execute() (Line: 44)
    Drupal\mysql\Driver\Database\mysql\Insert->execute() (Line: 1400)
    Drupal\Core\Entity\Sql\SqlContentEntityStorage->saveToDedicatedTables() (Line: 971)
    Drupal\Core\Entity\Sql\SqlContentEntityStorage->doSaveFieldItems() (Line: 718)
    Drupal\Core\Entity\ContentEntityStorageBase->doSave() (Line: 486)
    Drupal\Core\Entity\EntityStorageBase->save() (Line: 806)
    Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (Line: 352)
    Drupal\Core\Entity\EntityBase->save() (Line: 270)
    Drupal\node\NodeForm->save()
    call_user_func_array() (Line: 129)
    Drupal\Core\Form\FormSubmitter->executeSubmitHandlers() (Line: 67)
    Drupal\Core\Form\FormSubmitter->doSubmitForm() (Line: 597)
    Drupal\Core\Form\FormBuilder->processForm() (Line: 325)
    Drupal\Core\Form\FormBuilder->buildForm() (Line: 73)
    Drupal\Core\Controller\FormController->getContentResult() (Line: 39)
    Drupal\layout_builder\Controller\LayoutBuilderHtmlEntityFormController->getContentResult()
    call_user_func_array() (Line: 123)
    Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 627)
    Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 124)
    Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97)
    Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 181)
    Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
    Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 58)
    Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
    Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 106)
    Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 85)
    Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48)
    Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
    Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 36)
    Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 51)
    Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 704)
    Drupal\Core\DrupalKernel->handle() (Line: 19)
    
    
  • last update 10 months ago
    30,919 pass, 15 fail
  • last update 10 months ago
    26,069 pass, 1,831 fail
  • Open in Jenkins ā†’ Open on Drupal.org ā†’
    Environment: PHP 8.1 & MySQL 5.7 updated deps
    last update 10 months ago
    Build Successful
  • Open in Jenkins ā†’ Open on Drupal.org ā†’
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update 10 months ago
    Build Successful
  • šŸ‡§šŸ‡ŖBelgium weseze

    Same experience as maxilein.

    The patch fixes our problem for storing correct numbers.
    But if we try and enter bigger numbers we get a nasty WSOD and some SQL out-of-bound errors. Not a warning in the interface.

  • šŸ‡§šŸ‡ŖBelgium keszthelyi Brussels

    @maxilein, @weseze There is another issue and patch for the exception thrown when the entered value is too big: https://www.drupal.org/project/drupal/issues/1003692 šŸ› PDOException: SQLSTATE[22003] after entering large value in integer and decimal field Needs work

  • šŸ‡·šŸ‡ŗRussia APolitsin

    šŸ’„ Friends!
    šŸŽ‰šŸŽ‰šŸŽ‰šŸŽ‰ 10 years to this wonderful issue! šŸŽ‰šŸŽ‰šŸŽ‰šŸŽ‰
    šŸ”„ Created 2 Apr 2014 at 00:19 MSK

    Congratulations to everyone on this important event, especially parent #1 @exlin

    ā˜€ļø Good day, everyone.

  • šŸ‡¦šŸ‡ŗAustralia mstrelan

    I guess that's supposed to be sarcasm. Seems that we need to take 309 and add the test from 305 and review against that. A merge request would probably be helpful. The hook_requirements in 296 hasn't been addressed either I don't think.

  • i am seeing the same problem in D10.2.5...any fixes yet?

  • šŸ‡©šŸ‡ŖGermany Anybody Porta Westfalica

    I can sadly also confirm this core bug which basically makes using decimal fields with a default value impossible.

  • First commit to issue fork.
  • Pipeline finished with Canceled
    5 months ago
    Total: 59s
    #190669
  • Pipeline finished with Failed
    5 months ago
    Total: 478s
    #190672
  • šŸ‡©šŸ‡ŖGermany Grevil

    https://git.drupalcode.org/project/drupal/-/merge_requests/579 still has their target on 9.2.x and can be removed safely.

    New MR targeting 11.x can be found here: https://git.drupalcode.org/project/drupal/-/merge_requests/8282.

  • Pipeline finished with Failed
    5 months ago
    #190682
  • šŸ‡©šŸ‡ŖGermany Grevil

    [...] and add the test from 305 and review against that

    I think you meant the update hook provided in #305? Unfortunately, the update hook from #305 is pretty much a template implementation and won't work in its current state. I'll give it a shot.

    The hook_requirements in 296 hasn't been addressed either I don't think

    I'd agree with @pwolanin in #298:

    @anypost - this important patch has been waiting for 7+ years to get committed. we should not expand the scope [...]

    Any further work on this should be done in a follow-up issue.

  • Pipeline finished with Failed
    5 months ago
    Total: 182s
    #190714
  • Pipeline finished with Failed
    5 months ago
    Total: 350s
    #190713
  • Pipeline finished with Failed
    5 months ago
    Total: 184s
    #190719
  • Pipeline finished with Failed
    5 months ago
    Total: 265s
    #190718
  • Pipeline finished with Failed
    5 months ago
    Total: 186s
    #190724
  • šŸ‡©šŸ‡ŖGermany Grevil

    Looks like everything works as expected! Let's wait for the tests to succeed.

    Here is a static patch of the current MR for the time being (which also applies to 10.2.x, 10.3.x and 10.4.x).

  • Pipeline finished with Failed
    5 months ago
    Total: 344s
    #190723
  • Pipeline finished with Success
    5 months ago
    Total: 584s
    #190727
  • Pipeline finished with Failed
    5 months ago
    Total: 880s
    #190726
  • Status changed to Needs review 5 months ago
  • šŸ‡©šŸ‡ŖGermany Grevil

    Tests are all green now! Ready to get reviewed. šŸ‘

  • Status changed to Needs work 5 months ago
  • The Needs Review Queue Bot ā†’ tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide ā†’ to find step-by-step guides for working with issues.

  • šŸ‡©šŸ‡ŖGermany Anybody Porta Westfalica

    Nice work! Just gave it a try and at least I can now save the numeric fields I wasn't able to safe before! :) YAY

    Sadly the update hook still needs some work:
    The SQL storage cannot change the schema for an existing field (field_example in commerce_product entity) with data.

  • Pipeline finished with Success
    5 months ago
    Total: 588s
    #190815
  • Pipeline finished with Canceled
    5 months ago
    Total: 809s
    #190814
  • Status changed to Needs review 5 months ago
  • šŸ‡©šŸ‡ŖGermany Grevil

    Alright, the field storage should be updatable now, through the in 10.3.x newly introduced "updateFieldStorageDefinition()" method!( https://www.drupal.org/node/3397892 ā†’ )

    Please review (no idea why the review bot set this to needs work, it definitely applies on 11.x).

    Here is the new static patch (Update hook won't work for Drupal < 10.3.x).

  • Pipeline finished with Success
    5 months ago
    Total: 501s
    #190832
  • Pipeline finished with Success
    5 months ago
    Total: 741s
    #190833
  • šŸ‡ŗšŸ‡øUnited States smustgrave

    Probably because there is a 9.2 MR and a mix of patches when probably should just be a single MR and no patches

  • šŸ‡¦šŸ‡¹Austria maxilein

    Thanks for all the sudden speed!!

    I'd love to help by testing it.

    Why are the automated tests only limited choices ( e.g. 10.1.x) ?
    Is this already testable?

  • šŸ‡©šŸ‡ŖGermany Grevil

    @maxilein yes, this is testable if you are using 10.3.x! Just apply https://www.drupal.org/files/issues/2024-06-04/2230909-325.patch ā†’ through composer! :)

    @smustgrave probably... I don't have the permission to close https://git.drupalcode.org/project/drupal/-/merge_requests/579, unfortunately. Anybody with proper permissions, feel free to close it in favor of https://git.drupalcode.org/project/drupal/-/merge_requests/8282.

  • šŸ‡¦šŸ‡¹Austria maxilein

    Sorry. I am still on 10.2
    I don't have 10.3 yet

  • Status changed to Needs work 5 months ago
  • šŸ‡»šŸ‡³Vietnam phthlaap

    The HOOK_field_widget_form_alter has been deprecated, I think the issue summary needs update.

    function hook_field_widget_single_element_form_alter(&$element, \Drupal\Core\Form\FormStateInterface $form_state, $context) {
      $field_definition = $context['items']->getFieldDefinition();
      if ($field_definition->getType() == 'decimal') {
        $element['value']['#step'] = 'any';
      }
    }
    

    https://www.drupal.org/node/3180429 ā†’

  • Status changed to Needs review 5 months ago
  • šŸ‡µšŸ‡¹Portugal jcnventura

    No need to set this to "Needs work" if the workarounds in the issue summary are starting to get outdated. The workarounds are not really part of the issue, and are only there because of our failure to fix this in the last 10 years.

  • šŸ‡ŗšŸ‡øUnited States smustgrave

    Cleaning patches for clarity

    May need a rebase as the test-only feature is failing to run.

    Left some small comments.

  • Pipeline finished with Canceled
    5 months ago
    #207831
  • Pipeline finished with Canceled
    5 months ago
    Total: 225s
    #207830
  • Pipeline finished with Canceled
    5 months ago
    Total: 61s
    #207838
  • Pipeline finished with Canceled
    5 months ago
    #207837
  • Pipeline finished with Failed
    5 months ago
    #207841
  • Pipeline finished with Failed
    5 months ago
    Total: 255232s
    #207840
  • Status changed to Needs work 5 months ago
  • šŸ‡ŗšŸ‡øUnited States smustgrave

    Appears to have consistent Unit test failures.

    But test-only feature is now running and showing coverage!

  • šŸ‡¹šŸ‡­Thailand AlfTheCat

    Just pitching in with my report: after patching Drupal 10.3 I have no more issues with large numbers with large decimal scales. Thanks everyone!

  • šŸ‡¦šŸ‡¹Austria maxilein

    MR !8282 applies to D10.3.1.
    But the new max value seems a bit low.

    I have a decimal field with precison 20 and the update_hook fails on it.
    Is there a way to force the update?

    drush updb
     -------- ----------- --------------- ----------------------------------------
      Module   Update ID   Type            Description
     -------- ----------- --------------- ----------------------------------------
      field    10001       hook_update_n   10001 - Update existing DecimalItem
                                           fields to match the new maximum value.
     -------- ----------- --------------- ----------------------------------------
    
    
     Do you wish to run the specified pending updates? (yes/no) [yes]:
     > y
    
    >  [notice] Update started: field_update_10001
    >  [error]  The SQL storage cannot change the schema for an existing field (field_with_prec_20 in node entity) with data.
    >  [error]  Update failed: field_update_10001
     [error] Update aborted by: field_update_10001
     [error] Finished performing updates.
    
    

    I also tried to use this module: https://www.drupal.org/project/precision_modifier ā†’
    But it is not working (I opened an issue https://www.drupal.org/project/precision_modifier/issues/3460201 šŸ› D10.3.1 on decimal field with precision 20 no check appears Active )

    Is there a way to force this update?

  • šŸ‡¦šŸ‡¹Austria maxilein

    I am confused about the max size of the decimal numbers.
    Why do we limit fields to 14?

    Here https://www.drupal.org/docs/7/api/schema-api/data-types/decimal-numeric ā†’ it says:

    MySQL mappings in the schema.inc file:

    'numeric:normal' => 'DECIMAL',

    We use DECIMAL data type to store exact numeric values, where we do not want any rounding off, but exact and accurate values. A Decimal type can store a Maximum of 65 Digits, with 30 digits after decimal point.

    If Maximum Digits (or Precision) is not defined, It defaults to 10 and if Scale is not defined, it defaults to 0.

    All basic calculations (+, -, *, /) with DECIMAL columns are done with a precision of 65 digits.

    MySQL also says it uses 65 digits.
    https://dev.mysql.com/doc/refman/8.4/en/precision-math-decimal-character...

  • šŸ‡©šŸ‡ŖGermany Anybody Porta Westfalica

    @maxilein Re: #336: Have you read all the comments above? That would be important.
    Especially see #200!

    My impression is, that this change is made to align the database schema max with the max php precision value?

    Floats are complex, so this should definitely please be checked by someone who digged into this in the recent past. E.g. see https://stackoverflow.com/questions/14587290/can-i-rely-on-php-php-ini-p...

  • šŸ‡¦šŸ‡¹Austria maxilein

    Thank you Anybody. I really missed the setting in the php.ini

    My question was: if the database gives us a specified DECIMAL column with a precision of 65 digits - why would we limit ourselves to 14?!
    That does not make any sense. Even if it is the default precision limit of php.

    Maybe we should think of a informative check on the Status page - just to let anybody more easily understand the situation:

    php DECIMAL precision: 14 
    This value can be increased in php.ini: precision = 14 ). The max float precision of your system is: 1.7976931348623E+308
    

    echo "The max float precision of you system is: " . PHP_FLOAT_MAX;

  • šŸ‡¦šŸ‡¹Austria maxilein

    Testing the php.ini precision value with larger precision.

    I have a decimal field with a precision of 20 scale 2.
    (It dates back to when there was no limit by this patch). It has worked ever since.

    When I use the field with a default php precision of 14:

    I can enter numbers within the limits and I can enter something after the decimal point which other than zero.

    When I use the field with a default php precision of 24:

    I can add save large numbers according to the 20/2 settings: 123.123.123.123.123.123,00

    BUT I can not enter other digits other than zero behind the decimal point:
    eg. 123.123.123.123.123.123,01 -> is not a valid number.
    Even when the numbers are really small (e.g. 890.34)

  • šŸ‡¦šŸ‡¹Austria maxilein

    Another observation when precision on the site is set to 24 (instead of the default 14). It probably has nothing to do with decimal fields, but maybe someone of you can think of other potentially unsafe situations.

    False Status Message: Incompatible with PHP 8.3.6: https://www.drupal.org/project/diff/issues/3462791 šŸ› False Status Message: Incompatible with PHP 8.3.6:Diff Active

    the comparison from 8.3 in the info.yml file of the module is compared to a float and fails.
    I have not yet get a feedback of the module developers. So I can't say how they do that comparison.

  • šŸ‡¦šŸ‡¹Austria drupalfan2

    Attached is the same patch as in #325 but with unix line endings.
    Needed to be able to use the patch with an older version of the unix patch command line tool.

  • šŸ‡©šŸ‡ŖGermany Grevil

    @phthlaap what is up with MR 579? Seems quite broken to me?

    @maxilein I think we should create a follow-up issue concerning the higher precision issues. Most people will stay at the default PHP precision level.

  • Pipeline finished with Failed
    3 months ago
    Total: 506s
    #245441
  • Pipeline finished with Failed
    3 months ago
    Total: 544s
    #245442
  • šŸ‡©šŸ‡ŖGermany Anybody Porta Westfalica

    Re #3

    @maxilein I think we should create a follow-up issue concerning the higher precision issues. Most people will stay at the default PHP precision level.

    That's possible if we can be sure that the fixes from this issue are non-destructive. This should not break anything. Otherwise it has to be solved here.

  • Pipeline finished with Success
    3 months ago
    #245464
  • Pipeline finished with Failed
    3 months ago
    Total: 682s
    #245465
  • šŸ‡©šŸ‡ŖGermany Grevil

    Changes by @phthlaap actually introduced new phpunit test failures. I reverted them again.

  • Status changed to Needs review 3 months ago
  • šŸ‡©šŸ‡ŖGermany Grevil

    @smustgrave the unit test failures were introduced by unrelated changes done to the issue branch. The only failures left now are functional javascript failures. And I am fairly certain these are unrelated:

    Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5Test::testAttributeEncoding
    WebDriver\Exception\UnknownError: Unable to execute request for an existing
    session: java.util.concurrent.TimeoutException
    Build info: version: '4.23.0', revision: '77010cd'
    System info: os.name: 'Linux', os.arch: 'amd64', os.version:
    '5.4.266-178.365.amzn2.x86_64', java.version: '17.0.11'
    Driver info: driver.version: unknown

    At least, I see no point on how this is caused by the changes done here.

  • Pipeline finished with Success
    3 months ago
    Total: 505s
    #245538
  • Pipeline finished with Success
    3 months ago
    Total: 579s
    #245539
  • šŸ‡©šŸ‡ŖGermany Grevil

    Ha! I knew the test failures had nothing to do with this patch. Just some 11.x shenanigans going on. Rebasing fixed the tests.

    Please review!

  • šŸ‡©šŸ‡ŖGermany Anybody Porta Westfalica

    RTBC for MR !8282 once again, thank you @Grevil for bringing this right on track again!

    @phthlaap please stop these destructive changes.

  • šŸ‡¦šŸ‡¹Austria maxilein

    I think we must make sure that the following

    bug: I can not enter other digits than zero behind the decimal point on ANY decimal field:
    eg. 123.123.123.123.123.123,01 -> is not a valid number.
    Even on other fields with default precision of 14 and when the numbers are really small (e.g. 890.34)

    is not caused by this patch.
    After all it validates the numbers and I think it is very probable that the error is caused by some routine of this patch.
    And if not: it still seems like a matter of validation to me.

    Can someone point us to the code where this is done?

  • Status changed to Needs work 3 months ago
  • šŸ‡©šŸ‡ŖGermany Grevil

    @maxilein sorry, it sounded like this problem was only reproducible with manually adjusting the precision in the php.ini. But you are correct, this is reproducible:

    Steps to reproduce

    • Leave the default php.ini precision value at "14"
    • Create a decimal field on an entity (e.g. node: article)
    • Set the field precision to "14"

    Input / Output outcome

    123123123123,01 is valid, and the entity can be saved without any problem.

    123123123123123,01 is not valid and saving the entity will result in an error message saying:

    {FieldName} is not a valid number.

    123123123123123123,01 is not valid and will result in an EntityStorageException:

    Drupal\Core\Entity\EntityStorageException: SQLSTATE[22003]: Numeric value out of range: 1264 Out of range value for column 'field_dezimal_value' at row 1: INSERT INTO "node__field_dezimal" ("entity_id", "revision_id", "bundle", "delta", "langcode", "field_dezimal_value") VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5); Array ( [:db_insert_placeholder_0] => 2 [:db_insert_placeholder_1] => 35 [:db_insert_placeholder_2] => article [:db_insert_placeholder_3] => 0 [:db_insert_placeholder_4] => de [:db_insert_placeholder_5] => 1.2312312312312E+17 ) in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 817 of core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).
    Drupal\Core\Database\StatementWrapperIterator->execute() (Line: 44)
    Drupal\mysql\Driver\Database\mysql\Insert->execute() (Line: 1400)
    Drupal\Core\Entity\Sql\SqlContentEntityStorage->saveToDedicatedTables() (Line: 971)
    Drupal\Core\Entity\Sql\SqlContentEntityStorage->doSaveFieldItems() (Line: 718)
    Drupal\Core\Entity\ContentEntityStorageBase->doSave() (Line: 486)
    Drupal\Core\Entity\EntityStorageBase->save() (Line: 806)
    Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (Line: 354)
    Drupal\Core\Entity\EntityBase->save() (Line: 277)
    Drupal\node\NodeForm->save()
    [...]

    Back to needs work again. We also need new tests reproducing the beforementioned issue.

  • šŸ‡¦šŸ‡¹Austria maxilein

    @grevil. Yours is just an ugly unhandled but expected error. You tried to save a value larger than precision 14.

    My bug is different: it is only about the 2 digits behind the comma (,00).

    If default precision 14 is set you can save digits other than zero behind the comma.
    If precision is larger (e.g. 24) you CANNOT save digits other than ,00 behind the comma. That is the bug.

    I suppose there is some routine in this patch evaluating precision wrong therefore dropping the evaluation of digits other than ,00.

  • šŸ‡©šŸ‡ŖGermany Grevil

    @maxilein no. The precision value is

    The number of significant digits displayed in floating point numbers.

    (https://www.php.net/manual/en/ini.core.php#ini.precision)

    All the numbers I used for testing use a precision of 2.

    And as I said in #342, we should create a follow-up issue for any problems with the decimal field when using a higher precision than "14", since in 99% of all cases you don't need a higher precision value.

  • šŸ‡¦šŸ‡¹Austria maxilein

    @grevil: you are mixing precision and scale.

    the bug is messing up the scale validation.

  • šŸ‡©šŸ‡ŖGermany Anybody Porta Westfalica

    This is the largest rabbit hole I've seen for a long time! ;)

    There's another issue: If you have an existing decimal field with for example a precision of 20 you can't save changes to the field anymore, as there's a form validation error:

    Precision must be smaller or equal 14

    which is totally correct, but you're just locked in!

    Trying to change the precision in the field.storage...yml as a hacky workaround is refused by the system by

    The SQL storage cannot change the schema for an existing field (field_height in taxonomy_term entity) with data

    which is also totally fine and correct, but makes the lock-in even harder.

    Any ideas?

    Maybe we can at least find a way to first fix the user-facing issues and afterwards do further clean-up?
    This is a really problematic thing for Drupal as a framework for "simple" (haha) numeric fields! and we already have 10Y and 352 comments. Users being new to Drupal and running into this will be quite confused and unsettled...

  • šŸ‡¦šŸ‡¹Austria maxilein

    @anybody it makes sense to only be able to update fields within the bounds of the set precision of your site.

    In the past - before this patch - it was possible to create larger (>14 precision) fields. That is probably how you ended up with them. That is how I ended up with them.
    I would the say the behavior is as intended and makes sense.

    If you want to change a precision for an existing 20 field. Set php.ini to 20. Then use this module to rescale your precision ā†’ . Then afterwards adjust your php.ini back to the intended size.

Production build 0.71.5 2024