Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Status: Active » Needs review
FileSize
8.67 KB

Here's a first shot.

Used string as the value for DecimalItem. I guess we could add a specific TypedData class, not sure.

tstoeckler’s picture

See #1777268-3: Add a TypedData API for allowing metadata to be associated with data and following about the whole Decimal / Float debate. I don't really know much about the whole thing, but I remembered that discussion so wanted to give a pointer, because I'm pretty sure it's related at the very least.

Berdir’s picture

Thanks. So that discussion essentially said that we do not want to add a Decimal type. Which means use string here as we did not have before and still don't have a native interpretation of this in PHP...

On the other side Language isn't a native data type in PHP either :)

Status: Needs review » Needs work

The last submitted patch, number-field-item-1839076-1.patch, failed testing.

Berdir’s picture

Updates for Contrib module one	Other	LocaleUpdateTest.php	436	Drupal\locale\Tests\LocaleUpdateTest->testUpdateImportSourceRemote()

Haven't seen that one before..

Berdir’s picture

Status: Needs work » Needs review

#1: number-field-item-1839076-1.patch queued for re-testing.

Berdir’s picture

Updated to use static::

fago’s picture

Title: Implement the new entity field API for the number field type » Implement the new entity field API for the field test field type
Status: Needs review » Reviewed & tested by the community

Patch looks gread and has solid test coverage - good work!
-> RTBC as long as it still applies after the comments patch was committed.

fago’s picture

#7: number-field-item-1839076-7.patch queued for re-testing.

fago’s picture

Title: Implement the new entity field API for the field test field type » Implement the new entity field API for the number field type

Status: Reviewed & tested by the community » Needs work

The last submitted patch, number-field-item-1839076-7.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
8.47 KB
22.57 KB

No idea why this failed to apply.

Anyway, here is a re-roll against #1839082: Implement the new entity field API for the field test field type so that I can use the new base test class. The patch for the testbot contains the changes from that other issue so he can run the tests.

Status: Needs review » Needs work

The last submitted patch, number-field-item-1839076-7.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
das-peter’s picture

+++ b/core/modules/number/lib/Drupal/number/Tests/NumberItemTest.php
@@ -0,0 +1,106 @@
+    $float = rand(0, 1000) / 100;
...
+    $new_float = rand(1001, 2000) / 100;

I know this is very nitpicky, but shouldn't we ensure there's always at least one decimal digit? Otherwise this could be a candidate for a mandelbug, e.g. if somewhere the handling of decimal digits fails or vice versa.
Could be done like this:

  do {
    $float = rand(0, 1000) / 100;
  } while(fmod($float, 1) == 0);
Berdir’s picture

#12: number-field-item-1839076-7.patch queued for re-testing.

Berdir’s picture

@das-peter: A float without decimal places seems like a valid value. I'll make sure that it doesn't cause any issues with an explicit test run and confirm but I don't think we need to treat that specially.

Berdir’s picture

Re-roll that uses a fixed float value that does not cause any trouble. We have to fight with enough random fails already :)

fago’s picture

Status: Needs review » Reviewed & tested by the community

Thanks. Patch looks good and das-peter's concerns have been addressed.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

Status: Fixed » Closed (fixed)

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