Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[UX] Switch the "Precision" and "Scale" fields for decimal numbers from selects into proper number fields (with min/max) #4734

Open
klonos opened this issue Nov 3, 2020 · 10 comments · May be fixed by backdrop/backdrop#4998

Comments

@klonos
Copy link
Member

klonos commented Nov 3, 2020

Screen Shot 2020-11-03 at 11 08 21 pm

@klonos
Copy link
Member Author

klonos commented Nov 3, 2020

BTW, when I do dpm(ini_get('precision'));, I get 14, which seems to be the default for PHP(?). So questions re setting the min/max:

  • Does it make sense to have the precision setting for fields go above that number (in my local, the dropdown has min/max set to 10/20 respectively)? In other words, is it safe to specify '#max' => ini_get('precision'), for the new number FAPI field?
  • What happens if you develop locally, with a certain PHP precision setting, but on your production environment PHP precision is set to a different value?

@klonos
Copy link
Member Author

klonos commented Nov 3, 2020

...more info: https://www.drupal.org/docs/7/api/schema-api/data-types/decimal-numeric

The numeric data type is actually MySQL's Decimal. It allows you to specify the precision and scale of a number. Precision is the total number of significant digits in the number and scale is the total number of digits to the right of the decimal point. For example, 123.4567 has a precision of 7 and a scale of 4.

^^ I would like that (some trimmed/groomed version of it) added to the help text in the UI.

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.

@indigoxela
Copy link
Member

...I get 14, which seems to be the default for PHP(?)

Yep, 14 is the default
But it could, of course, be set to something else in different environments.

Regarding decimal in MariaDB, here's the official info:

  • The maximum number of digits (M) for DECIMAL is 65.
  • The maximum number of supported decimals (D) is 30 before MariaDB 10.2.1 and 38 afterwards.

@klonos
Copy link
Member Author

klonos commented Nov 4, 2020

Thanks @indigoxela 🙏 ...it seems that we have our max values then.

@NormPlum
Copy link

I made a PR for this. I didn't change the min-max values, just used the existing ones.

@indigoxela
Copy link
Member

I made a PR for this.

Many thanks for your PR 🙏 , it seems to work as expected in the sandbox, but NumberFieldSettingsTestCase fails. So this needs some closer inspection.

@indigoxela
Copy link
Member

FTR: I removed the "good first issue" label, as that didn't seem appropriate when a functional test needs fixing. 😉

@NormPlum
Copy link

I added a new commit that updates the values in the test. It was using a precision of 7 which isn't allowed (10 is the minimum). I made it 12 and updated the values to test that works. Also the decimal_separator isn't a setting so I removed that.

Tests still failing though. Don't know why.

@indigoxela
Copy link
Member

Tests still failing though. Don't know why.

That's odd, both tests seem unrelated (neither BackupSettingsTestCase nor ModuleUninstallTestCase are involved in the change).

It might be, that these are (random) false negatives, so here's a trick: Close your PR, wait a minute, then re-open your PR (via GitHub UI). I'd bet, both tests pass then.

@indigoxela
Copy link
Member

It was using a precision of 7 which isn't allowed (10 is the minimum).

Wow, nice catch. Who added 7 there... (🤡 guess who...). And decimal_separator is clearly a formatter setting (display only). Many thanks for cleaning that up. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants