Decimal and Float item generates wrong sample values

Please find attached patch

I am a novice so I hope it was ok to upload this patch and move the ticket status to "Needs review"?

Nice find!

We need a small automated test for this as well, to assert that correct values are generated so we don't break it in the future. Thanks!

I don't like that this test relies on a sideeffect, there's no positive test that tests that the generated values are correct.

+++ b/core/modules/field/tests/src/Kernel/Number/NumberItemTest.php
@@ -97,7 +97,15 @@ public function testNumberItem() {
+    // This should through an exception if generated value is out of range.

/s/through/throw/

Here is a patch.

I think #12 is in the wrong issue.

The patch in #12 is for another issue, use the patch in #10.

Setting NW to resolve #11.

Updating version and moving to field system component, which seems a better fit.

Addressed #11

@yogeshmpawar, thanks.

1. +++ b/core/modules/field/tests/src/Kernel/Number/NumberItemTest.php
   @@ -97,7 +97,15 @@ public function testNumberItem() {
   +      ->setSetting('min', 99)
   +      ->setSetting('max', 100);
   

   Why are 99 and 100 used?

2. +++ b/core/modules/field/tests/src/Kernel/Number/NumberItemTest.php
   @@ -97,7 +97,15 @@ public function testNumberItem() {
        $entity->field_decimal->generateSampleItems();
   

   This is only testing generateSampleItems for decimal. I think we should add tests for Float and Integer as well while we are here.

I add the lines that tests for float and integer as @quietone suggested on #20

This is only testing generateSampleItems for decimal. I think we should add tests for Float and Integer as well while we are here.

attaching a patch and a interdiff.

@beatrizrodrigues, thanks.

I noticed some more things this time.

1. +++ b/core/modules/field/tests/src/Kernel/Number/NumberItemTest.php
   @@ -97,7 +97,18 @@ public function testNumberItem() {
   +    $entity->field_decimal
   

   Setting of min/max values needs to be set for all three fields being tested here. Right not the min/max for field_integer and field_float are NULL, so the generated samples can be anything.

   And above this line are two lines that are generating samples values for field_integer and field_float. The sample values are not tested and are soon overwritten a few lines down (the lines added in the patch). These two lines need to be removed. Hope that makes sense.

2. +++ b/core/modules/field/tests/src/Kernel/Number/NumberItemTest.php
   @@ -97,7 +97,18 @@ public function testNumberItem() {
   +    // This should throw an exception if generated value is out of range for
   +    // decimal, integer and float fields, respectively.
   

   Can be deleted because the method generateSampleItems does not throw an exception

3. +++ b/core/modules/field/tests/src/Kernel/Number/NumberItemTest.php
   @@ -97,7 +97,18 @@ public function testNumberItem() {
        $this->entityValidateAndSave($entity);
   

   Add a comment here which is where an error will be found. I like "Confirm that the generated sample values are within range.' but you may prefer something else.

oh, right, @quietone. I work on the points you said. Thanks!

So, I did the changes @quietone suggested, and when I run the tests (now defining min and max to float and integer too) I got the same problem that was happen in DecimalItem, but now, happening in FloatItem. I followed the same logic and I fixed it, and all the tests passed. Here is the new patch, and the interdiff.

Sending patch again because of the phpcs problems that were found.

@beatrizrodrigues, thanks again. This looks much better. I notice that there were coding standard errors in #24. You can run those tests locally. Have a look at the instructions for running the coding standard checks locally so you can be sure the tests will run before uploading a patch. Thanks.

I've gone through the comments again. My question in #20.1 has been answered by myself. The actual values don't matter here, what matters is that the entity save and validation does. There are no other comments that need attention.

The last failing patch was in #8. Since we have changed the tests a new failing patch needs to be made.

@beatrizrodrigues, just in case you don't know, a failing patch is a patch with just the changes to the test. I usually name mine the same as the success patch but with '-fail' before the dot. And then run the test locally to make sure it really does fail. Assuming all is well then upload the failing patch and the success patch in the same comment. Make sure to upload the failing patch first. It is last, when the test fails the issue will be set to NW, which we don't want. I hope all that makes sense.

Thank you so much @quietone for share your knowledge with me. I'm will work on the points you said.

Here's the patch and the failing patch, as was asked. I checked for code standards problems and all seems fine. Thank you again!

@beatrizrodrigues, Awesome! thanks again.

I was surprised that the failing test did not also fail for the field_integer. I looked at \Drupal\Core\Field\Plugin\Field\FieldType\IntegerItem::generateSampleValue correctly sets the min/max values for integers so that is why there is no change for that in this patch. (It has been a long, unusual day for me). Yes, this is ready.

Updated the IS.

RTBC from me.

Testing is begin run on the fail patch, starting a test on the success patch.

Back to RTBC per #30 after #3255836: Test fails due to Composer 2.2 solved the unrelated test failure.

Last test failure looks like a random test failure to me, back to RTBC to trigger retest.

Committed and pushed b2901ce78ca to 10.0.x and 399c0a851e3 to 9.4.x and d277261d2ff to 9.3.x. Thanks!