Decimal separator and precision settings unavailable when aggregating decimal fields

Thanks for your report. Moving to the right component.

Testing on 8.1.2. Tested also on 8.0.x versions.
This is the problem on views since the first stable release of drupal 8.
This problem occurs when aggregation is set on SUM or AVERAGE aggregation function.

Setting this to unassigned for now. Posted the screenshot of the issue.

Well, noone had time/motivation to work on this issue :)

Think it is a bit strange if it not is anyone that is interested to make the settings in Views to be right than using aggregate option.

Well, what about you?

Here is a quick outline of thoughts:

• \Drupal\views\Plugin\views\field\NumericField::buildOptionsForm has support for float fields, using $this->definition['float']
• In \Drupal\views\Plugin\views\display\DisplayPluginBase::getHandlers we initialize the fields, and provide a special way for aggregated fields
• \Drupal\views\Plugin\views\query\Sql::getAggregationInfo defines these special fields
• We could somehow let those special fields define a definition for those fields

This should be totally doable in a BC compatible way.

Unassigning myself for now, but I'll be back.

It seems like this piece of code is responsible for the bad output.

function template_preprocess_views_view_field(&$variables) {
  $variables['output'] = $variables['field']->advancedRender($variables['row']);
}

It seems to me "Precision should be set."

$this->options['set_precision']

or else you'll get:

$remainder = abs($value) - intval(abs($value));
      $value = $value > 0 ? floor($value) : ceil($value);
      $value = number_format($value, 0, '', $this->options['separator']);
      if ($remainder) {
        // The substr may not be locale safe.
        $value .= $this->options['decimal'] . substr($remainder, 2);
      }

A temporary solution could be:

/**
 * Prepares variables for views field templates.
 *
 * Default template: views-view-field.html.twig.
 *
 * @param array $variables
 *   An associative array containing:
 *   - field: The field handler object for the current field.
 *   - row: Object representing the raw result of the SQL query for the current
 *     field.
 *   - view: Instance of the ViewExecutable object for the parent view.
 */
function THEME_preprocess_views_view_field(&$variables) {
  $variables['field']->options['set_precision'] = TRUE;
  $variables['field']->options['precision'] = 2;
  $variables['output'] = $variables['field']->advancedRender($variables['row']);
}

The solution if we can fix this

