Validate the min, max and default values for Numeric fields

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

public static function validateMinMax
1. can this be typehinted please

       '#description' => $this->t('The maximum value that should be allowed in this field. Leave blank for no maximum.'),
+
     ];

2. seems unintended

Did not test yet.

Fixing type hint.

Patch #23 is applied and working fine for Drupal version 10.1.x. It can move for RTBC.

Thank you @kkalashnikov but a screenshot of the patch does not provide value, we know that by the CI build. See https://www.drupal.org/about/core/policies/maintainers/how-is-credit-gra... → . Removing credit for that.

Reviewing #23

Confirmed the fields mentioned in the IS does save invalid option (min: 4; max: -4)

Applied patch
Tested all the scenarios in the IS (integer, float, decimal) and I get a warning when trying to save those values.

Good work!

I took a look at the patch.

+++ b/core/modules/field/tests/src/Functional/Number/NumberFieldTest.php
@@ -197,7 +197,7 @@ public function testNumberIntegerField() {
+    // Submit a valid integer.

@@ -207,7 +207,7 @@ public function testNumberIntegerField() {
+    // Try to set a value below the minimum value.

@@ -215,7 +215,7 @@ public function testNumberIntegerField() {
+    // Try to set a decimal value.

@@ -223,7 +223,7 @@ public function testNumberIntegerField() {
+    // Try to set a value above the maximum value.

These changes are out of scope

1. +++ b/core/modules/field/tests/src/Functional/Number/NumberFieldTest.php
   @@ -428,6 +428,64 @@ public function testCreateNumberDecimalField() {
   +  public function testCreateDecimalMaxLessThanMinValidation() {
   

   These and the following should be incorporated into an existing test. There is no need to add additional setup costs for a functional test here.

2. +++ b/core/modules/field/tests/src/Functional/Number/NumberFieldTest.php
   @@ -428,6 +428,64 @@ public function testCreateNumberDecimalField() {
   +    $this->helperValidateFieldConfig('integer', $edit);
   ...
   +    $this->helperValidateFieldConfig('integer', $edit);
   

   Can we use the same field and not re-create one?

3. +++ b/core/modules/field/tests/src/Functional/Number/NumberFieldTest.php
   @@ -451,4 +509,31 @@ public function assertSetMinimumValue(FieldConfigInterface $field, $minimum_valu
   +   * Helper function to test form config submit.
   

   This should tell me what it does. See Coding standards for summary lines → for more details.

4. +++ b/core/modules/field/tests/src/Functional/Number/NumberFieldTest.php
   @@ -451,4 +509,31 @@ public function assertSetMinimumValue(FieldConfigInterface $field, $minimum_valu
   +  public function helperValidateFieldConfig($type, $edit) {
   

   The title should reflect what the function is doing. It isn't doing validation.

Able to reproduce the issue using IS
Verified and tested MR #27 on Drupal 10.1.x-dev
The patch applied cleanly
The issue gets resolved after applying the patch.
Refer to the attached after patch screenshot

Moving back to RTBC. The changes in #26 appear to be addressed
26.1 = was removed
26.2 = helper function was removed and test simplified
26.3 = the test is existing so not sure we should update here
26.4 = help function was removed

This is changing the UI, adding tag.

Hello @quietone, I'm not sure if I understood or fixed the issues about moving the unit test to a kernel class.
Also, I didn't find any other way to test validateMinAndMaxConfig so I tested the method statically.
Maybe, we can also add the test back that I implemented in the other mr
Waiting for some feedback to update the code if necessary.

Left a small comment on MR 4709 about adding an additional test set.

The kernel test looks good but don't see any reason to not include the additional asserts from MR 3436. If it were it's own function/bootstrap then maybe but that's expanding an existing tests. @quietone thoughts?

I added the test min 1 max 0.
Also, the Functional test with a tiny improvement where I added an assert to check the numbers as string values for the Integer type field. Now, as we have the kernel test I don't think we need to test for every number type. Just for integer is enough, but if you guys think is necessary I can add for each number field type.

Sweet! Seems to address the failure from the previous MR.

Hiding patches; I just wasted 20 minutes reviewing an old thing. :P

NW for many things as per my MR comments. Thanks everyone!

This issue Field UI doesn't validate a field's default value on create 🐛 Field UI doesn't validate a field's default value on create Active is very similar, should I fix this issue here added as related issue or it's better to fix in another branch?
Working on code review comments

joaopauloc.dev → closed merge request !3436

Small tweak.

Seems feedback has been addressed!

Thanks @joaopauloc.dev and @smustgrave!

We should avoid mixed if at all possible. PHP 8 supports union types so we can say int|string based on the current test. (This fix includes an API addition and is therefore notbackportable, so we don't need to worry about legacy PHP 7 compatibility for D9.)

Interesting aside that does not actually affect the MR here: Note the behavior of int typehints with integer strings:
https://3v4l.org/LnQvW

Regarding #41, I added that to the related issues for now.

That was my mistake for suggesting it. Didn't know we wanted to avoid mixed but now I do!

Found a few things, commented in the MR. Of most concern is the behavior change which now prevents setting the min = max.

Think this is good for RTBC again.

Committed 2ceafd0 and pushed to 11.x. Also cherry-picked to 10.2.x. Thanks!

lauriii → closed merge request !4709

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