Problem/Motivation

#2735997: Decimal separator and precision settings unavailable when aggregating decimal fields introduced a major regression in a minor release where views aggregation output was broken for any non string or numeric field. We have some tests around aggregation output, e.g QueryGroupByTest but many of these are either a) on the id or name field, or b) don't test the actual views output, rather testing the result rows themselves which don't surface the bug.

Proposed resolution

Add test coverage

Remaining tasks

Decide how much test coverage we should add
Do it

Issue fork drupal-3517853

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

acbramley created an issue. See original summary.

acbramley opened merge request !11761

acbramley’s picture

acbramley commented
Title: [regression] COUNT aggregation no longer outputs count value for some fields » [regression] COUNT aggregation no longer outputs count value for numeric fields
Issue summary: View changes
Status: Active » Needs review

It seems like string based fields work fine (tested the output of COUNT on the name and that works without the fixed) so perhaps it's a numeric field issue?

acbramley’s picture

acbramley commented
Status: Needs review » Needs work

That breaks testFieldAggregationSettings again, gonna have to dig further into this

acbramley’s picture

acbramley commented

Test is correctly failing now:

       ├ Failed asserting that two strings are equal.
       ┊ ---·Expected
       ┊ +++·Actual
       ┊ @@ @@
       ┊ -'4'
       ┊ +'Thu,·1·Jan·1970·-·10:00\n

I'm debugging how views renders this stuff 🤯

When we get to StylePluginBase::elementPreRenderRow we have the correct count value in $data['#row']. What's happening is that value of 4 is being passed to the timestamp formatter which results in the Jan 1 1970 date (unix epoch)

Before #2735997: Decimal separator and precision settings unavailable when aggregating decimal fields, the field handler was being overridden to numeric, therefore $field in that preRender was the NumericField plugin, not EntityField.

acbramley’s picture

acbramley commented
Title: [regression] COUNT aggregation no longer outputs count value for numeric fields » [regression] Views aggregation no longer output aggregated value for some fields
Issue summary: View changes

Confirmed this isn't just an issue with COUNT either, SUM etc have the same behaviour. I'm starting to think we do need to revert here and come up with a proper fix. The field handlers should be overridden because we don't want to go through the EntityField plugin when outputting aggregation data.

acbramley’s picture

acbramley commented
Issue summary: View changes
acbramley’s picture

acbramley commented
Title: [regression] Views aggregation no longer output aggregated value for some fields » [regression] Views aggregation no longer outputs aggregated value for some field types
acbramley’s picture

acbramley commented

I also confirmed this issue with other field types such as Link

elc’s picture

elc commented

The contrib module Fraction is also affected by this, with aggregation values disappearing.

In Core 10.4.5 the SUM output used Drupal\views\Plugin\views\field\NumericField, but in 10.4.6 it turned up in a Drupal\fraction\Plugin\views\field\FractionField using Drupal\fraction\Fraction\FractionItem::setValue(['numerator' => NN.NN]). setValue wasn't expecting such a value (normally it would be presented with FractionItem::setValue(['decimal' => NN.NN]) or ['numerator' => X, 'denominator' => Y]) resulting in the value always being empty as per the FractionItem::isEmpty().

Using both AUM & AVG in the same view ends up with one affecting the other, where in 10.4.5 that did not happen. I haven't figured out where that is happening.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Priority: Normal » Major

I can also confirm this issue using commerce_reports. This is a major problem as these reports are essential for the stores.

Edit: Actually this is the case for us: #3517926: Upgrading from 10.4.5 to 10.4.6 removes views format plural on Content ID field

joelpittet made their first commit to this issue’s fork.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Since @catch reverted the other issue, I caught up the branch to see if it now passes as I assume is expected now.

acbramley’s picture

acbramley commented
Title: [regression] Views aggregation no longer outputs aggregated value for some field types » Improve test coverage for views aggregation output
Assigned: acbramley » Unassigned
Category: Bug report » Task
Issue summary: View changes

Rescoping this issue for improved test coverage now that the regression was reverted.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Reviewed & tested by the community

I temporarily reverted the revert from #2735997: Decimal separator and precision settings unavailable when aggregating decimal fields locally, including the flawed change to Sql::getAggregationInfo(), and ran these tests to confirm they go red 🔴 as expected.

This is test-only, with no runtime code changes and hope to land this in before #2735997: Decimal separator and precision settings unavailable when aggregating decimal fields which I already checked and also passes with these tests as well locally.

I also ran the full QueryGroupByTest locally: passing on SQLite. The tests passed after I caught up with main in gitlab-ci here and still pass 🟢 after my small local sort fix. The sort criteria are needed because grouped-query row order is database-dependent: it passed on MySQL CI but failed on SQLite without deterministic ordering.

RTBC thanks @acbramley for demonstrating the problem and pointing out my git bisect sleuth work was way off the mark!

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Really hoping to get this in then right after #2735997: Decimal separator and precision settings unavailable when aggregating decimal fields

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue tags: +Quick fix

  • catch committed 90c240f9 on main 
    task: #3517853 Improve test coverage for views aggregation output

By:...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main and 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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