ISSUE
In "NumericField.php"

  public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    if (!empty($this->definition['float'])) {

This is actually not set if your field is a decimal or a float. :/ (I think it should be ?)
and this allows you to set the precision and other configuration via the views interface and thus resolving the issue. I could put the if switch in comment, but I don't know how it should exactly work.

And I don't know where "definition" is set actually :/

Solved the number of decimals with {{ field_myfield|round(2, 'common') }}. With a little bit more of Twig perhaps I can manage to also make a comma instead of a point.

Try using Twig's "number_format" filter instead of "round":
{{ field_myfield|number_format(2, '.', ',') }}

Arguments:

• decimal: The number of decimal points to display
• decimal_point: The character(s) to use for the decimal point
• thousand_sep: The character(s) to use for the thousands separator

From: http://twig.sensiolabs.org/doc/2.x/filters/number_format.html

If aggregation is enabled then views field definition will be overriden here \Drupal\views\Plugin\views\query\Sql::getAggregationInfo() (See screenshot for more details).

If you want to have an ability to set "Precision", "Thousands marker", "Decimal point" then you need to enable additional options for the float numbers. You can do this by adding "float" option to your field definition. This can be done at least on hook_views_data_alter().

Here is an example:

function MYMODULE_views_data_alter(array &$data) {
  $data['node_field_data']['field_amount']['field']['float'] = TRUE;
}

Dear @nikita_tt,
for me works this:

$data['node__field_test']['field_test']['field']['float'] = TRUE;

Anyway, I think it is just a workaround.

Updated the title a bit to make it clearer what this is about

I tried controlling the decimal precision by using twig in a custom text field, such as the following:

{{ field_value|round }}
{{ field_value|round(1, 'common') }}

But none of these worked. They all returned a 0.

On a side note, no other twig math functions worked either, even for basic arithmetic. Is that a separate bug or are twig functions in the Views UI limited to conditional logic?

It really seems there should be options in the UI to control decimal precision when using aggregation. Any updated ideas on how to achieve this?

#16 is good enough

Hi,

Tried #16 but its not working, the value returned as 0. Since the views uses the order total value (Sum and Average). Any update on this?

Hey all, I created a patch for this.

Can everyone test it and let me know if you find any side-effects. With this patch, you don't need to put anything in your template file or implement any hooks. Aggregated fields will have the same settings that non-aggregated decimal fields have, just like how it works in Drupal 7 right now.

It also fixes the bug of the 'Group column' and 'Group column (additional)' sections missing from a field's Aggregation settings

I overlooked something with patch #33. The 'thousand marker' settings was being duplicated. Fixed with this patch.

Thanks for fixing the whitespace issue @ranjith_kumar_k_u but there's actually a problem with the patch, which I only found out after I saved my view. I'll see if I can find a different way to fix this.

Edit:
Actually I think the patch works. I've been testing on two different sites, one was a vanilla install of drupal 9(using 9.5.x-dev) and the other one, a real site, with some custom modules. I found out that it was one of the custom modules that had caused the issue. It seems to be working fine with the vanilla drupal 9 site. I would love to get others' feedback though.

Ignore my previous patches. This problem seems to have originated from this commit: lets use annotated handlers in even more places

See attached image. In get_aggregation_info(), somebody pasted the field handler where it should've fallen through to the original handler. It seems like a copy/paste error.

This patch corrects that error, removing the field handler from sum, avg, min, max, and stddev_pop

#2735997-38: Decimal separator and precision settings unavailable when aggregating decimal fields While testing with @ramil g we thought through the possible regressions and this may need a CR for those that have implemented #2735997-23: Decimal separator and precision settings unavailable when aggregating decimal fields, but the twig template workaround should continue to work.

This also fixes a bug that we found that when you choose aggregation, the "Aggregation settings" lose the EntityField additional fields and yield a PHP 8.1 Warning (though notice in PHP 7) because it expects the "Group column" and " Group columns (additional)" there but they hide after any other option than "Group results together" is chosen.

Lovely to see older bugs getting fixed!

This issue summary is out of date and does not document the proposed change here. Adding tag. I've added some steps to reproduce because I had to poke around and figure out how to create the problem.

In #39 @joelpittet explains the testing that was done but this is a UI issue so before and after screen shots should be added to the issue summary.

Since no tests are breaking with the change should one be added to prevent future problems?

@quietone, thanks for the review, I've added the proposed change and yes probably could use a regression test.

I can apply the patch successfully and add before and after screenshots. Please review the screenshots.

@asad_ahmed Can you let us know why there is a comma separator in your screenshot?

Created patch for updated version 10.1.x

Resolve #47

Thanks for sharing a pragmatic Twig solution in #16 @Ben Greenberg. Drupal 10 is now on Twig 3, so the URL is https://twig.symfony.com/doc/3.x/filters/number_format.html, but the syntax is the same.

@akram khan Thanks for the patch, could you explain the changes you made so we are all clear as your reroll added more removals than @ramil g did.

We have various screenshots on this issue, so removing the PHP related issue tag and needs screenshots.

Thanks @ramil g. The test looks great and it's red/green on test-only/with fix. 🚀 it!

Moving priority due to the PHP error that is triggered
https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-...

Trigger a PHP error through the user interface, but only under rare circumstances or affecting only a small percentage of all users, even if there is a workaround.

I tested on Drupal 11.x today, standard install and reproduce the problem. Applied the diff and the problem was fixed. Updated credit.

There are no screenshots in the issue summary. Ah, I see they are in #44. @asad_ahmed, when adding screenshots it helps everyone working on the issue if they are available from the issue summary. This save reviewers and committers from hunting through the comments to find the correct images.

Can someone explain why the test is not testing the scenario in the Issue Summary? The test seems to be testing a different problem. It is a problem I encountered when testing this. The problem was that I could not change the aggregate settings from 'Sum' to anything else. Settings to NW to understand the testings.

@quietone I don't believe the screenshots in #44 accurately display the root problem for why the decimal settings change between aggregation being on and off.

I added the screenshot from #38 which identifies where this stems from to the issue summary, which displays the accidental copy/paste error in https://git.drupalcode.org/project/drupal/-/commit/684b4a036e736b16d21a1..., which manifests itself into the originally reported issue as well as other Views UI issues including the error I mentioned in #56 when I bumped the priority and which @ramil g wrote the tests against.

Also I added a bunch more screenshots to the IS illustrating both problems.

One thing I don't know how to change the title to indicate the original issue is resolved + issues with any numeric aggregated field? Any suggestions?

LMK if there’s anything I can do to get this patch committed. It should be a slam dunk, but I may be missing something that’s holding it up.

To reiterate, this issue was introduced accidentally due to a copy/paste error, and it has been there for quite some time.
https://git.drupalcode.org/project/drupal/-/commit/684b4a036e736b16d21a1...

@joelpittet I don't think there was anything in particular holding it up, but I think it's a victim of the following:

1. The RTBC queue has been constantly fluctuating between 75-150 for several months, it feels impossible to get it under 50, so it's easy for individual issues to slip through when the list is very long. This is despite ~150 commits to 11.x in the past month and however many other issues moved to needs work in the same period.

2. It was RTBC and un-RTBCed a couple of times, that meant it wasn't the oldest issue in the RTBC queue until this week, even though it was more-or-less RTBC for a lot longer.

This is the search I use when trying to approach the RTBC queue FIFO:
https://www.drupal.org/project/issues/search/drupal?text=&assigned=&subm...

The 'status changed' column is usually a good indicator, but it's not perfect - because it counts from the most recent status change, not the first change to that status. On the other hand, there are issues that were first RTBCed five years ago and have 100 comments since because they were never really RTBC, so 'first RTBC' would also not be accurate for different issues.

3. The title made the issue look a lot more complex than it was, so at least for me I expected to be looking at it for at least an hour before I'd feel comfortable committing it. It can take longer to get to those issues. Obviously a quick scan of the MR would have indicated it's pretty straightforward but you have to look at them first, and that goes back to point #1 and #2.

None of these are good reasons for an issue to get held up, but I think they probably are the reasons.

Committed/pushed to 11.x and cherry-picked to 11.1.x, 10.5.x, and 10.4.x, thanks!

Thanks for the context, @catch—much appreciated! I was pretty sure it would get in eventually, just didn’t want it to be one of those “not really RTBC” cases.

I’m really glad this bug is finally squashed—I kept running into it with aggregation enabled on all my migrations!

I know this is probably not the right place but in case someone else has the same problem as me, I just wanted to note that this fix breaks the SUM and AVG functions on the commerce_order.total_price__number field using 10.4.6 and Commerce 3.x. Putting back 'field' => 'numeric' in getAggregationInfo() for SUM and AVG makes them working again for the commerce_order.total_price__number field like before. I wanted to share this even if it is only a temporary "bandaid" solution for a couple projects, I will investigate further for a better long term alternative when I have time. Thank you.

This has also caused a regression for us with a pretty simply view after upgrading to 11.1.6

The view uses a COUNT aggregation on the changed field to output the number of nodes changed per month for a given date range. In 11.1.5 this output a numeric count, now it outputs the formatted changed date.

Reverting this commit fixes it again.

I've attached an example view. Should this be reverted?

Steps to reproduce:
- Install standard on 11.1.6
- Install rest module
- Import attached view
- Go to /test/count
- Notice HTML in changed_1 field
- Revert commit
- Notice counts in changed_1 field

Reading through this issue a bit more, it seems like from the last screenshot in the IS the field handler should not have been removed from COUNT and COUNT DISTINCT?

Tracking the regression over in #3517853: Improve test coverage for views aggregation output I've got a test case going so far.

I'm afk at the moment but this needs a revert. See related issue.

The further I dive into this the more I'm seeing that this should be reverted (x-posted with @catch).

We do want to use the Numeric plugin for displaying aggregation data, configuring entity field based formatter settings that get applied to the aggregated value is never going to work properly for all cases.

The real bug here is why the Numeric plugin isn't being used when the field is first being added - this is what causes the original WSOD error because it's using EntityField::buildGroupByForm, then the next time you edit it it's using the parent (HandlerBase) and submitGroupByForm doesn't have the group_columns fields which then passes NULL to array_filter.

We already have some special handling in the Numeric field plugin around float precision when $this->definition['float'] is set, so maybe we can figure out how to use that to allow Decimal settings in aggregation output.

We also obviously need a lot more test coverage for aggregation output, some of which can be seen over in #3517853: Improve test coverage for views aggregation output

Revert MR up

Maybe these errors are connected to this long standing issue: https://www.drupal.org/project/drupal/issues/2230909

Hello,

I've just updated from 10.4.5 to 10.4.6 and it breaks my view that uses aggregation set to "count DISTINCT". The format plural configuration has gone since the 'field' => 'numeric' key/value has been removed from the "count_distinct" under getAggregationInfo() method.

I was using the count DISTINCT and the format plural to display the nodes referencing the listed nodes.

Is there an alternative way to do the same now that the 'field' => 'numeric' is removed, or the removal was a mistake?

More info on this issue I've just opened: Upgrading from 10.4.5 to 10.4.6 removes views format plural on Content ID field.

Thank you.

RTBCing the revert MR.

This reportedly also caused #3518896: hook_views_post_execute(ViewExecutable() not working after update to 10.4.6.

Committed the revert MR to the four relevant branches - this will go out in the next patch release. Back to needs work for the original problem.

@catch, It looks like there may be a regression—thanks for catching that.

Ideally, we'd add more tests here to show the new regression, no? The test in here shows the regression we'd been living with for a while.

See the screenshot of the various problems this solves in #58 #2735997-58: Decimal separator and precision settings unavailable when aggregating decimal fields on top of it looking to be a copy/paste mistake (I keep saying that but looking at the diff it might be hard to see what I am saying), I wish dawehner could confirm this... sigh

There's test coverage for at least one of the bugs this caused in https://git.drupalcode.org/project/drupal/-/merge_requests/11761/diffs I agree we should try to get that in before this issue gets worked on again, but ideally we have even more tests to cover the issues other users have reported.

I don't think this is a copy-paste mistake at all, this is intended and required for aggregation output to be functional, see my comment in #74

@acbramley see the test we have here, there was a regression fixed here as well. The screenshot shows the field handler was copied to all the handlers where in D7 it was only on 2 (that's why it appears to be a copy/paste mistake, it wasn't copied over like that on any of the other related commits at the time, in my deep dive/hunt for the root of the problem). Removing them let the default shine through (EntityField in our case).

https://git.drupalcode.org/project/drupal/-/commit/d15e2ea581dac9e112f15...

The here is the field we are testing aggregation on (id).
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/views...

Sorry if I sound defensive, I am in a rush to get out the door... I will look a bit later to see if I can understand a bit deeper what is going on between the two regressions. It feels like it needs to be some sort of fallback in my guess, but it is likely way more complicated than that.

Removing them let the default shine through (EntityField in our case)

That's my point, you can't use those views handlers on aggregated fields (see my comment above for reasoning)

It works for id because it happens to work on certain fields, see #3517853: Improve test coverage for views aggregation output for more info on that.

Also the test you're pointing to that uses https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/views... doesn't actually test the output of the view itself, just what's on the row result, which isn't actually what gets output. E.g the row result for the timestamp aggregation is correct, but the output isn't.

is there a new release for 10.4.6+?
10.4.6 is still broken and there's no patch here for 10.4.x

@jannakha patch releases are on the first Wednesday of each month, so there'll be a new 10.4 release around 7th May.

Just a note, that applying the diff from the commit to 10.4 does fix the issue for price fields, but it changes the price field formatting structure in views from having the price options to simply having the number format options.

IIRC the pricing format was broken before anyway with aggregation, as I had to add my own $, etc.

Adding related issue #3151654: Date field is not displaying correct value on a views with aggregation max/min, date fields are also totally broken when aggregating.

Reworked MR !10837 following the direction @acbramley outlined in #74.

The numeric handler override in getAggregationInfo() stays (I reverted them, a big false-positive from what looked like a copy/paste error to me originally). The MR now fixes two separate bugs.

1. ConfigHandlerGroup::submitForm() recreated the handler without the aggregation override. The submit handler did not match the handler that built the form. For entity fields this passed NULL to array_filter() and caused the error I mentioned in #56.
2. Views data never set 'float' => TRUE for decimal and float fields. Without that flag, NumericField hides the precision and decimal point options. FieldViewsDataProvider and EntityViewsData now set it. This fixes the original report and matches the workaround from #22.

Base decimal fields also gain numeric filter and argument handlers. Their schema column type is "numeric", which EntityViewsData did not handle before.

The regressions from the reverted commit cannot recur because the handler override is unchanged. New tests cover both bugs and fail without the fixes.

AI disclosure: Generated with the help of an LLM. Poured over the code changes and manually committed the generated pieces as I grasped what they were doing.

@acbramley (or anybody here) could I get a review on this? I feel it solves all the issues we ran into (and with the previous reverted "fix")

Re-writing the issue summary to be clear on this problem and proposed solution, and mention the exception found while working on this issue is split to #3613882: Aggregation settings form builds and submits with different handlers.

Moving the priority to Normal because the exception is in #3613882: Aggregation settings form builds and submits with different handlers

I think this should be ready to go, pipeline is green after rebasing with the regression tests that cover the regression from the original commit, I've also applied the latest changes and tested my client project and that's no longer broken.

The only thing I think may be missing unless I'm not seeing it is test coverage that the decimal and precision settings actually give the desired output in the aggregation output.

New test is looking great!