NumericField.php does not support negative value rendering in range -0.xx.

Let's fix this properly in Drupal Core first, with tests etc.

Here is the patch

Here is the patch

Here is patch in specified format

See also: #2332743: views_handler_field_numeric error for float values between zero and negative one. in the Views issue queue.

Issues must be fixed in Drupal 8 first, then backported.

Simpler method using string length after decimal point to determine default precision. This also fixes a floating point precision issue where '1000.1234' would render as '1,000.12339999999995'. Includes tests for both issues.

I really like how this is rewritten! This is absolutely cleaner, I wonder whether number_format maybe changed at some point in time?

+++ b/core/modules/views/src/Tests/Handler/FieldNumericTest.php
@@ -0,0 +1,46 @@
+ * Definition of Drupal\views\Tests\Handler\FieldNumericTest.

This should be "Contains \"... just for consistency

Needs work per #18.

changed to contains

Added some more tests to the ones given in #17, there is more that needs testing with this handler.

Added tests for 'hide_empty' and 'precision'. The precision tests also failed for #17 because of

+++ b/core/modules/views/src/Plugin/views/field/Numeric.php
@@ -131,17 +131,15 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+      $precision = $this->options['set_precision'];

Needs to be $this->options['precision'];

Also the empty check was done after the rewriting instead of before rewriting so a value of 0 would not be considered empty with a precision set because it would be changed before the !==0 check.

Will add failing tests and patch, interdiff is against #17

Had no internet in the train and missed that some more work had been done since yesterday, but #20 did not address the extra issues I saw, so will post my patch.

Rerolled the patch to work with the latest 8.0.x. Especially the renamed file NumericField.php.

Re-rolling patch with #2546416: Rename ViewUnitTestBase to ViewKernelTestBase

Also updated issue summary and title.

A quick note for anyone who wants to manually test this: since #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency there are no entity fields that use the numeric plugin - so you cannot test with a number entity field, you will need to use a views field that actually uses the numeric plugin. Currently only the file count field and user id field does (and not the node id field).
As these fields will never contain negative values, this bug will only affect fields that are using the numeric field plugin outside of core.

Thanks for the reroll.

Added beta eval.

Expanded the test coverage a bit to also include the decimal and separator options, since these are not covered by tests at the moment and they should be.

New patch looks good.

This should be unassigned now, hopefully someone can pick this up and RTBC.

+++ b/core/modules/views/src/Tests/Handler/FieldNumericTest.php
@@ -0,0 +1,103 @@
+ * @file
+ * Contains \Drupal\views\Tests\Handler\FieldNumericTest.
+ */

Nitpick: :)

I assume this needs a reroll in the meantime.

@dawehner, yup, it did, still applied but needed array conversion and the test needed to be moved to the right spot. Also fixed the nitpick :)

+++ b/core/modules/views/tests/src/Kernel/Handler/FieldNumericTest.php
@@ -0,0 +1,98 @@
+    $this->assertEqual($view->field['age']->advancedRender($view->result[0]), '0');
+    $this->assertEqual($view->field['age']->advancedRender($view->result[1]), '0.1234');
+    $this->assertEqual($view->field['age']->advancedRender($view->result[2]), '-0.1234');
+    $this->assertEqual($view->field['age']->advancedRender($view->result[3]), '1,000.1234');
+    $this->assertEqual($view->field['age']->advancedRender($view->result[4]), '-1,000.1234');
...
+    $this->assertEqual($view->field['age']->advancedRender($view->result[0]), '');
+    $this->assertEqual($view->field['age']->advancedRender($view->result[1]), '0.12');
+    $this->assertEqual($view->field['age']->advancedRender($view->result[2]), '-0.12');
+    $this->assertEqual($view->field['age']->advancedRender($view->result[3]), '1,000.12');
+    $this->assertEqual($view->field['age']->advancedRender($view->result[4]), '-1,000.12');

Do you mind using assertEquals and swapping parameters around?

#36 fixed.

Thank you @Lendude!

Unrelated fail

+++ b/core/modules/views/src/Plugin/views/field/NumericField.php
@@ -149,24 +149,24 @@ public function submitOptionsForm(&$form, FormStateInterface $form_state) {
+    if (!empty($this->options['set_precision'])) {
+      $precision = $this->options['precision'];
+    }
+    elseif ($decimal_position = strpos($value, '.')) {
+      $precision = strlen($value) - $decimal_position - 1;
+    }
+    else {
+      $precision = 0;
+    }
+    $value = number_format($value, $precision, $this->options['decimal'], $this->options['separator']);

Moving this to after the hide empty check looks wrong. Because if you set the precision to 0 and the value is 0.1 before it would be hidden if you have the empty zero option.

Here's a test and fix to show what I'm on about. Also https://twitter.com/alexpott/status/856989395991265284

#42 seems related to #1232920: Empty rounded numeric fields not hidden properly, that focusses on decimal fields though.

We don't need to use preg_match - https://3v4l.org/q4VKG ?!?!?

More tests added.

@alexpott thanks for all the test coverage!

Had some minor formatting/comment nitpicks, but in order to see if we have all the possible scenario's covered, I decided to rewrite the testing using a dataprovider.

This just addressed some nits and is a 1-1 rewrite of the tests, nothing new added.

Patch looks solid just minor observation.

1. +++ b/core/modules/views/tests/src/Kernel/Handler/FieldNumericTest.php
   @@ -0,0 +1,133 @@
   +      'precision_2-hide_empty-hide_zero' => [
   ...
   +        [0, 0.1234, -0.1234, 1000.1234, -1000.1234, 0.0001, -0.0001],
   

   We should add empty sting here because it is also testing hide empty. I know we added it blow but we can add it here too.

2. No separator case is also missing.

Doing core mentoring with @Lendude.

Fixed feedback from #46.

Thanks, @Lendude and @Mirnaxvb

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

Great how this simplifies the code too.